Closed Bug 1305579 Opened 3 years ago Closed 3 years ago

Toggling iframe with display:none lost scroll position

Categories

(Core :: Layout, defect)

48 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox49 --- wontfix
firefox-esr45 --- unaffected
firefox50 --- wontfix
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: alice0775, Assigned: kats)

References

Details

(Keywords: regression, Whiteboard: [parity-Chrome][parity-Edge])

Attachments

(2 files)

Attached file testcase
The problem occurs since Firefox48.
Firefox47, Edge and Chrome works as expected.

Steps To Reproduce:
1. Open testcase attached
2. Scroll down iframe content
3. Click on [TOGGLE] twice to toggle css display property

Actual Results:
lost scroll position

Expected Results:
keep scroll position


Regression Window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=6aa3f079d304ddb862ee9e5893683b9b883883f4&tochange=9489784a0ea049b149ad0c6d22ac121ef2542239

Regressed by:
9489784a0ea0	Kartikaya Gupta — Bug 1269539 - Ensure that the scroll position is restored properly on reloading a page which loads incrementally. r=tnikkel
Flags: needinfo?(bugmail)
Summary: Toggling iframe with display:none will lost scroll position → Toggling iframe with display:none lost scroll position
See Also: → 1305283
*sigh* never ending stream of scroll position bugs...
Assignee: nobody → bugmail
Flags: needinfo?(bugmail)
What seems to be happening is that at the point that ScrollToRestoredPosition is called, the scrollRange is still 0. And since the page isn't "loading" anymore, we don't take the branch at [1] and instead continue down to clear mRestorePos and mLastPos. If we took that branch things would be better, so we probably need to modify the conditional to also cover this scenario somehow.

[1] http://searchfox.org/mozilla-central/rev/767e1e9b118269f957ca1817138472146278a29e/layout/generic/nsGfxScrollFrame.cpp#4230
Try push is green, and I manually verified Alice's testcase works as expected on the build  for OS X (and anyway I turned Alice's test into a mochitest which is included in the try push - thanks Alice for the reduced testcase!).

Unfortunately the patch doesn't seem to fix bug 1117640 which sounds similar. However that bug was filed long before I made any of my changes so it probably has a different root cause. I'll put the patch up for review.
Comment on attachment 8795678 [details]
Bug 1305579 - If we fail to restore the scroll position while there's a reflow pending, hold on to it and try again later.

https://reviewboard.mozilla.org/r/81644/#review80486
Attachment #8795678 - Flags: review?(tnikkel) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f52d02de70b2
If we fail to restore the scroll position while there's a reflow pending, hold on to it and try again later. r=tnikkel
https://hg.mozilla.org/mozilla-central/rev/f52d02de70b2
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment on attachment 8795678 [details]
Bug 1305579 - If we fail to restore the scroll position while there's a reflow pending, hold on to it and try again later.

Approval Request Comment
[Feature/regressing bug #]: bug 1269539
[User impact if declined]: In some cases, hiding and then re-showing an iframe by setting/unsetting display:none on it will cause the iframe to lose its scroll position and go back to 0,0
[Describe test coverage new/current, TreeHerder]: added an automated test for it
[Risks and why]: moderate risk because there have been a ton of bugs with losing the scroll position and it seems like every fix regresses something else. however we have been adding automated tests with each fix so in theory the code should be getting better. I'm requesting aurora uplift only for now to get more coverage on this and catch any fallout sooner.
[String/UUID change made/needed]: none
Attachment #8795678 - Flags: approval-mozilla-aurora?
Duplicate of this bug: 1305283
one more question: Will the next stable Firefox have this fixed? When will it available?
kats asked for uplift to aurora, which would mean that the fix would not make it into the next stable release, but the one after that.
Comment on attachment 8795678 [details]
Bug 1305579 - If we fail to restore the scroll position while there's a reflow pending, hold on to it and try again later.

Fix a regression related to layout. Take it in 51 aurora.
Attachment #8795678 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Nokizilla from comment #11)
> one more question: Will the next stable Firefox have this fixed? When will
> it available?

(In reply to Timothy Nikkel (:tnikkel) from comment #12)
> kats asked for uplift to aurora, which would mean that the fix would not
> make it into the next stable release, but the one after that.

As it stands it should be available in Firefox 51. However I could request uplift to beta (Firefox 50) as well if we don't uncover problems with the patch.
Even though we haven't seen any regressions reported from this patch I'm a little hesitant to request uplift to beta. This code in general has been touched a lot and I'd rather let it ride the trains to get more bake time unless there's an urgent need to ship it sooner - which doesn't seem to be the case here.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #16)
> Even though we haven't seen any regressions reported from this patch I'm a
> little hesitant to request uplift to beta. This code in general has been
> touched a lot and I'd rather let it ride the trains to get more bake time
> unless there's an urgent need to ship it sooner - which doesn't seem to be
> the case here.

Unsure about what is regressions means, is it the one I did the my duplicate bug report https://bugzilla.mozilla.org/show_bug.cgi?id=1305283

This bug affect my everyday work, I still on FF 48.0.2 due to this issue. Maybe I need to use the beta version in the next release to avoid the bug.
(In reply to Nokizilla from comment #17)
> Unsure about what is regressions means

What I meant by regressions is other stuff breaking as a result of this fix. So far we haven't seen any regressions, meaning we haven't gotten reports of things broken as a result of this fix.

> This bug affect my everyday work, I still on FF 48.0.2 due to this issue.
> Maybe I need to use the beta version in the next release to avoid the bug.

Yes, that's certainly an option for you. So far you are the only one to have reported this problem (I'm pretty sure Alice filed this bug after looking at your bug report in bug 1305283), so it's likely not a critical bug for a large number of people. So I don't want to rush through into a sooner release because we might find out later that it caused a regression that affects a lot of people.
You need to log in before you can comment on or make changes to this bug.