508816 - Scrollbox overflows on the wrong side in RTL mode

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Marco Bonardo [:mak]]

If you are in RTL mode, when a scrollbox overflows it will do it on the right, while it should instead overflow on the left.

Expected result:
LTR [element1 element2 elem]

RTL [ent3 element2 element1]

while currently RTL behaves like LTR.

The tabbar is using a trick to workaround this:

> if (this.tabContainer.mTabstrip._isRTLScrollbox) {
>   /* In RTL UI, the tab is visually added to the left side of the
>    * tabstrip. This means the tabstip has to be scrolled back in
>    * order to make sure the same set of tabs is visible before and
>    * after the new tab is added */
>
>   this.tabContainer.mTabstrip.scrollByPixels(this.mTabs[0].clientWidth);
> }

and i'm using a similar trick in bug 382466, but this should not be necessary.

Comment 1

[Neil Deakin]

Do you have a testcase that shows what you mean? Do you mean that the scrollbar appears on the right or that the scrolled view isn't scrolled to the right edge? The latter I think is because of layout issues with xul boxes, see bug 192767.

Comment 2

[Marco Bonardo [:mak]]

the scrolled view is not scrolled to the right

Comment 3

[Marco Bonardo [:mak]]

Attachment: test

maybe this test could allow you to see the problem, i tried it with FORCE RTL extension. the last hbox works correctly while both scrollbox are not scrolled to the right.

Comment 4

[Neil Deakin]

OK then, dbaron added a special check for xul scrollframes in bug 192767 that makes them initially scrolled left (in nsGfxScrollFrameInner::GetScrolledRect). Maybe he can let us know what the issues here were.

Comment 5

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

See bug 192767 comment 56 through bug 192767 comment 61.  I couldn't get the testcases in bug 192767 comment 57 to produce sensible results with my new approach, so I gave up and just made them behave the same as they did before the patch.

Comment 6

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

And I don't remember exactly what the issues were, but I think it was things like the scrollbars being out of sync with the display or the display scrolling "past the edge" of the area that was supposed to be scrollable.

Comment 7

[Dão Gottwald [:dao]]

I removed || mIsXUL from nsGfxScrollFrameInner::GetScrolledRectInner and couldn't see a difference in attachment 212448 [details]...

Comment 8

[Dão Gottwald [:dao]]

It doesn't seem to make a difference for this bug's testcase either. I'm not sure what its point is, though.

Comment 9

[Dão Gottwald [:dao]]

This has been identified as bug 610685's underlying problem.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I do not want to block on this, but assigning to Simon, who hopefully can fix it for FF4.

Comment 11

[Dietrich Ayala (:dietrich)]

Bug 610685 is a betaN blocker, which depends on this bug getting fixed, so this does actually need to block.

Comment 12

[Simon Montagu :smontagu]

Attachment: Layout patch (w-i-p)

This fixes the test cases in attachment 212448 [details], but causes a bunch of failures on tryserver which I need to investigate. It doesn't fix bug 610685.

Comment 13

[Simon Montagu :smontagu]

Attachment: Remove workarounds in browser

This removes the two blocks marked as workarounds for this bug. I think there must be other unmarked things that need to be removed or modified, though.

Comment 14

[Marco Bonardo [:mak]]

Comment on attachment 503635 [details] [diff] [review]
Remove workarounds in browser

I'm not sure how much feedback you need on this... I tried applying both patches and clobbering my build. I've made a new profile and set intl.direction.en = rtl, then restarted browser.
Creating multiple bookmarks on bookmarks toolbar and tabs on tabbar still seems to scroll the wrong side here, bookmarks are cutoff and app-tab is not there. If I maximize the window the scrollbox doesn't seem to "snap" back to the right side, as well.

Comment 15

[Dão Gottwald [:dao]]

Attachment: testcase, block vs. box layout

Comment 16

[Johnathan Nightingale [:johnath]]

This bug seems a little sleepy, but it's a hardblocker blocking another hardblocker. Can we get an estimate for when there will be a reviewable patch, here?

Comment 17

[Simon Montagu :smontagu]

Attachment: Layout patch w-i-p, v.2

Comment 18

[Simon Montagu :smontagu]

Attachment: Remove workarounds in browser, v.2

Comment 19

[Simon Montagu :smontagu]

Attachment: Remove workarounds for marquee

Comment 20

[Simon Montagu :smontagu]

With this version of the patch Dão's testcase looks good wrt to horizontal positioning at least. Unfortunately there are also differences in vertical positioning and sizing between the two blocks, so it won't work as a reftest.

Pinned tabs in RTL also work fine. There was an issue with the scrolling arrows at the right and left edge of the tabstrip, which required removing the RTL special casing in scrollbox.xml. There is one remaining issue with tabs: when the tabstrip is scrolled to the beginning (right) and one opens a new tab or jumps to an existing tab that is currently not in view (e.g. with Cmd-9 or ctrl-tab), the current tab doesn't scroll into view. I'm not sure if this is a problem in my patch or because of another workaround that needs to be removed. If someone can point me to where we handle this case, that would be great.

The changes to setting size from GetMinSize caused a lot of failures in marquee reftests. Attachment 505456 [details] [diff] fixes this by backing out most of bug 407016.

Comment 21

[Dão Gottwald [:dao]]

