Closed
Bug 226534
Opened 21 years ago
Closed 21 years ago
[FIXr]Getting "Back" does not restore horizontal scroller position
Categories
(Core :: Layout, defect, P2)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla1.7alpha
People
(Reporter: relf, Assigned: bzbarsky)
References
()
Details
Attachments
(1 file, 1 obsolete file)
3.75 KB,
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•21 years ago
|
||
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
Assignee | ||
Comment 2•21 years ago
|
||
Assignee | ||
Comment 3•21 years ago
|
||
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)
Assignee | ||
Comment 4•21 years ago
|
||
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? :-)
Assignee | ||
Comment 6•21 years ago
|
||
> 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.
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.
Assignee | ||
Comment 9•21 years ago
|
||
OK. I'll try to move that into a member var.
Attachment #136135 -
Flags: superreview?(roc)
Attachment #136135 -
Flags: review?(roc)
Assignee | ||
Comment 10•21 years ago
|
||
Attachment #136135 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
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+
Assignee | ||
Updated•21 years ago
|
Summary: [FIX]Getting "Back" does not restore horizontal scroller position → [FIXr]Getting "Back" does not restore horizontal scroller position
Assignee | ||
Comment 11•21 years ago
|
||
Checked in for 1.7a.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Product: Core → Core Graveyard
Updated•6 years ago
|
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in
before you can comment on or make changes to this bug.
Description
•