857324 - Tbpl's bottom-right summary pane cuts off entries as of the 2013-03-08 nightly

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Terrence Cole [:terrence]]

When the screen is not wide enough, the bottom right pane will not wrap and entries get hidden. No scroll bar appears when this happens to let us know that there are more entries to look at, they simply disappear.

Comment 1

[Terrence Cole [:terrence]]

Attachment: Good.

This is what the screen looks like when the window is wide enough to show all of the entries.

Comment 2

[Ed Morley [:emorley]]

Huh weird?! Until recently a scrollbar appeared (and in fact, one still appears when using Chrome). Nightly regression?

Comment 3

[Terrence Cole [:terrence]]

Attachment: Bad.

When you shrink the window by a couple of pixels, the bottom two entries simply disappear. There is enough whitespace there for 4 or 5 more columns, so there's no need to not wrap. It would be nice to show more entries here, but as a simple alternative, just making this pane overflow:scroll would at least make TBPL usable.

Comment 4

[Terrence Cole [:terrence]]

(In reply to Ed Morley [:edmorley UTC+1] from comment #2)
> Huh weird?! Until recently a scrollbar appeared (and in fact, one still
> appears when using Chrome). Nightly regression?

/me fires up 17
Yes, indeed, it looks like a regression! Any idea what component I should file this under?

Comment 5

[Arpad Borsos [:Swatinem]]

(In reply to Terrence Cole [:terrence] from comment #4)
> /me fires up 17
> Yes, indeed, it looks like a regression! Any idea what component I should
> file this under?

CSS3 Columns?

Comment 6

[Ed Morley [:emorley]]

Not sure - my usual MO would be to use http://mozilla.github.com/mozregression/ then see if anything leapt out in the 24hr range - and if so, use that component.

Comment 7

[Ed Morley [:emorley]]

Last good nightly: 2013-03-07
First bad nightly: 2013-03-08

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ee4879719f78&tochange=cb432984d5ce

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The style sheet has:

#results {
  margin: 0;
  padding: 5px 0 0 10px;
  position: absolute;
  top: 0;
  bottom: 0;
  left: 500px;
  right: 0;
  overflow: hidden;
  overflow-x: auto;
  /* I really don't know whats going on here,
   * try changing these values using firebug, the columns simply won't
   * respect their set width.
   */
  -moz-column-width: 40em;
  -webkit-column-width: 40em;
  column-width: 40em;
  -moz-column-fill: auto;
  -webkit-column-fill: auto;
  column-fill: auto;
}

So I'm willing to guess this is a regression from http://hg.mozilla.org/mozilla-central/rev/f5b6b9b0157d unless demonstrated otherwise?  Scott, could you have a look?  (First thing to check is that it's actually a regression from that cset.)

Comment 9

[Scott Johnson (:jwir3)]

Hm, so I'm not entirely sure that this is a regression from http://hg.mozilla.org/mozilla-central/rev/f5b6b9b0157d I've been bisecting this, and the following revision:

changeset:   123456:c4bed36b752a
user:        L. David Baron <[e-mail]>
date:        Thu Feb 28 23:46:54 2013 -0800
summary:     Bug 671976:  Add SpecialPowers.gc() calls so bug 671976 assertions show up in correct tests, and correctly annotate assertions.

Also seems to have the issue, although only somewhat intermittently. (That is, it doesn't always happen, but it does sometimes happen when decreasing the horizontal size of the window). See the following video for what I mean (I'm using a build from the above revision for this video):

http://people.mozilla.com/~sjohnson/screencaptures/bug857324.ogv

Comment 10

[Scott Johnson (:jwir3)]

I get something very different on linux than comment 7 indicates. I get the following using mozregression:

Last good nightly: 2012-07-31
First bad nightly: 2012-08-01

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9d2a7a8ca1c7&tochange=582d4c67b3a7

Note that, as I say in comment 9, this seems to be intermittent. It doesn't happen every time I resize the window, but it does happen _sometimes_. In the nightlies I mark as 'good', I can't get the problem to reproduce with a reasonable amount of effort.

