1305579 - Toggling iframe with display:none lost scroll position

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Alice0775 White]

Attachment: 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

Comment 1

[Kartikaya Gupta (email:[e-mail])]

*sigh* never ending stream of scroll position bugs...

Comment 2

[Kartikaya Gupta (email:[e-mail])]

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

Comment 3

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=78af00f87dba

Comment 4

[Kartikaya Gupta (email:[e-mail])]

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 5

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1305579 - If we fail to restore the scroll position while there's a reflow pending, hold on to it and try again later.

Review commit: https://reviewboard.mozilla.org/r/81644/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/81644/

Comment 6

[Timothy Nikkel (:tnikkel)]

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
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

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f52d02de70b2

Comment 9

[Kartikaya Gupta (email:[e-mail])]

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

Comment 11

[Nokizilla]

one more question: Will the next stable Firefox have this fixed? When will it available?

Comment 12

[Timothy Nikkel (:tnikkel)]

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 13

[Gerry Chang [:gchang]]

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.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

(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.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9827864e90f2

Comment 16

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 17

[Nokizilla]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 18

[Kartikaya Gupta (email:[e-mail])]

(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.