1515822 - scroll position sometimes is not saved inside display: contents

Status: RESOLVED FIXED

(Core :: Layout: Scrolling and Overflow, defect, P3)

Description

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Attachment: scroll.html

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:64.0) Gecko/20100101 Firefox/64.0

Steps to reproduce:

1. Make a div display: contents, let's say its name is A.
2. Make a scroll-able div, put it inside the div A.
3. Use history.pushState/popstate to change the div A to display: none.

For more testing please see the attached file.


Actual results:

The scroll position does not restored.


Expected results:

The scroll position should be restored.

Comment 1

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Attachment: wip.patch

Just a WIP that works, but I don't think it's optimal.

The reason why I cannot use IsDisplayContents is that at the time we call it in nsCSSFrameConstructor::RecreateFramesForContent, the flag has been unset.

:emilio, do you know we have any flags in servo to distinguish "display: contents" and "display: none" in this case?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

display: contents is not a flag, but by the time you get there we have already forgotten of the old style.

So we need to walk the content tree in a similar fashion in ContentRemoved:

  https://searchfox.org/mozilla-central/rev/9528360768d81b1fc84258b5fb3601b5d4f40076/layout/base/nsCSSFrameConstructor.cpp#7451

So looks to me that all the ContentRemoved callers which aren't ContentRemoved itself and are REMOVE_FOR_RECONSTRUCTION call CaptureStateForFramesOf on their own. It would make sense to move the CaptureStateForFramesOf to ContentRemoved itself, and change the REMOVE_FOR_RECONSTRUCTION flag here:

  https://searchfox.org/mozilla-central/rev/9528360768d81b1fc84258b5fb3601b5d4f40076/layout/base/nsCSSFrameConstructor.cpp#7463

For aFlags. I think that's more correct anyway, actually.

Comment 3

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Attachment: Bug 1515822 - Capture frame state in nsCSSFrameConstructor::ContentRemoved. r=emilio,mats

Move CaptureStateForFramesOf into ContentRemoved, so we can traverse frames
which were under display: contents as well.

Comment 4

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Attachment: Bug 1515822 - Add testcase. r=emilio

Depends on D15319

Comment 5

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Unfortunately I've lost my ssh key to push to try or autoland.
But the testcase passed on my machine.

Comment 6

[Cameron McCormack (:heycam)]

(In reply to Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla) from comment #5)
> Unfortunately I've lost my ssh key to push to try or autoland.

Here's a try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=657e3158cb9632fd3260aadf8e00932e46124821

Comment 7

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

(In reply to Cameron McCormack (:heycam) (away Jan 1) from comment #6)
> Here's a try run:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=657e3158cb9632fd3260aadf8e00932e46124821

Thanks!

I guess the assertion is a little bit aggressive.
I will try to get my try server key back, would be better.

Comment 8

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

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

Comment 9

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Since :mats is blocking review now, and I'm not sure when will he be available again, do we have other victim to review this bug?
Thanks.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

(In reply to Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla) from comment #9)

Since :mats is blocking review now, and I'm not sure when will he be available again, do we have other victim to review this bug?
Thanks.

I suspect he just missed the Phabricator review request.

Comment 11

[Mats Palmgren (inactive)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #10)

(In reply to Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla) from comment #9)

Since :mats is blocking review now, and I'm not sure when will he be available again, do we have other victim to review this bug?
Thanks.

I suspect he just missed the Phabricator review request.

Hmm, I have no clue why it says I'm blocking review requests.
My bugzilla account certainly isn't blocking requests.
Does Phabricator also have setting for that? I couldn't find one...

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/46591113989e
Capture frame state in nsCSSFrameConstructor::ContentRemoved. r=emilio,mats
https://hg.mozilla.org/integration/autoland/rev/47cafac7c288
Add testcase. r=emilio

Comment 13

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Thanks so much for your help! :)

Comment 14

[Emilio Cobos Álvarez (:emilio)]

(In reply to Mats Palmgren (:mats) from comment #11)

Hmm, I have no clue why it says I'm blocking review requests.
My bugzilla account certainly isn't blocking requests.
Does Phabricator also have setting for that? I couldn't find one...

In Phabricator there are two kinds of reviewers, blocking and non-blocking. Blocking reviewers need to approve the patch for it to land, non-blocking reviewers don't, as long as at least one of them have approved the patch.

So tagging you and Daniel both as a blocking reviewer, for example, would mean that I need review from both, tagging you both as a non-blocking reviewers means that I need review from at least one of you.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

(In reply to Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla) from comment #13)

Thanks so much for your help! :)

NP, thank you for fixing it!

Comment 16

[Mats Palmgren (inactive)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #14)

In Phabricator there are two kinds of reviewers, blocking and non-blocking. Blocking reviewers need to approve the patch for it to land, non-blocking reviewers don't, as long as at least one of them have approved the patch.

Ah, Wei-Cheng's comment makes sense now. Thanks.

Comment 17

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/46591113989e
https://hg.mozilla.org/mozilla-central/rev/47cafac7c288