Comment 11

[Scott Johnson (:jwir3)]

It could very well be:

http://hg.mozilla.org/mozilla-central/rev/34d30fa24371

Comment 12

[Scott Johnson (:jwir3)]

So my bisecting leads to:

changeset:   101000:34d30fa24371
user:        Scott Johnson <[e-mail]>
date:        Tue Jul 31 11:21:19 2012 -0500
summary:     Bug 764567: Implement column-fill part of CSS3 multicol spec, now with regression fixes [r=roc].

Comment 13

[Scott Johnson (:jwir3)]

This is probably related to the mShouldRevertToAuto member variable, and there's already a bug to get rid of it: bug 846578.

Comment 14

[Alex Keybl [:akeybl]]

Adding qawanted to find out which branches (Aurora/Beta/Release) are impacted by this bug.

Comment 15

[Scott Johnson (:jwir3)]

The reason I think the regression ranges are different is that the problem originally manifested itself in bug 764567, but it was intermittent. Bug 810726 seems to have made it more frequent (i.e. anytime the width of the window is changed, the problem occurs).

Quick status update on this issue:

I have a working solution for this bug, but I'm just looking into issues with assertions from the test for bug 724978 that cropped up when I push the patch.

Comment 16

[Scott Johnson (:jwir3)]

Attachment: b857324 (WIP)

This is my current patch. It's not quite ready for review yet, because, as can be seen from the following try run, the test needs a bit of work:
https://tbpl.mozilla.org/?tree=Try&rev=ddf3c171a92c

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking 22 & 23 for now, we'll wait for QA confirmation that this does not affect 21.

Comment 18

[Scott Johnson (:jwir3)]

Attachment: b857324

What's happening:
During a subsequent call to ReflowChildren while we're trying to find the correct balance height, we overflow vertically. This would normally trigger the mShouldRevertToAuto flag, and restart reflow, but, we call ReflowChildren() again before continuing on to the next iteration of the outer loop (while (mShouldRevertToAuto)). ReflowChildren() resets the mShouldRevertToAuto flag in the beginning of its method invocation.

I've fixed this by 1) eliminating the loop that restarts reflow when we've overflown vertically, 2) refactored this detection out into a separate method called 'EnsureColumnHeight', which will also call ReflowChildren one last time if necessary for the balancing, and 3) renamed the mShouldRevertToAuto member variable to a less confusing 'mHasExcessHeight' member variable, which indicates more clearly that we would normally overflow vertically, if we continued with balancing.

Comment 20

[u279076]

The following builds do NOT reproduce this bug:
* Firefox 17.0.5esr
* Firefox 19.0.2
* Firefox 20.0

The following builds reproduce this bug:
* Firefox 21.0b3
* Firefox 22.0a2 2013-04-18
* Firefox 23.0a1 2013-04-18

Comment 21

[Mats Palmgren (inactive)]

Comment on attachment 739029 [details] [diff] [review]
b857324

This pattern occurs in all three places we reflow the columns:

  bool feasible = ReflowChildren(...);
  feasible = EnsureColumnHeight(..., feasible);

so I suggest we encapsulate it in one place, for example:
rename EnsureColumnHeight to ReflowColumns (or some such) and
incorporate the initial ReflowChildren call into it like so:

nsColumnSetFrame::ReflowColumns(...)
{
  bool feasible = ReflowChildren(...);
  if (aColData.mHasExcessHeight) {
    //height computations...
    ChooseColumnStrategy(...)
    feasible = ReflowChildren(...);
  }
  return feasible;
}

>+  if (aColData.mHasExcessHeight) {
>+    nscoord minComputedHeight = std::min(aReflowState.ComputedHeight(),
>+                                         aReflowState.mComputedMaxHeight);

Redundant since mComputedMinHeight <= ComputedHeight() <= mComputedMaxHeight
is an invariant.  That said, the height computations here seems unnecessary
since ChooseColumnStrategy already does that correctly.

>+    aConfig = ChooseColumnStrategy(aReflowState, true,
>+                                   minComputedHeight,
>+                                   aConfig.mKnownInfeasibleHeight);

So you're forcing a non-balancing reflow (2nd param), but providing
explicit [in]feasable heights (3rd/4th param).  This doesn't make sense
since [in]feasable heights shouldn't be used unless we're balancing.
It looks like this is just for breaking out of the loop inside the
"if (config.mIsBalancing..." block in Reflow() in a roundabout way?

I think it would be better to make that explicit in Reflow() instead.
Simplifying ReflowColumns:

nsColumnSetFrame::ReflowColumns(...)
{
  bool feasible = ReflowChildren(...);
  if (aColData.mHasExcessHeight) {
    ChooseColumnStrategy(aReflowState, true);
    MOZ_ASSERT(aConfig.mColMaxHeight != NS_INTRINSICSIZE);
    ReflowChildren(...); // return value doesn't matter when !mIsBalancing
  }
  return feasible;
}

And then in Reflow(), last in the while-loop:

+      if (!config.mIsBalancing) {
+        // balancing resulted in mHasExcessHeight so we gave up on that
+        break;
+      }
     }
 
