Closed Bug 851445 Opened 11 years ago Closed 11 years ago

Back button does not restore scroll position on a specific site (Allegro.pl)

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Version:
19 Branch
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla23
Tracking Flags:
Tracking Status
firefox19 --- affected
firefox20 - ---
firefox21 - ---
firefox22 - ---

People

(Reporter: krzysztof.glebowicz, Assigned: roc)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

reduced testcase
(deleted), text/html
Details
fix v2
(deleted), patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 5.1; rv:22.0) Gecko/20130314 Firefox/22.0
Build ID: 20130314030914

Steps to reproduce:

After pressing back button on website allegro.pl, scroll position is not restored. Versions affected: 19 to 22. Version not affected: 18. 

Steps to reproduce:
1. Go to http://allegro.pl/dla-dzieci
2. Scroll to the middle of the page
3. Press any link (for instance "Akcesoria")
4. Press the Back button



Actual results:

Scroll position is not restored. I see the top of the page. 


Expected results:

Scroll position should be restored. I should see the "Akcesoria" link.
Blocks: 251784
Regression range:
m-c
good=2012-11-15
bad=2012-11-16
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a761bfc192b5&tochange=58ebb638a7ea

Suspected bug: Bug 811301
Status: UNCONFIRMED → NEW
status-firefox19: --- → affected
tracking-firefox20: --- → ?
tracking-firefox21: --- → ?
tracking-firefox22: --- → ?
Component: Untriaged → Layout
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
Blocks: 811301
We'd take a low risk uplift, but this is not critical enough to track for upcoming releases.
Assignee: nobody → roc
tracking-firefox20: ? → -
tracking-firefox21: ? → -
tracking-firefox22: ? → -
This is also an issue on engadget.com, amongst others -- for all cases: when going back, refreshing, or restoring session.
Attached file reduced testcase (deleted) — 
It looks like reframing the root element while the page is loading makes us forget the scroll position that's being restored.
This behavior gets triggered when Modernizr is loaded in <head>, which is probably why it affects many sites.
I think basically the problem is that before the patch in bug 811301 we extracted the old scrolling state from the history to the root scroll frame after document.load fired. But after bug 811301 we restore it into mRestorePos as soon as the root scroll frame is constructed and remove it from the history data. Then the reframe of the root scroll frame causes mRestorePos to be wiped.
Attached patch fix (obsolete) (deleted) — — Splinter Review 

        Attachment #727512 -
        Flags: review?(matspal)

    
    

    
  
Comment on attachment 727512 [details] [diff] [review]
fix

r=mats

        Attachment #727512 -
        Flags: review?(matspal) → review+

    
    

    
  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> Created attachment 727512 [details] [diff] [review]
> fix

I think there is a useless blank line (#12) at the end of the file bug851445_helper.html.

    
    

    
  
Is it possible to get this landed or is it still waiting for something?

    
    

    
  
(In reply to Loic from comment #9)
> I think there is a useless blank line (#12) at the end of the file
> bug851445_helper.html.

Thanks, fixed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd6b604d1706

    
    

    
  
Comment on attachment 727512 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 811301
User impact if declined: Fail to restore scroll position when going back to certain sites
Testing completed (on m-c, etc.): just landed
Risk to taking this patch (and alternatives if risky): pretty low-risk patch
String or IDL/UUID changes made by this patch: none

        Attachment #727512 -
        Flags: approval-mozilla-aurora?

    
    

    
  
Backed out due to test failure. https://hg.mozilla.org/integration/mozilla-inbound/rev/45dc77d9bb09

https://tbpl.mozilla.org/php/getParsedLog.php?id=21337983&tree=Mozilla-Inbound&full=1
22:45:16 INFO - 102207 ERROR TEST-UNEXPECTED-FAIL | /tests/content/events/test/test_wheel_default_action.html | doTestScroll(aSettings=all delta values are not customized), Scroll to only right by oblique pixel wheel event with overflow-y: hidden (body is scrollable): scrolled vertical - got 820, expected 1000
etc

    
    

    
  
This actually detected a real bug.

What happens is:
-- Scroll frame to 1000,820.
-- Reframe. mRestorePos.y == 820 in the new frame. mLastPos.y == 820.
-- Scroll frame to 1000,1000. mRestorePos.y is still 820, however.
-- Reframe. We use mRestorePos.y as the scroll state, and scroll back to 820.

The underlying issue is that we're only supposed to use mRestorePos if mLastPos == GetLogicalScrollPosition().

    
    

    
  

      
      
      
      

        Attached patch
          
          
        fix v2 (deleted)
        — 
          — Splinter Review
      

    


  

        Attachment #727512 -
        Attachment is obsolete: true

        Attachment #727512 -
        Flags: approval-mozilla-aurora?

        Attachment #732648 -
        Flags: review?(matspal)

    
    

    
  
Comment on attachment 732648 [details] [diff] [review]
fix v2

r=mats

        Attachment #732648 -
        Flags: review?(matspal) → review+

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/51ea67404430
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: