226534 - [FIXr]Getting "Back" does not restore horizontal scroller position

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Max Alekseyev]

Linux build 2003102905

To reproduce:
1. Open URL
2. Scroll to the right and choose some picture
3. RMB click and select "View Iamge"
4. Click "Back" button
5. Horizontal scroller position is NOT restored

Expected result: Mozilla should restore horizontal scroller position like it
does for vertical scroller

Comment 1

[Boris Zbarsky [:bzbarsky]]

The problem here is the IBMBIDI code in
nsGfxScrollFrameInner::AddHorizontalScrollbar that unconditionally resets the
scroll position to 0 or 0x7FFFFFFF when the horizontal scrollbar is added.  This
is treated as a user scroll event and thus cancels session history restoration...

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 136135 [details] [diff] [review]
Proposed patch

roc, what do you think?  Also, is this 1.6 material?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Taking.

Comment 5

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

This code looks okay, but I wonder if this is really the right fix.

Do you think this would still work if we replaced the use of the "dir" attribute
here with simple getting and setting of a member variable of
nsGfxScrollFrameInner? I think it would work.

But I wonder if a better fix for this BIDI issue would be to whack nsSliderFrame
around here:
http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/nsSliderFrame.cpp#247
Logically, in RTL you could think of the current pos as being the offset from
the right hand side, not the left hand side. So we could modify the
nsSliderFrame code for RTL scrollbar maxpos changes to keep maxpos - curpos
constant. The only tricky part is how to stop *that* from being detected as a
user scroll event. Where is that detection code BTW?

Another thing that would be crazy but cool is if when curpos == maxpos and
maxpos is increasing and curpos > 0, automatically update curpos to equal the
new maxpos. Then when a page is loading and you scroll down to the bottom, as
more of the page loads, we'll keep scrolling down automatically for you. What do
you think? :-)

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Where is that detection code BTW?

In nsScrollBoxFrame.  Basically, any change in the scrolling view's position
that is NOT triggered by a session history restore is treated as a user scroll
event (makes sense).  See the uses of mLastPos in that file.

> Another thing that would be crazy but cool

Yeah, we have existing bugs on the fact that all of our treatment of scrolling
just kinda sucks like that (session history stuff, what happens when the window
is resized, etc, etc).  To be truthful, I just don't have the time or desire to
reengineer all this code.

Comment 7

[Oleg Sidletskiy]

similar bug: Bug 99094

Comment 8

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

Okay fair enough. But since we're trying to reduce/eliminate the setting of
attributes during reflow, could you change 'dir' here to be a member variable?
Thanks.

I don't think this is 1.6 material, BTW. It's not a high visibility bug and not
a regression AFAIK.

Comment 9

[Boris Zbarsky [:bzbarsky]]

OK.  I'll try to move that into a member var.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch with member

Comment 11

[Boris Zbarsky [:bzbarsky]]

Checked in for 1.7a.