-    if (!feasible && !aPresContext->HasPendingInterrupt()) {
+    if (config.mIsBalancing && !feasible && ...) {

It seems easier to comprehend the control flow when making these checks
explicit.  What do you think?


A few other nits:

>+    // We need to reflow our children again because our size has changed.

Has it?

>  // We overflowed vertically, but have not exceeded the number
>+ // of columns. We're going to go into overflow columns now, so balancing
>+ // no longer applies.

"of columns" seems to fit on the first line.

>layout/generic/nsColumnSetFrame.h
>     nscoord mMaxOverflowingHeight;
>-    // Whether or not we should revert back to 'auto' setting for column-fill.
>-    // This happens if we overflow our columns such that we no longer have
>-    // enough room to keep balancing.
>-    bool mShouldRevertToAuto;
>+    // This flag determines whether the last reflow of children exceeded the
>+    // maximum allowed height. If so, we set the height to this maximum
>+    // allowable height, and continue reflow without balancing.
>+    bool mHasExcessHeight;

What does "this maximum allowable height" refer to?  I'm guessing
mColMaxHeight but maybe it could be confused with mMaxOverflowingHeight
on the line above?

>+  bool EnsureColumnHeight(const nsHTMLReflowState& aReflowState, ...

It looks like EnsureColumnHeight/ReflowChildren has the same set of params -
please make their param order the same.

Comment 22

[Scott Johnson (:jwir3)]

Attachment: B857324 (V2)

(In reply to Mats Palmgren [:mats] from comment #21)
> Comment on attachment 739029 [details] [diff] [review]
> b857324
> 
> This pattern occurs in all three places we reflow the columns:
> 
>   bool feasible = ReflowChildren(...);
>   feasible = EnsureColumnHeight(..., feasible);
> 
> so I suggest we encapsulate it in one place, for example:
> rename EnsureColumnHeight to ReflowColumns (or some such) and
> incorporate the initial ReflowChildren call into it like so:
> 
> nsColumnSetFrame::ReflowColumns(...)
> {
>   bool feasible = ReflowChildren(...);
>   if (aColData.mHasExcessHeight) {
>     //height computations...
>     ChooseColumnStrategy(...)
>     feasible = ReflowChildren(...);
>   }
>   return feasible;
> }

Done. Thanks for the suggestion!

> Redundant since mComputedMinHeight <= ComputedHeight() <= mComputedMaxHeight
> is an invariant.  That said, the height computations here seems unnecessary
> since ChooseColumnStrategy already does that correctly.

Yep, you're right. I removed it.

> So you're forcing a non-balancing reflow (2nd param), but providing
> explicit [in]feasable heights (3rd/4th param).  This doesn't make sense
> since [in]feasable heights shouldn't be used unless we're balancing.
> It looks like this is just for breaking out of the loop inside the
> "if (config.mIsBalancing..." block in Reflow() in a roundabout way?
> 
> I think it would be better to make that explicit in Reflow() instead.
> Simplifying ReflowColumns:
> 
> nsColumnSetFrame::ReflowColumns(...)
> {
>   bool feasible = ReflowChildren(...);
>   if (aColData.mHasExcessHeight) {
>     ChooseColumnStrategy(aReflowState, true);
>     MOZ_ASSERT(aConfig.mColMaxHeight != NS_INTRINSICSIZE);
>     ReflowChildren(...); // return value doesn't matter when !mIsBalancing
>   }
>   return feasible;
> }
> 
> And then in Reflow(), last in the while-loop:
> 
> +      if (!config.mIsBalancing) {
> +        // balancing resulted in mHasExcessHeight so we gave up on that
> +        break;
> +      }
>      }
> -    if (!feasible && !aPresContext->HasPendingInterrupt()) {
> +    if (config.mIsBalancing && !feasible && ...) {
> 
> It seems easier to comprehend the control flow when making these checks
> explicit.  What do you think?

Yes, I agree completely. I simplify the control flow even more by a refactoring in bug 861184, but there are some issues with tests that I need to work on before I land that, so it won't block this bug. I took this suggestion as well, as I think it's a good one. Thanks. I changed the assertion a bit, though (see comment below about "maximum allowable height".

> >+    // We need to reflow our children again because our size has changed.
> 
> Has it?

I guess it "may" change. I modified the comment, but I noticed in some test cases if we don't reflow our children again one last time, then we end up with a stale column height (from the previous reflow), which is usually a column height that tried balancing, but realized it couldn't fit in the computed height of the column set. Thus, it's a balance height that overflows in the block direction out of the column set frame's bounds.
 
> >  // We overflowed vertically, but have not exceeded the number
> >+ // of columns. We're going to go into overflow columns now, so balancing
> >+ // no longer applies.
> 
> "of columns" seems to fit on the first line.

It does, except for the period at the end, so in lieu of having a period at the beginning of the next line, I opted for putting "of" on the first line, and "columns" on the second.

> What does "this maximum allowable height" refer to?  I'm guessing
> mColMaxHeight but maybe it could be confused with mMaxOverflowingHeight
> on the line above?

So this means that the height of the anonymous divs inside of the column set frame exceeded the computed height of the column set frame itself when we were balancing. Thus, "maximum allowable height" == the minimum of either aReflowState.mComputedHeight or aReflowState.mComputedMaxHeight.
  
> It looks like EnsureColumnHeight/ReflowChildren has the same set of params -
> please make their param order the same.

I changed it now so the param order matches.

Comment 23

[bhavana bajaj [:bajaj]]

Adding needsinfo on :jwir3, to see if we can do the same backout as we did in Fx20 if that's a less riskier options compared to the upcoming patch ?

Also note, we are going to build with beta 4 tomorrow,which is the last opportunity to land any speculative low risk fix.

Comment 24

[Scott Johnson (:jwir3)]

(In reply to bhavana bajaj [:bajaj] from comment #23)
> Adding needsinfo on :jwir3, to see if we can do the same backout as we did
> in Fx20 if that's a less riskier options compared to the upcoming patch ?
> 
> Also note, we are going to build with beta 4 tomorrow,which is the last
> opportunity to land any speculative low risk fix.

Bhavana: I assume you're talking about the backout patches in bug 810726, which we can do if that's the desired course of action. 

I think that's probably lower risk than uplifting the patch attached to this bug to beta.

Comment 25

[Mats Palmgren (inactive)]

Comment on attachment 740418 [details] [diff] [review]
B857324 (V2)

> > >+ // We need to reflow our children again because our size has changed.
> > 
> > Has it?
> 
> I guess it "may" change. I modified the comment, but I noticed in some test
> cases if we don't reflow our children again one last time, then we end up
> with a stale column height (from the previous reflow), which is usually a
> column height that tried balancing, but realized it couldn't fit in the
> computed height of the column set. Thus, it's a balance height that
> overflows in the block direction out of the column set frame's bounds.

OK, that's a much clearer explanation.  I think the "our size" is a bit
confusing in the comment, since the "our" in "our children" refers to the
ColumnSetFrame itself.  How about:

// We need to reflow our children again since we bailed out of balancing
// and some columns might have the wrong height.

Or just paste your explanation wholesale!


> +      // XXX_jwir3:
> +      // There are specific situations where contentBottom can exceed
> +      // NS_INTRINSICSIZE, which causes a problem, e.g.
> +      // in layout/generic/crashtests/479938-1.html
> +      if (contentBottom <= NS_INTRINSICSIZE &&
> +          (contentBottom > aReflowState.mComputedMaxHeight ||
>             contentBottom > aReflowState.ComputedHeight()) &&
>             aConfig.mBalanceColCount < INT32_MAX) {

I think it's better to leave this as is and skip the assertion in
ReflowColumns.  (I think contentBottom > INTRINSICSIZE should fall
into the same path as other "HasExcessHeight" cases.)

r=mats with those changes.

Comment 26

[Mats Palmgren (inactive)]

Attachment: layout/reftests/columns/columnfill-auto-3.html

Also, could you add this test?  It's the max-height version of your
columnfill-auto-2.html test.  It triggers the assertion ;-)
I think you can use the same -ref file.

The diff is:
@@ -19,9 +19,8 @@
     left: 500px;
     right: 0px;
     top: 0px;
-    bottom: 0px;
     background-color: orange;
-    height: 120px;
+    max-height: 120px;
     -moz-column-width: 640px;
     -moz-column-fill: auto;
     -moz-column-gap: 0;

Comment 27

[Scott Johnson (:jwir3)]

Try server run:
https://tbpl.mozilla.org/?tree=Try&rev=c8e45506ace2

Note: I modified layout/reftests/resize-reflow-001 to have a bit more height, as, on android tbpl this test was failing due to the content slightly exceeding the height, causing it to not balance. This only happens on android, and on tbpl, I can't reproduce it locally either on my desktop or on my phone. My original thought was that it was being caused by font inflation - i.e. the text sizes were larger on android than on desktop, causing it to take up more height. Disabling font inflation didn't have an effect, though, so I'm not quite sure what's happening. Further frustrating the issue is that since android reftests run in opt mode, and just based on the output model of android in general, I can't seem to get any reasonable output from the reftest runs on try.

In any case, I've spent quite a bit of time on this, and I think it's better to just fix the issue by increasing the height slightly, as it doesn't cause the test to differ in functionality AFAICT. (In reply to Mats Palmgren [:mats] from comment #26)

> Also, could you add this test?  It's the max-height version of your
> columnfill-auto-2.html test.  It triggers the assertion ;-)
> I think you can use the same -ref file.

Sure. I'll add it.

Comment 28

[bhavana bajaj [:bajaj]]

(In reply to Scott Johnson (:jwir3) from comment #24)
> (In reply to bhavana bajaj [:bajaj] from comment #23)
> > Adding needsinfo on :jwir3, to see if we can do the same backout as we did
> > in Fx20 if that's a less riskier options compared to the upcoming patch ?
> > 
> > Also note, we are going to build with beta 4 tomorrow,which is the last
> > opportunity to land any speculative low risk fix.
> 
> Bhavana: I assume you're talking about the backout patches in bug 810726,
> which we can do if that's the desired course of action. 

Yep, I meant bug 810726.Given we are going-to-build with beta 4 today & this patch is not even on inbound & would miss the needed testing if we rushed to uplift I'll prefer the backout instead for beta 4. 

in addition,due to the estimated risk here, its not worthwhile landing something risky in our final two beta's as well.
> 
> I think that's probably lower risk than uplifting the patch attached to this
> bug to beta.

Comment 29

[Scott Johnson (:jwir3)]

Try looks green, pushing to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f31e6ad99dfe

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Hmm, did you forget to qref or something? Backed out for Android reftest-3 failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5679169643d

https://tbpl.mozilla.org/php/getParsedLog.php?id=22161139&tree=Mozilla-Inbound

Comment 31

[Scott Johnson (:jwir3)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #30)
> Hmm, did you forget to qref or something? Backed out for Android reftest-3
> failures.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b5679169643d
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=22161139&tree=Mozilla-
> Inbound

Eh... I must have. Sorry. Thanks for backing out, Ryan, I'll try again tomorrow with the correct changeset.

Comment 32

[Scott Johnson (:jwir3)]

(In reply to Mats Palmgren [:mats] from comment #25)
> I think it's better to leave this as is and skip the assertion in
> ReflowColumns.  (I think contentBottom > INTRINSICSIZE should fall
> into the same path as other "HasExcessHeight" cases.)

I handled this by removing the assertion, but as it's been removed, it won't fire for columnfill-auto-3 any longer...

Comment 33

[Scott Johnson (:jwir3)]

Re-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/52127bafd50b

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/52127bafd50b

Comment 35

[Scott Johnson (:jwir3)]

(In reply to bhavana bajaj [:bajaj] from comment #28)
> Yep, I meant bug 810726.Given we are going-to-build with beta 4 today & this
> patch is not even on inbound & would miss the needed testing if we rushed to
> uplift I'll prefer the backout instead for beta 4. 
> 
> in addition,due to the estimated risk here, its not worthwhile landing
> something risky in our final two beta's as well.

I've added a backout patch for Firefox 21 to bug 810726. It's in review, and as soon as it gets r+, I will request approval from you, Bhavana, and uplift it.

Comment 36

[Alex Keybl [:akeybl]]

Can we do the same for Beta 22? a=akeybl if so

Comment 37

[Scott Johnson (:jwir3)]

(In reply to Alex Keybl [:akeybl] from comment #36)
> Can we do the same for Beta 22? a=akeybl if so

I think it would be better to uplift the patch for this bug instead. It's pretty low risk, and there have been quite a few changes to nsColumnSetFrame in Fx22 that make the backout a bit higher risk than I would like...

Comment 38

[Scott Johnson (:jwir3)]

Comment on attachment 740418 [details] [diff] [review]
B857324 (V2)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 810726
User impact if declined: regression where some circumstances will make columns not correctly balance
Testing completed (on m-c, etc.): m-c, some aurora 
Risk to taking this patch (and alternatives if risky): not incredibly risky. The only risk would be an invalid column rendering (which is what is happening right now anyway). It would allow us to keep the column-fill feature in Fx22.
String or IDL/UUID changes made by this patch: none.

Comment 39

[Alex Keybl [:akeybl]]

(In reply to Scott Johnson (:jwir3) from comment #37)
> (In reply to Alex Keybl [:akeybl] from comment #36)
> > Can we do the same for Beta 22? a=akeybl if so
> 
> I think it would be better to uplift the patch for this bug instead. It's
> pretty low risk, and there have been quite a few changes to nsColumnSetFrame
> in Fx22 that make the backout a bit higher risk than I would like...

If this causes new regressions though, wouldn't that mean we'd be backing out the feature later in the release?

Comment 40

[Alex Keybl [:akeybl]]

(In reply to Alex Keybl [:akeybl] from comment #39)
> If this causes new regressions though, wouldn't that mean we'd be backing
> out the feature later in the release?

(this or any of the dependent patches)

Comment 41

[Alex Keybl [:akeybl]]

Basically, I'm wondering if we should just bite the bullet now and backout rather than wait and backout if we cause a new web regression.

Comment 42

[Alex Keybl [:akeybl]]

Comment on attachment 740418 [details] [diff] [review]
B857324 (V2)

We haven't had the time to create a backout patch, and Scott reiterated in email that it would be much riskier to backout. Let's take this forward fix.

Comment 43

[Scott Johnson (:jwir3)]

Alex:

Can I assume the minor refactorings in bug 861181 and 861182 are also approved, since this bug depends on them?

Comment 44

[Scott Johnson (:jwir3)]

Landed on beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/8ca75debb97a

Comment 45

[Paul Silaghi, QA [:pauly]]

This is not entirely fixed.
STR:
1. Start latest nightly
2. Open https://tbpl.mozilla.org/
3. Click Ubuntu opt - d
4. Open View/Sidebar/History

AR:
Bottom scrollbar not ok:
http://img189.imageshack.us/img189/7366/noscroll.png

ER:
Resize the sidebar a little - bottom scrollbar ok
http://img845.imageshack.us/img845/2413/withscroll.png

Comment 46

[Scott Johnson (:jwir3)]

(In reply to Paul Silaghi [QA] from comment #45)
> This is not entirely fixed.
> STR:
> 1. Start latest nightly
> 2. Open https://tbpl.mozilla.org/
> 3. Click Ubuntu opt - d
> 4. Open View/Sidebar/History
> 
> AR:
> Bottom scrollbar not ok:
> http://img189.imageshack.us/img189/7366/noscroll.png
> 
> ER:
> Resize the sidebar a little - bottom scrollbar ok
> http://img845.imageshack.us/img845/2413/withscroll.png

I can confirm that this does happen on Linux 64bit with the May 22 nightly.

*sigh* I guess I'll have to look into this again and see what went wrong.

Comment 47

[Scott Johnson (:jwir3)]

(In reply to Scott Johnson (:jwir3) from comment #46)
> (In reply to Paul Silaghi [QA] from comment #45)
> > This is not entirely fixed.
> > STR:
> > 1. Start latest nightly
> > 2. Open https://tbpl.mozilla.org/
> > 3. Click Ubuntu opt - d
> > 4. Open View/Sidebar/History
> > 
> > AR:
> > Bottom scrollbar not ok:
> > http://img189.imageshack.us/img189/7366/noscroll.png
> > 
> > ER:
> > Resize the sidebar a little - bottom scrollbar ok
> > http://img845.imageshack.us/img845/2413/withscroll.png
> 
> I can confirm that this does happen on Linux 64bit with the May 22 nightly.
> 
> *sigh* I guess I'll have to look into this again and see what went wrong.

Actually, on second look, I can't reproduce this issue. I tried a number of times, and, at first, thought I could, but this was just my debug build acting extremely slowly for tbpl.mozilla.org, and not reflowing as quickly as I would expect it to.

Comment 48

[Scott Johnson (:jwir3)]

> Actually, on second look, I can't reproduce this issue. I tried a number of
> times, and, at first, thought I could, but this was just my debug build
> acting extremely slowly for tbpl.mozilla.org, and not reflowing as quickly
> as I would expect it to.

Is there a width range (even in really abstract terms... like "I move the window from being the width of the screen so left side covers up the 'JP' text on OS X 10.8 before releasing the mouse" or something to that effect) that you are able to consistently reproduce this issue? 

Also, could you look in about:buildconfig and give me the revision that your nightly is built off of?

Comment 49

[Paul Silaghi, QA [:pauly]]

I made a screencast for a better understanding on latest nightly 24.0a1 (2013-05-29) rev e58336e81395, Win 7 x64: http://screencast.com/t/W46TpKqY2Re
It is 100% reproducible on FF 22b3, 23.0a2 (2013-05-29), 24.0a1 (2013-05-29).

Comment 50

[Scott Johnson (:jwir3)]

(In reply to Paul Silaghi [QA] from comment #49)
> I made a screencast for a better understanding on latest nightly 24.0a1
> (2013-05-29) rev e58336e81395, Win 7 x64: http://screencast.com/t/W46TpKqY2Re
> It is 100% reproducible on FF 22b3, 23.0a2 (2013-05-29), 24.0a1 (2013-05-29).

Paul:

Thank you for the screencast! It definitely clears up the use case you're describing, and I can now reproduce it. However, my research indicates that this bug exists prior to bug 810726 landing, which means that, although it's very similar to this bug, it's not a regression from bug 810726. 

Thus, I'll open another bug to track this issue.

Comment 51

[Paul Silaghi, QA [:pauly]]

The work continues in bug 877716.
Verified fixed this one.