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

RESOLVED FIXED in mozilla1.7alpha

Status

()

Core
Layout: Misc Code
P2
normal
RESOLVED FIXED
15 years ago
15 years ago

People

(Reporter: Max Alekseyev, Assigned: bz)

Tracking

Trunk
mozilla1.7alpha
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

15 years ago
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
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...
Assignee: history → misc
Component: History: Session → Layout: Misc Code
Comment on attachment 136135 [details] [diff] [review]
Proposed patch

roc, what do you think?  Also, is this 1.6 material?
Attachment #136135 - Flags: superreview?(roc)
Attachment #136135 - Flags: review?(roc)
Taking.
Assignee: misc → bz-vacation
Priority: -- → P2
Summary: Getting "Back" does not restore horizontal scroller position → [FIX]Getting "Back" does not restore horizontal scroller position
Target Milestone: --- → mozilla1.7alpha
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? :-)
> 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

15 years ago
similar bug: Bug 99094
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.
OK.  I'll try to move that into a member var.
Created attachment 137073 [details] [diff] [review]
Patch with member
Attachment #136135 - Attachment is obsolete: true
Attachment #137073 - Flags: superreview?(roc)
Attachment #137073 - Flags: review?(roc)
Attachment #137073 - Flags: superreview?(roc)
Attachment #137073 - Flags: superreview+
Attachment #137073 - Flags: review?(roc)
Attachment #137073 - Flags: review+
Summary: [FIX]Getting "Back" does not restore horizontal scroller position → [FIXr]Getting "Back" does not restore horizontal scroller position
Checked in for 1.7a.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.