331607 - scroll position can not be retained in overflowing RTL blocks

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Hideo Saito]

Horizontal scroll position can not be retained in overflowing scrollable RTL blocks.

steps are:
1. load a following page
 http://mozilla.org.il/board/viewtopic.php?t=53
2-1. move only horizontal scrollbar to the left side
2-2. move scrollbar to the left side and down vertical scrollbar
3. reload the page

actual result for case 2-1:
horizontal scrollbar go back to the right side with content

actual result for case 2-2:
only horizontal scrollbar go back to the right side(the content does not go back)

Comment 1

[Hideo Saito]

Attachment: patch

Comment 2

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

Comment on attachment 216121 [details] [diff] [review]
patch

>-    if (y > cy || x > cx) {
>+    if (y > cy || (IsLTR() ? x > cx : x < cx)) {

Can't these just turn into != ?  See the comment in nsGfxScrollFrame.h for why:

  /**
   * GetScrolledRect is designed to encapsulate deciding which
   * directions of overflow should be reachable by scrolling and which
   * should not.  Callers should NOT depend on it having any particular
   * behavior (although nsXULScrollFrame currently does).

>-    SetCoordAttribute(mVScrollbarBox, nsXULAtoms::maxpos, maxY - minY);
>+    if (mHasVerticalScrollbar) {
>+      SetCoordAttribute(mVScrollbarBox, nsXULAtoms::maxpos, maxY - minY);
>+    }

Why are these changes needed or desired?

Comment 3

[Hideo Saito]

Attachment: patch

> Can't these just turn into != ?

This patch came from an attachment 213318 [details] [diff] [review] for Bug 192767.
I am sure "x != cx" is good since the code is simple.

> Why are these changes needed or desired?

Please refer to https://bugzilla.mozilla.org/show_bug.cgi?id=192767#c73.
The change for vertical scrollbar is not needed for this problem.

Comment 4

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

Comment on attachment 216153 [details] [diff] [review]
patch

You should change both the x and y checks to !=.

I don't think checking mHasHorizontalScrollbar is the right fix, and I don't understand what you're describing in bug 192767 comment 73:  what's the testcase?  what prevents the scroll position from moving to the left?  Is it that maxpos should be set first, before curpos?

Comment 5

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

And please do retain the symmetry between vertical and horizontal.

Comment 6

[Hideo Saito]

Attachment: testcase

Comment 7

[Hideo Saito]

Attachment: patch

When |scrollingView->ScrollTo(x, y, 0)| is called from nsGfxScrollFrameInner::ScrollToRestoredPosition called from PresShell::EndLoad, scrolledRect is not set right rect at ClampScrollValues called from nsScrollPortView::ScrollToImpl. 

Then, scrollable->GetScrollPosition(curPosX, curPosY) called from nsGfxScrollFrameInner::LayoutScrollbars can not get right values curPosX and curPosY.

I think that ScrollToRestoredPosition should be called previously.

Comment 8

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

Comment on attachment 216313 [details] [diff] [review]
patch

>+  ScrollToRestoredPosition();

While you're changing this code, please change this line to mInner.ScrollToRestoredPosition() to save a virtual function call.

Or I suppose I will, since I should check this in for you as well (after doing a bit more testing).

Comment 9

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

That said, the patch looks fine, but I can't tell what it's fixing -- I don't see any problems on the testcase.  What's the bug here?

Comment 10

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

Er, ok, I see the problem with 2-1., but not 2-2.

Comment 11

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

Checked in to trunk.

Comment 12

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

Thanks for the patch.

Comment 13

[Hideo Saito]

With 2-2, the problem is not solved yet. I am sorry, needs works.

Comment 14

[Uri Bernstein]

(In reply to comment #13)
> With 2-2, the problem is not solved yet. I am sorry, needs works.
> 

Like David (comment 10), I never could, and still can't, reproduce the problem with the 2-2 case (I'm on Mac).

Perhaps it would be better to open a separate bug for that case, which seems to be a separate (maybe platform-specific?) issue, instead of re-opening this one?

Comment 15

[Hideo Saito]

Yes, I will open a new bug since a problem of 2-1 was already fixed.

I can see a problem of 2-2 on Windows XP using nightly build and PowerPC/Linux using my build.

I can not see the problem in the case of |scrollingView->ScrollTo(x, y, 0)| of |nsGfxScrollFrameInner::ScrollToRestoredPosition| is called twice continuously from |PresShell::EndLoad| and |nsHTMLScrollFrame::Reflow|. For example, following code is able to avoid this problem although I don't know the reason yet. I think that each system differs as timing.

nsGfxScrollFrameInner::ScrollToRestoredPosition()
    if (y != cy || x != cx) {
      scrollingView->ScrollTo(x, y, 0);
      scrollingView->ScrollTo(x, y, 0); <-- This code is added.

Comment 16

[(no longer active)]

Mass-assigning the new rtl keyword to RTL-related (see bug 349193).