333829 - RTL horizontal scrollbar can not return to the left edge on reload

Status: RESOLVED WORKSFORME

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

Description

[Hideo Saito]

Horizontal scrollbar in RTL direction can not return to the left edge on reload(i.e. it goes back to the right edge) although the contents return to the left side. This problem came from Bug 331607.

steps are:
1. load the following page
 http://mozilla.org.il/board/viewtopic.php?t=53
2. move horizontal scrollbar to the left edge
3. reload the page

actual result:
Only horizontal scrollbar goes back to the initial position(right edge).

Comment 1

[Hideo Saito]

Attachment: patch

Please refer to |nsSliderFrame::CurrentPositionChanged| called from |nsGfxScrollFrameInner::SetCoordAttribute|.

    PRInt32 maxpos = GetMaxPosition(scrollbar);

    if (curpos < 0)
      curpos = 0;
    else if (curpos > maxpos)
      curpos = maxpos;

I think that |maxpos| needs to be set before |curpos|.

Comment 2

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

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

r+sr=dbaron given a comment explaining why it needs to be in this order

Comment 3

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

Checked in to trunk.  Thanks for debugging this and writing the patch.

Comment 4

[Hideo Saito]

Attachment: added patch

I am sorry, this patch has a regress.
If the window is resized, the contents does not change(resize reflow can not start). Please review this added patch. I think that checking for the ranges of |curPos| needs in order to start |ScrollbarChanged| called from |CurPosAttributeChanged|. If this added patch is wrong, please back base patch from trunk.

Comment 5

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

Comment on attachment 218592 [details] [diff] [review]
added patch

r+sr=dbaron.  Checked in to trunk, with comment added.  Thanks for fixing this as well.

Comment 6

[Peter van der Woude [:Peter6]]

it looks like this may have caused:
Bug 334262 and Bug 334266 (possibly dupes)

Comment 7

[Peter van der Woude [:Peter6]]

and no, it wasn't fixed by the additional checkin at 04/16 12:58:22 

Comment 8

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

Also, sorry for checking this in before you'd requested review -- I should have waited until you asked.

Comment 9

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

I've backed this out for now due to the regressions.  I don't have time to look into them; if you have time to do so, that would be great.

Comment 10

[Hideo Saito]

Attachment: patch

I can see this problem on current seamenkey. However I can't fix previous posted patch because I can't see the regressions on firefox. If possible, I wish a new patch is reviewed although this patch may have same regressions. I think that the correct value should be set to |mCurPos| for current position.

Comment 11

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

Sorry to take so long to get to this.

I don't see how this works, or at least how it will work in all cases.  The code right above the code you change permanently changes the curpos attribute based on minpos and maxpos.  Given that, how do you get the original curpos back?

Comment 12

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

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

Marking review- since I think this makes the issue of whether the curpos attribute and mCurPos are clamped to within minpos and maxpos even more confusing than it is already.

Comment 13

[Uri Bernstein]

The linked URL no longer has a horizontal scrollbar, and I can't create a testcase where I can see this problem (even in a 2006-04-13 build). Is anyone able to create a testcase for this?

Comment 14

[(no longer active)]

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

Comment 15

[Hideo Saito]

Attachment: patch

This problem is yet reproduced with the saved page on my disk, when the value of |maxpospx| is sometimes 0 at |nsSliderFrame::CurrentPositionChanged|, though the value is almost always 100. The meaning of the value of 100 is a default value at |nsSliderFrame::GetMaxPosition|. If the scroll position(that is pointed by |mCurPos|) is 100, this problem is always reproduced, because |mCurPos| is forced to set 100, in the result that the position is not changed at |nsSliderFrame::CurrentPositionChanged| since the value of |mCurPos| is already 100.

In the posted patch, following optimization will be ignored at |nsSliderFrame::CurrentPositionChanged| using new variable |mForcePositionChange|.

  // do nothing if the position did not change
  if (mCurPos == curpospx && !mForcePositionChange)
      return NS_OK;

For reproducing this problem, a complex page may be needed like as that the scroll position moves to the right side of the window first and returns to the current position when the page is reloaded.

Comment 16

[Hideo Saito]

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

David, could you review this patch?

I think that the value of |mCurPos| has two meanings, first is the current scroll position and second is the forced value by |maxpospx|, so that these cases need to be distinguished. This approach is to skip the optimization using a new boolean |mForcePositionChange| in the second case.

Comment 17

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

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

Sorry for taking so long to get to this.

I'd echo Uri's comment:  do you have a testcase where this bug is still present?  This patch seems like it introduces extra complexity, and I'd like to know that that complexity is needed.



If you do have a testcase, could you add the testcase as an automated test and re-request review?  This code has become very fragile, and we should really be adding automated tests for changes we make to it.

Comment 18

[(no longer active)]

The page in comment 0 no longer has a horizontal scrollbar.  I wrote a simple test case.  Comment 0 is WorksForMe with this test case.