(In reply to comment #20)
> There is one remaining issue with tabs: when
> the tabstrip is scrolled to the beginning (right) and one opens a new tab or
> jumps to an existing tab that is currently not in view (e.g. with Cmd-9 or
> ctrl-tab), the current tab doesn't scroll into view. I'm not sure if this is a
> problem in my patch or because of another workaround that needs to be removed.
> If someone can point me to where we handle this case, that would be great.

This should be handled by ensureElementIsVisible in scrollbox.xml

Comment 22

[Simon Montagu :smontagu]

Attachment: Layout patch v.3

This version passes tryserver without needing any workarounds in marquee.

I'm not very happy about this part:

+  if (childRect.x == 0 || originalRect.IsEmpty())
+    // only check minSize on initial reflow

That seems like a repugnant hack, but I couldn't find any other way to get the same sizes in the two RTL cases in attachment 50409 [details]

Comment 23

[Simon Montagu :smontagu]

Attachment: Remove workarounds in browser, v.3

Comment 24

[Dão Gottwald [:dao]]

Comment on attachment 506352 [details] [diff] [review]
Remove workarounds in browser, v.3

I don't understand the scrollPaddingRect code, switching review request to Enn.

Comment 25

[Neil Deakin]

Comment on attachment 506352 [details] [diff] [review]
Remove workarounds in browser, v.3

>+          if (ltr) {
>+            innerRect.left = outerRect.left - this._scrollbox.scrollLeft;
>+            innerRect.right = innerRect.left + this._scrollbox.scrollWidth;
>+          } else {
>+            innerRect.right = outerRect.right - this._scrollbox.scrollLeft;
>+            innerRect.left = innerRect.right - this._scrollbox.scrollWidth;
>+          }

You add in one line and subtract in three. It seems like there should be an even number of each.

I never fully understood this method either. Might be good to ask mstange about it. The rest of the patch is ok though.

For a simple test opened in rtl:

<scrollbox maxwidth="100" style="overflow: scroll;">
  <button label="One"/>
  <button label="Two"/>
  <button label="Three"/>
  <button label="Four and the rest of the numbers go here"/>
</scrollbox>

I get an assertion:

###!!! ASSERTION: curPos.x not a multiple of device pixels: 'curPosDevPx.x*appUnitsPerDevPixel == curPos.x', file /builds/moz2/ready/layout/generic/nsGfxScrollFrame.cpp, line 1756

Comment 26

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

Comment on attachment 506351 [details] [diff] [review]
Layout patch v.3

>+  if (childRect.x == 0 || originalRect.IsEmpty())
>+    // only check minSize on initial reflow

I'm not happy about this either.  What happens without it?

>+    if (minSize.width > childRect.width) {
>+      if (!mInner.IsLTR())
>+        childRect.x += childRect.width - minSize.width;

>+    if (childRect.width < mInner.mScrollPort.width)
>+    {
>+      if (!mInner.IsLTR())
>+        childRect.x += childRect.width - mInner.mScrollPort.width;

In these two cases, it might be clearer to swap the operands of the - and change the += to a -=, since it makes it clearer that you're reducing the value of .x .

Otherwise this looks fine.

Comment 27

[Simon Montagu :smontagu]

I'm flailing around in circles here, I need to do some kind of brain dump.

The difference in the position of the green block in attachment 504099 [details] seems to be caused by that call to GetMinSize() in nsXULScrollFrame::LayoutScrollArea.

In the block layout case, the size of the scrolled frame (mInner.mScrolledFrame) is no greater than the scroll port (mInner.mScrollPost.Size), even though it has a child element which is wider than the scroll port. The frame's overflow rect includes the size of the child element.

In the box layout case, the call to GetMinSize forces the scrolled frame to expand to the size of the child element. Later on, when placing the fixed-position child in nsHTMLReflowState::InitAbsoluteConstraints, this size is returned by GetHypotheticalBoxContainer, in cbWidth, and this pushes the fixed-position child too far to the right by the extra size.

Comment 28

[(no longer active)]

Simon, it's hard for me to tell whether you've got things under control here, or whether you need help.  Let me know if you need help.  My plate is almost empty, and I can take this.

Comment 29

[Simon Montagu :smontagu]

(In reply to comment #28)
> Simon, it's hard for me to tell whether you've got things under control here,
> or whether you need help. 

I thought "I'm flailing around in circles" was a clear cry for help :)
I'm beginning to think that we can't fix this without fixing bug 383026, which would be quite a scary change at this point.

Comment 30

[Simon Montagu :smontagu]

... or rather, the equivalent fix in nsIScrollableFrame::Get/SetScrollPosition.

Comment 31

[Simon Montagu :smontagu]

Attachment: Layout patch w-i-p, v.4

I think this is a better approach, but it's still not quite right.

Comment 32

[Simon Montagu :smontagu]

Attachment: Layout patch w-i-p, v.5

This fixes some issues in the previous version. It doesn't fix bug 610685. There is also a bug with the autocomplete drop-down in RTL: when it has a vertical scrollbar, the content is shifted too far to the right.

There's also still a fundamental problem with this: it solves the issue that I described in comment 27 for the RTL tests in attachment 504099 [details], but it breaks RTL marquees, which currently depend on that behaviour. Reverting bug 407016, as I tried with an earlier version, makes marquee reftests work but causes a whole lot of assertions in crashtests, e.g. in layout/base/crashtests/265027-1.html, 88 instances of
ASSERTION: computed width should always be computed: 'computedSize.width != NS_INTRINSICSIZE', file ../../../../../layout/xul/base/src/nsBoxFrame.cpp, line 
690
and 3 of
ASSERTION: The computed inner margin is auto?: 'NS_AUTOMARGIN != aInnerMargin.right', file ../../../layout/tables/nsTableOuterFrame.cpp, line 813

Comment 33

[Simon Montagu :smontagu]

Comment on attachment 503635 [details] [diff] [review]
Remove workarounds in browser

The last layout patch works best with this browser patch

Comment 34

[(no longer active)]

(In reply to comment #32)
> Created attachment 508514 [details] [diff] [review]
> Layout patch w-i-p, v.5
> 
> This fixes some issues in the previous version. It doesn't fix bug 610685.

Do you know why?  I mean, the only reason that this is a blocker is because of bug 610685.  If this patch doesn't fix it, it's obviously not enough.  :(

> There is also a bug with the autocomplete drop-down in RTL: when it has a
> vertical scrollbar, the content is shifted too far to the right.
> 
> There's also still a fundamental problem with this: it solves the issue that I
> described in comment 27 for the RTL tests in attachment 504099 [details], but it breaks
> RTL marquees, which currently depend on that behaviour.

Well, I think we should fix marquees to no longer depend on that behavior, then.

> Reverting bug 407016,
> as I tried with an earlier version, makes marquee reftests work but causes a
> whole lot of assertions in crashtests, e.g. in
> layout/base/crashtests/265027-1.html, 88 instances of
> ASSERTION: computed width should always be computed: 'computedSize.width !=
> NS_INTRINSICSIZE', file ../../../../../layout/xul/base/src/nsBoxFrame.cpp, line 
> 690
> and 3 of
> ASSERTION: The computed inner margin is auto?: 'NS_AUTOMARGIN !=
> aInnerMargin.right', file ../../../layout/tables/nsTableOuterFrame.cpp, line
> 813

This sounds like we're reading stuff before reflowing somewhere...

I'll take a closer look at this tomorrow.  I'm assigning it to myself.  Please dump any other information which could help me down in this bug as well.  Thanks!

Comment 35

[(no longer active)]

Comment on attachment 506351 [details] [diff] [review]
Layout patch v.3

Simon, this is obsolete now, right?

Comment 36

[(no longer active)]

Comment on attachment 508514 [details] [diff] [review]
Layout patch w-i-p, v.5

> void
> nsXULScrollFrame::LayoutScrollArea(nsBoxLayoutState& aState,
>                                    const nsPoint& aScrollPosition)
> {
>   PRUint32 oldflags = aState.LayoutFlags();
>-  nsRect childRect = nsRect(mInner.mScrollPort.TopLeft() - aScrollPosition,
>+  nsRect childRect = nsRect(nsPoint(0, 0),
>                             mInner.mScrollPort.Size());

Why is this change OK?  Seems like with the changes here, aScrollPosition only has an effect in LTR mode, which seems wrong to me...

>   PRInt32 flags = NS_FRAME_NO_MOVE_VIEW;
> 
>   nsRect originalRect = mInner.mScrolledFrame->GetRect();
>   nsRect originalVisOverflow = mInner.mScrolledFrame->GetVisualOverflowRect();
> 
>   nsSize minSize = mInner.mScrolledFrame->GetMinSize(aState);
>   
>   if (minSize.height > childRect.height)
>     childRect.height = minSize.height;
>   
>   if (minSize.width > childRect.width)
>     childRect.width = minSize.width;
> 
>+  if (childRect.width > mInner.mScrollPort.width ||
>+      childRect.height > mInner.mScrollPort.height)
>+    childRect = mInner.GetScrolledRectInternal(childRect, 
>+                                               mInner.mScrollPort.Size());
>+
>   aState.SetLayoutFlags(flags);
>   mInner.mScrolledFrame->SetBounds(aState, childRect);
>   mInner.mScrolledFrame->Layout(aState);
> 
>   childRect = mInner.mScrolledFrame->GetRect();
> 
>   if (childRect.width < mInner.mScrollPort.width ||
>       childRect.height < mInner.mScrollPort.height)
>@@ -2658,16 +2661,28 @@ nsXULScrollFrame::LayoutScrollArea(nsBox
>     childRect.height = NS_MAX(childRect.height, mInner.mScrollPort.height);
> 
>     // remove overflow areas when we update the bounds,
>     // because we've already accounted for it
>     // REVIEW: Have we accounted for both?
>     mInner.mScrolledFrame->SetBounds(aState, childRect, PR_TRUE);
>   }
> 
>+  if (mInner.IsLTR())
>+    childRect.x = mInner.mScrollPort.x - aScrollPosition.x;
>+  else {
>+    childRect.x = mInner.mScrollPort.XMost() - childRect.width;
>+    if (!originalRect.IsEmpty()) {
>+      nscoord rtlScrollPos = mInner.mScrollPort.XMost() - originalRect.XMost();
>+      childRect.x -= rtlScrollPos;
>+    }
>+  }
>+  childRect.y = mInner.mScrollPort.y;
>+  mInner.mScrolledFrame->SetBounds(aState, childRect);
>+
>   nsRect finalRect = mInner.mScrolledFrame->GetRect();
>   nsRect finalVisOverflow = mInner.mScrolledFrame->GetVisualOverflowRect();
>   // The position of the scrolled frame shouldn't change, but if it does or
>   // the position of the overflow rect changes just invalidate both the old
>   // and new overflow rect.
>   if (originalRect.TopLeft() != finalRect.TopLeft() ||
>       originalVisOverflow.TopLeft() != finalVisOverflow.TopLeft())
>   {
>@@ -2870,17 +2885,18 @@ nsXULScrollFrame::Layout(nsBoxLayoutStat
>   if (styles.mHorizontal != NS_STYLE_OVERFLOW_SCROLL)
>   {
>     // These are only good until the call to LayoutScrollArea.
>     nsRect scrolledRect = mInner.GetScrolledRect();
>     nsSize scrolledContentSize(scrolledRect.XMost(), scrolledRect.YMost());
> 
>     // if the child is wider that the scroll area
>     // and we don't have a scrollbar add one.
>-    if (scrolledContentSize.width > mInner.mScrollPort.width
>+    if ((scrolledContentSize.width > mInner.mScrollPort.width ||
>+         scrolledRect.x < 0)

In what cases scrolledRect.x might be negative?  And why do we want to show a scrollbar in that case?

>         && styles.mHorizontal == NS_STYLE_OVERFLOW_AUTO) {
> 
>       if (!mInner.mHasHorizontalScrollbar) {
>            // no scrollbar? 
>           if (AddHorizontalScrollbar(aState, scrollbarBottom))
>              needsLayout = PR_TRUE;
> 
>            // if we added a horizontal scrollbar and we did not have a vertical


I'm also interested in dbaron's feedback here.

Comment 37

[Simon Montagu :smontagu]

(In reply to comment #34)
> > This fixes some issues in the previous version. It doesn't fix bug 610685.
> 
> Do you know why?  I mean, the only reason that this is a blocker is because of
> bug 610685.  If this patch doesn't fix it, it's obviously not enough.  :(

Obviously, but I don't know why :(  I thought that attachment 504099 [details] was supposed to emulate the tab-bar with a pinned tab, so that if one worked, the other would, but there must be some reason why that isn't true.

(In reply to comment #35)
> Comment on attachment 506351 [details] [diff] [review]
> Layout patch v.3
> 
> Simon, this is obsolete now, right?

I left it unobsoleted because we might yet go that route, or something similar, with the changes to marquee if we can fix the assertions.

(In reply to comment #36)
> > void
> > nsXULScrollFrame::LayoutScrollArea(nsBoxLayoutState& aState,
> >                                    const nsPoint& aScrollPosition)
> > {
> >   PRUint32 oldflags = aState.LayoutFlags();
> >-  nsRect childRect = nsRect(mInner.mScrollPort.TopLeft() - aScrollPosition,
> >+  nsRect childRect = nsRect(nsPoint(0, 0),
> >                             mInner.mScrollPort.Size());
> 
> Why is this change OK?  Seems like with the changes here, aScrollPosition only
> has an effect in LTR mode, which seems wrong to me...

rtlScrollPos below is intended as a replacement for aScrollPosition.x for the RTL case (aScrollPosition.y is used in both directions). 

> >     // if the child is wider that the scroll area
> >     // and we don't have a scrollbar add one.
> >-    if (scrolledContentSize.width > mInner.mScrollPort.width
> >+    if ((scrolledContentSize.width > mInner.mScrollPort.width ||
> >+         scrolledRect.x < 0)
> 
> In what cases scrolledRect.x might be negative?  And why do we want to show a
> scrollbar in that case?

Yes, this change is wrong, it should be
-    if (scrolledContentSize.width > mInner.mScrollPort.width
+    if ((scrolledContentRect.XMost() > mInner.mScrollPort.width ||
+         scrolledContentRect.x < 0)

Comment 38

[Simon Montagu :smontagu]

wrt the marquee assertions, I've been looking through http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/old/[e-mail]-8c888d960b6d/try-lnx-dbg/tryserver_fedora-debug_test-crashtest-build680.txt.gz again, and it seems that at least some of them are covered by bug 363722. That has a w-i-p patch, but it was made WORKSFORME by bug 407016, so it clearly needs to be fixed if we are reverting 407016.

That might be a more productive direction to work on, if we're sure that the change at http://hg.mozilla.org/try/rev/c78941400549#l1.95 is the right thing to do.

The full set of patches for that tryserver push are:
http://hg.mozilla.org/try/rev/c78941400549 (layout)
http://hg.mozilla.org/try/rev/6ecb256cc311 (browser)
http://hg.mozilla.org/try/rev/8c888d960b6d (marquee)

Comment 39

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

Comment on attachment 508514 [details] [diff] [review]
Layout patch w-i-p, v.5

>@@ -2870,17 +2885,18 @@ nsXULScrollFrame::Layout(nsBoxLayoutStat
>   if (styles.mHorizontal != NS_STYLE_OVERFLOW_SCROLL)
>   {
>     // These are only good until the call to LayoutScrollArea.
>     nsRect scrolledRect = mInner.GetScrolledRect();
>     nsSize scrolledContentSize(scrolledRect.XMost(), scrolledRect.YMost());
> 
>     // if the child is wider that the scroll area
>     // and we don't have a scrollbar add one.
>-    if (scrolledContentSize.width > mInner.mScrollPort.width
>+    if ((scrolledContentSize.width > mInner.mScrollPort.width ||
>+         scrolledRect.x < 0)
>         && styles.mHorizontal == NS_STYLE_OVERFLOW_AUTO) {

I don't like the added check here.

However, there's also something I don't like about the existing code:  the way we set scrolledContentSize seems incorrect for RTL, where GetScrolledRect can return something that extends to the right.  Would replacing the initialization of scrolledContentSize with:

  nsSize scrolledContentSize(scrolledRect.Size());

also fix what this change is fixing?


I still need to understand the LayoutScrollArea changes.

Comment 40

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

Comment on attachment 508514 [details] [diff] [review]
Layout patch w-i-p, v.5

> void
> nsXULScrollFrame::LayoutScrollArea(nsBoxLayoutState& aState,
>                                    const nsPoint& aScrollPosition)
> {
>   PRUint32 oldflags = aState.LayoutFlags();
>-  nsRect childRect = nsRect(mInner.mScrollPort.TopLeft() - aScrollPosition,
>+  nsRect childRect = nsRect(nsPoint(0, 0),
>                             mInner.mScrollPort.Size());

I don't like this change either.  Ignoring the origin of mInner.mScrollPort means that childRect does not exclude a scrollbar placed on the left side (whereas it does exclude a scrollbar placed on the right side).

However, I wonder whether mInner.mScrollPort gets updated correctly before we call nsXULScrollFrame::LayoutScrollArea.  I don't see anything that would do that, although I could be missing something.  The various "try this configuration" possibilities in nsXULScrollFrame::Layout are essentially trying different possibilities for scrollbar configuration and therefore different possibilities for the size and position of mInner.mScrollPort.  *If* mInner.mScrollPort doesn't get updated, then that information probably needs to be passed to LayoutScrollArea.

Comment 41

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

The other changes to LayoutScrollArea in v5 don't make much sense to me either.

Something like the second part of the LayoutScrollArea changes in v3 might be necessary, though, although I suspect we might want to realign the scrolled frame's kids rather than the scrolled frame itself.  (Though I'm really not sure under what conditions a frame's Layout method would cause it to shrink, and when that happens I'm not sure what positioning changes we should make to its kids when we expand it again.)

Comment 42

[(no longer active)]

OK, I'm going to post some patches here which should be applied on top of Simon's patches.  I'll mark them as EN, where "E" stands for "Ehsan" and "N" is the sequence number in which these patches should be applied.

Comment 43

[(no longer active)]

Attachment: Part E1: Mark a crashtest as asserting like bug 611532

With these patches, a crashtest asserts.  This assertion is bug 611532, and will be fixed when that bug lands.  We should just annotate it for now, and later back it out when that bug is fixed.

Comment 44

[(no longer active)]

Attachment: Part E2: Adjust the known assertion count based on the new marquee implementation

The assertions we hit when loading layout/generic/crashtests/265867-1.html are all known (bug 575011).  With these patches, we'll get fewer of those.  This patch adjusts the assertion count.

With part E1 and E2, the crashtest should pass across the board.

Comment 45

[(no longer active)]

(In reply to comment #40)
> Comment on attachment 508514 [details] [diff] [review]
> Layout patch w-i-p, v.5
> 
> > void
> > nsXULScrollFrame::LayoutScrollArea(nsBoxLayoutState& aState,
> >                                    const nsPoint& aScrollPosition)
> > {
> >   PRUint32 oldflags = aState.LayoutFlags();
> >-  nsRect childRect = nsRect(mInner.mScrollPort.TopLeft() - aScrollPosition,
> >+  nsRect childRect = nsRect(nsPoint(0, 0),
> >                             mInner.mScrollPort.Size());
> 
> I don't like this change either.  Ignoring the origin of mInner.mScrollPort
> means that childRect does not exclude a scrollbar placed on the left side
> (whereas it does exclude a scrollbar placed on the right side).

And this is what breaks a lot of tests in the tree because scrollTop/scrollLeft are not calculated correctly any more, etc.

> However, I wonder whether mInner.mScrollPort gets updated correctly before we
> call nsXULScrollFrame::LayoutScrollArea.  I don't see anything that would do
> that, although I could be missing something.  The various "try this
> configuration" possibilities in nsXULScrollFrame::Layout are essentially trying
> different possibilities for scrollbar configuration and therefore different
> possibilities for the size and position of mInner.mScrollPort.  *If*
> mInner.mScrollPort doesn't get updated, then that information probably needs to
> be passed to LayoutScrollArea.

mScrollPort is updated at the end of TryLayout.  Is that good enough?

Comment 46

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

TryLayout is a method on nsHTMLScrollFrame; this is nsXULScrollFrame.

Comment 47

[(no longer active)]

(In reply to comment #46)
> TryLayout is a method on nsHTMLScrollFrame; this is nsXULScrollFrame.

Ah, right, yes.

I also don't see mScrollPort info being updated anywhere...  But how come this has worked all the time?

Comment 48

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

I think it's actually sort of ok; it's updated by the six-param overload of AddRemoveScrollbar, whose second and third parameters are nscoord& (not just nscoord).  (That's called from the 4-param version of AddRemoveScrollbar, which is called from (Add/Remove)(Horizontal/Vertical)Scrollbar, which are called from Layout.

By "sort of ok", I mean the fundamental problem I was worried about isn't there.  However, the code in question isn't rtl-safe; AddRemoveScrollbar should be manipulating the scrollport differently when adding or removing a scrollbar on the left side.

Comment 49

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

(In reply to comment #48)
> By "sort of ok", I mean the fundamental problem I was worried about isn't
> there.  However, the code in question isn't rtl-safe; AddRemoveScrollbar should
> be manipulating the scrollport differently when adding or removing a scrollbar
> on the left side.

Actually, never mind that.  The code looks like it does try to handle scrollbars on the left correctly (except that a bunch of the parameters look like they're named backwards).  I didn't review it really closely, though.

Comment 50

[(no longer active)]

(In reply to comment #37)
> (In reply to comment #36)
> > > void
> > > nsXULScrollFrame::LayoutScrollArea(nsBoxLayoutState& aState,
> > >                                    const nsPoint& aScrollPosition)
> > > {
> > >   PRUint32 oldflags = aState.LayoutFlags();
> > >-  nsRect childRect = nsRect(mInner.mScrollPort.TopLeft() - aScrollPosition,
> > >+  nsRect childRect = nsRect(nsPoint(0, 0),
> > >                             mInner.mScrollPort.Size());
> > 
> > Why is this change OK?  Seems like with the changes here, aScrollPosition only
> > has an effect in LTR mode, which seems wrong to me...
> 
> rtlScrollPos below is intended as a replacement for aScrollPosition.x for the
> RTL case (aScrollPosition.y is used in both directions). 

Well, aScrollPosition.y was left out by mistake.  This is why scrollTop was broken for the XUL scrollboxes we have in our test suite.  This patch seems to fix this issue:  http://pastebin.mozilla.org/1012168.  I'll submit a new patch for dbaron to review based on your patch in http://hg.mozilla.org/try/rev/dee63044c685 and this one.

Comment 51

[(no longer active)]

Attachment: Layout patch v.6

Comment 52

[(no longer active)]

Pushed all of the patches to try:

http://tbpl.mozilla.org/?tree=MozillaTry&rev=60f310ef93f7

Comment 53

[(no longer active)]

Asa, this patch is not the only patch needed for this bug, there is still more work to be done here.

Comment 54

[Asa Dotzler [:asa]]

sorry 'bout that Ehsan. I removed the status update.

Comment 55

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

So I think there are fundamentally three changes in the layout patch, although I'm not sure I understand the third:

 (1) change GetScrolledRectInternal to not special-case XUL, and instead allow scrolling to the left in RTL regardless of HTML vs. XUL.  This part I've been happy with; it's represented by the patch at http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/c701a507bcf4/xul-scroll-frame-scroll-left

 (2) fix nsXULScrollFrame::Layout to use the correct in cases where GetScrolledRect is not doing top-left clamping.  I'd prefer this fixed as I described in comment 39 rather than the way it was done in the patches in this bug, but I think they should have equivalent results.  The way I suggested in comment 39 comes out to this (untested) patch: http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/c701a507bcf4/xul-scroll-frame-scrolled-content-size

 (3) fix nsXULScrollFrame::LayoutScrollArea to preserve scroll position relative to the right edge rather than relative to the left edge in cases when the "origin" is the top right rather than the top left.  At least, I think that's what's needed based on reading the code, and I think the various patches in this bug accomplish it in various roundabout and probably rather buggy ways.  But I think what should happen here is that we should try and do that in as direct a manner as possible.  In particular, I think in RTL we want to keep constant the vector from the top-right corner of the frame to the top-right corner of the scrollport (though *maybe* we want the second value to be the top-right corner of the scrollframe; it only makes a difference for the RTL-with-scrollbars-on-left case, and I'm really not sure what should happen there when scrollbars appear and disappear -- neither possibility seems particularly nice in terms of behavior since one way involves a visual jump and the other way involves a logical jump).

I'm reasonably confident that (3) needs to be done.  I'm not so confident that doing it would be sufficient, since I still don't understand what the various changes (across the different patches, in which they're the part that varies the most) to LayoutScrollArea are trying to do.

Comment 56

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

(In reply to comment #55)
> top-right corner of the scrollframe; it only makes a difference for the
> RTL-with-scrollbars-on-left case, and I'm really not sure what should happen

sorry, I should have said "RTL-with-scrollbar-on-*right* case"

Comment 57

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

And I'm still trying to figure out how nsHTMLScrollFrame handles #3; I haven't found any handling yet, even where I'd have expected to find it.  (nsHTMLScrollFrame::Reflow and its calls to mInner.GetScrollPosition() and PlaceScrollArea()).

Comment 58

[(no longer active)]

(In reply to comment #55)
> So I think there are fundamentally three changes in the layout patch, although
> I'm not sure I understand the third:
> 
>  (1) change GetScrolledRectInternal to not special-case XUL, and instead allow
> scrolling to the left in RTL regardless of HTML vs. XUL.  This part I've been
> happy with; it's represented by the patch at
> http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/c701a507bcf4/xul-scroll-frame-scroll-left

That is completely correct.

>  (2) fix nsXULScrollFrame::Layout to use the correct in cases where
> GetScrolledRect is not doing top-left clamping.  I'd prefer this fixed as I
> described in comment 39 rather than the way it was done in the patches in this
> bug, but I think they should have equivalent results.  The way I suggested in
> comment 39 comes out to this (untested) patch:
> http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/c701a507bcf4/xul-scroll-frame-scrolled-content-size

Makes sense to me, and I _think_ Simon had the same thing in mind, although he can probably confirm when he wakes up.  I *think* that your changes should be fine; I'll be sure when I finish the reftests that I'm writing for this bug right now.

>  (3) fix nsXULScrollFrame::LayoutScrollArea to preserve scroll position
> relative to the right edge rather than relative to the left edge in cases when
> the "origin" is the top right rather than the top left.

Yes, this what I think too.

>  At least, I think
> that's what's needed based on reading the code, and I think the various patches
> in this bug accomplish it in various roundabout and probably rather buggy ways.

This may be possible to be accomplished in cleaner ways.  I need to think about it more.

>  But I think what should happen here is that we should try and do that in as
> direct a manner as possible.  In particular, I think in RTL we want to keep
> constant the vector from the top-right corner of the frame to the top-right
> corner of the scrollport (though *maybe* we want the second value to be the
> top-right corner of the scrollframe; it only makes a difference for the
> RTL-with-scrollbars-on-left case, and I'm really not sure what should happen
> there when scrollbars appear and disappear -- neither possibility seems
> particularly nice in terms of behavior since one way involves a visual jump and
> the other way involves a logical jump).

So, this makes _some_ sense to me.  However, I'm still not convinced that we need to do this, not at least we know how to address the RTL-with-scrollbars-on-the-right case.

> I'm reasonably confident that (3) needs to be done.  I'm not so confident that
> doing it would be sufficient, since I still don't understand what the various
> changes (across the different patches, in which they're the part that varies
> the most) to LayoutScrollArea are trying to do.

I really think Simon should weigh in here too.  (With regards to comment 57 too).  I confess that I still don't know this code that well, as I have spent most of today fixing the rough edges of the patches (read: test failures)...

Comment 59

[(no longer active)]

Attachment: Part E3: XUL and HTML reftests

These reftests incorporate the testcases attached to this bug as reftests.  The tests fail on a non-patched build, and pass on a patched build.

Comment 60

[(no longer active)]

Attachment: Part E1: Mark a crashtest as asserting like bug 611532

I had specified the wrong assertion count; meant to specify two...

Comment 61

[Simon Montagu :smontagu]

(In reply to comment #39)
> Would replacing the initialization
> of scrolledContentSize with:
> 
>   nsSize scrolledContentSize(scrolledRect.Size());
> 
> also fix what this change is fixing?

I agree that this would be a better way to do it, but in that case why do we even need scrolledContentSize? Couldn't we just use scrolledRect.width and .height directly?

Comment 62

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

(In reply to comment #61)
> (In reply to comment #39)
> > Would replacing the initialization
> > of scrolledContentSize with:
> > 
> >   nsSize scrolledContentSize(scrolledRect.Size());
> > 
> > also fix what this change is fixing?
> 
> I agree that this would be a better way to do it, but in that case why do we
> even need scrolledContentSize? Couldn't we just use scrolledRect.width and
> .height directly?

We don't; we could just used scrolledRect.width and scrolledRect.height.

Comment 63

[Simon Montagu :smontagu]

Attachment: Layout patch v.7, addressing dbaron's comments

Comment 64

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

Comment on attachment 509102 [details] [diff] [review]
Layout patch v.7, addressing dbaron's comments

>+  if (GetStateBits() & NS_FRAME_FIRST_REFLOW) {
>+    // only check minSize on initial reflow

I still think this is wrong, and it's likely to break cases of dynamic changes to the content inside the scrollbars (and even cases where it doesn't change much, but the minSize matters).

>@@ -2832,20 +2862,19 @@ nsXULScrollFrame::Layout(nsBoxLayoutStat
>   
>   // now look at the content area and see if we need scrollbars or not
>   PRBool needsLayout = PR_FALSE;
> 
>   // if we have 'auto' scrollbars look at the vertical case
>   if (styles.mVertical != NS_STYLE_OVERFLOW_SCROLL) {
>     // These are only good until the call to LayoutScrollArea.
>     nsRect scrolledRect = mInner.GetScrolledRect();
>-    nsSize scrolledContentSize(scrolledRect.XMost(), scrolledRect.YMost());
> 
>     // There are two cases to consider
>-      if (scrolledContentSize.height <= mInner.mScrollPort.height
>+      if (scrolledRect.height <= mInner.mScrollPort.height

Could you also fix the parallel height case just below?



More later, once I try to understand some of the other changes...

Comment 65

[Simon Montagu :smontagu]

Comment on attachment 509102 [details] [diff] [review]
Layout patch v.7, addressing dbaron's comments

Apologies for the patch churn and bug spam -- this fails some reftests.

Comment 66

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

That said, a few other thoughts about preserving scroll position (item #3 in comment 55):

 - I don't think it can be done correctly only in LayoutScrollArea; it has to be saved in Layout (which does save the scroll position, but top-left relative), since Layout does things that can change the scroll port before calling LayoutScrollArea.  I think that saved scroll position in layout should be a logical scroll position.

 - It does require a first-reflow check (probably on the scrolled frame?) when initializing that logical scroll position, since we want to initialize to logical (0,0) even when the physical (0,0) default frame position is not logical (0,0).

Comment 67

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

(In reply to comment #66)
>  - I don't think it can be done correctly only in LayoutScrollArea; it has to
> be saved in Layout (which does save the scroll position, but top-left
> relative), since Layout does things that can change the scroll port before
> calling LayoutScrollArea.  I think that saved scroll position in layout should
> be a logical scroll position.

Though I suppose this difference isn't that horrible; it just means that if a vertical scrollbar disappears or appears, you'll end up with the horizontal position off by the width of the vertical scrollbar (i.e., keeping the left edge constant instead of the right edge constant).  That it itself isn't that horrible... although it might also lead to a state where the scrollbar and the scrolling state are inconsistent with each other.

Comment 68

[Simon Montagu :smontagu]

(In reply to comment #66)
> That said, a few other thoughts about preserving scroll position (item #3 in
> comment 55):
> 
>  - I don't think it can be done correctly only in LayoutScrollArea; it has to
> be saved in Layout (which does save the scroll position, but top-left
> relative), since Layout does things that can change the scroll port before
> calling LayoutScrollArea.  I think that saved scroll position in layout should
> be a logical scroll position.

The latest version attempts to do exactly that, in https://bugzilla.mozilla.org/attachment.cgi?id=509102&action=diff#a/layout/generic/nsGfxScrollFrame.cpp_sec7

(In reply to comment #64)
> >+  if (GetStateBits() & NS_FRAME_FIRST_REFLOW) {
> >+    // only check minSize on initial reflow
> 
> I still think this is wrong, and it's likely to break cases of dynamic changes
> to the content inside the scrollbars (and even cases where it doesn't change
> much, but the minSize matters).

Can you suggest an alternative way to handle the problem described in comment 27?

> >     // There are two cases to consider
> >-      if (scrolledContentSize.height <= mInner.mScrollPort.height
> >+      if (scrolledRect.height <= mInner.mScrollPort.height
> 
> Could you also fix the parallel height case just below?

Sorry, I don't understand what this refers to.

Comment 69

[Simon Montagu :smontagu]

Comment on attachment 509102 [details] [diff] [review]
Layout patch v.7, addressing dbaron's comments

Very odd: the reftest 508816-1.xul fails with this on my Mac box, and 508816-2.html fails on my Linux box.

Comment 70

[Simon Montagu :smontagu]

I'm not 100% sure, but the minSize issue may be the same as described in bug 433621. Does bug 433621 comment 6 mean that we could fix bug 610685 without the minSize change in this bug, by tweaking the scrollbar css?

Comment 71

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

(In reply to comment #68)
> (In reply to comment #66)
> > That said, a few other thoughts about preserving scroll position (item #3 in
> > comment 55):
> > 
> >  - I don't think it can be done correctly only in LayoutScrollArea; it has to
> > be saved in Layout (which does save the scroll position, but top-left
> > relative), since Layout does things that can change the scroll port before
> > calling LayoutScrollArea.  I think that saved scroll position in layout should
> > be a logical scroll position.
> 
> The latest version attempts to do exactly that, in
> https://bugzilla.mozilla.org/attachment.cgi?id=509102&action=diff#a/layout/generic/nsGfxScrollFrame.cpp_sec7

Oops, I was clearly reading too fast.

> > >     // There are two cases to consider
> > >-      if (scrolledContentSize.height <= mInner.mScrollPort.height
> > >+      if (scrolledRect.height <= mInner.mScrollPort.height
> > 
> > Could you also fix the parallel height case just below?
> 
> Sorry, I don't understand what this refers to.

There's a second variable called |scrolledContentSize| in the vertical-scrollbar case a little below that should have the equivalent changes made to it.

Comment 72

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

Comment on attachment 509102 [details] [diff] [review]
Layout patch v.7, addressing dbaron's comments

>+  if (!mInner.IsLTR()) {
>+    // In the RTL case, the scrolled position is the offset of the right
>+    // edge of the scrolled frame from the right edge of the scroll port
>+    nsRect scrolledRect = mInner.mScrolledFrame->GetRect();
>+    if (scrolledRect.IsEmpty())
>+      oldScrollPosition.x = 0;
>+    else
>+      oldScrollPosition.x = mInner.mScrollPort.XMost() - scrolledRect.XMost();
>+  }

In this case, I think it's actually better to check (mInner.mScrolledFrame->GetStateBits() & NS_FRAME_FIRST_REFLOW) rather than scrolledRect.IsEmpty().


(In reply to comment #27)
> In the box layout case, the call to GetMinSize forces the scrolled frame to
> expand to the size of the child element. Later on, when placing the
> fixed-position child in nsHTMLReflowState::InitAbsoluteConstraints, this size
> is returned by GetHypotheticalBoxContainer, in cbWidth, and this pushes the
> fixed-position child too far to the right by the extra size.

So maybe that's just how box layout works?  Do you get the same results in the LTR case?  Why do we need to change that for this bug?  (Isn't it just an artifact of the testcase?)

Comment 73

[Simon Montagu :smontagu]

Attachment: Layout patch v.8

Changes since earlier versions:

Removed the hack with only doing GetMinSize on first reflow, and changed GetPositionOfChildIgnoringScrolling to get the logical position, as we discussed on IRC.

When setting oldScrollPosition in Layout, I don't think either the first reflow check or the rect.IsEmpty() check are necessary, since if we set it this early (before mInner.mScrollPort is first set to clientRect), the scrolled frame and the scroll port are both empty anyway on first reflow, at least in the cases I've been testing.

When fixing the scrolled position after layout, I use RoundAppUnitsToNearestDevPixels, which fixes the assertion mentioned in comment 25. I'll be adding the testcase from there as a crashtest.

Plus a new change in SaveState and ScrollToRestoredPosition to fix a bug with restoring scrolled position on reload. SaveState now saves the scroll position even when it's (0,0). That assumes that saving (0,0) in LTR doesn't actually do any harm and this was just an optimization -- is that assumption correct?

Comment 74

[Simon Montagu :smontagu]

Attachment: Ehsan's tests, slightly tweaked to prevent the failure I was seeing on Linux

Comment 75

[Simon Montagu :smontagu]

Attachment: Still more tests

Crashtest for the "curPos.x not a multiple of device pixels" assertion, and dbaron's tests from attachment 212448 [details] as != reftests.

Comment 76

[Simon Montagu :smontagu]

(In reply to comment #73)

> Plus a new change in SaveState and ScrollToRestoredPosition to fix a bug with
> restoring scrolled position on reload. SaveState now saves the scroll position
> even when it's (0,0). That assumes that saving (0,0) in LTR doesn't actually do
> any harm and this was just an optimization -- is that assumption correct?

On reflection, I'm not quite sure about the change in ScrollToRestoredPosition -- it seems it would make more sense to make mLastPos save the logical position as well, but I'm not sure how to test this.

Comment 77

[Simon Montagu :smontagu]

Attachment: Layout patch v.8a

Changed mLastPos to always be the logical position as well.

Comment 78

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

Comment on attachment 509477 [details] [diff] [review]
Layout patch v.8a

I think you should add a method to nsGfxScrollFrameInner called GetLogicalScrollPosition that encapsulates the logic that you repeat in a whole bunch of places.  Then you can just add the word "Logical" in GetPositionOfChildIgnoringScrolling and you can also use it in ScrollToRestoredPosition and nsXULScrollFrame::Layout

I think you also need to fix nsGfxScrollFrameInner::RestoreState to set mLastPos differently (using the logical scroll position).

It would probably be better to make mRestorePos also be a logical scroll position, though if you don't want to deal with changing that, that's ok.  If you do that, you probably don't need to remove the "Don't save scroll position if we are at (0,0)" code.

Otherwise, I think this looks good, though I want to look a few things over a little more carefully.

Comment 79

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

Comment on attachment 509477 [details] [diff] [review]
Layout patch v.8a

Yeah, r=dbaron with comment 78 addressed; I'd sort of like to look at the revised patch, though.

Comment 80

[Simon Montagu :smontagu]

Attachment: Remove workarounds in browser, v.4

With the current version of the layout patch, we only need to remove the existing workarounds.

The last hunk of the patch is a fix for a bug that I came across while testing: when scrolling the tab bar in RTL with the mouse wheel, scrolling up and down are reversed. Apparently we have always had this bug, though I can't find any report of it. Strictly speaking it's out of scope here, but it would be good to fix it anyway.

Comment 81

[Simon Montagu :smontagu]

Attachment: Layout patch v.9

Addressed comment 78

(In reply to comment #78)
> It would probably be better to make mRestorePos also be a logical scroll
> position, though if you don't want to deal with changing that, that's ok.

That would require special-casing RTL all through ScrollTo and its descendants, wouldn't it? I really think we don't want to mess with that at this point.

Comment 82

[Simon Montagu :smontagu]

Attachment: Tightened up the reftests

Comment 83

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

(In reply to comment #81)
> That would require special-casing RTL all through ScrollTo and its descendants,
> wouldn't it? I really think we don't want to mess with that at this point.

I don't think it would require that; you just couldn't pass mRestorePos straight to ScrollTo in ScrollToRestoredPosition.

Comment 84

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

Comment on attachment 510099 [details] [diff] [review]
Layout patch v.9

r=dbaron

Comment 85

[(no longer active)]

So, is everything ready to go here?

Comment 86

[Simon Montagu :smontagu]

http://hg.mozilla.org/mozilla-central/rev/18a59f6f099b
http://hg.mozilla.org/mozilla-central/rev/2609e7fdf1bf
http://hg.mozilla.org/mozilla-central/rev/fffaf649c714
http://hg.mozilla.org/mozilla-central/rev/4c62984f12d1