Nothing Special   »   [go: up one dir, main page]

Page MenuHomePhabricator

Bug 1676791 - Part 9: Define Scroller to handle the source of ScrollTimeline.
ClosedPublic

Authored by boris on Nov 19 2021, 3:41 AM.

Details

Summary

Per spec, "auto" represents the scrolling element of the document.
However, the scrolling element might be changed, based on the layout, in
quirks mode. Besides, the content of the root scroll frame is the root
element, instead of the scrolling element (e.g. body element) in both
standard and quirks modes. So now we define a special type, Scroller, to
represent the source of scroll-timeline, and use its |mType| to decide
which scroll frame we would like to use. In addition, hope this change let
us easier to implement nearest scroller.

Note: for auto scroller, we register this ScrollTimeline to the root
element, in both modes. Once we expose ScrollTimeline interface to the
script, we can rely on the |mType| of Scroller to return the correct
source element, whether it is scrolling element or not.

Diff Detail

Repository
rMOZILLACENTRAL mozilla-central
Lint
Lint Not Applicable
Unit
Tests Not Applicable
Build Status
Buildable 379388
Build 474225: arc lint + arc unit

Event Timeline

Update comment of part 8 and write a wip for part 9

phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".
phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".
phab-bot removed a project: secure-revision.
boris retitled this revision from Bug 1676791 - Part 9: Use the scrolling element of Document and fix bugs in quirks mode. to Bug 1676791 - Part 9: Use the scrolling element of document as default scroller..
boris edited the summary of this revision. (Show Details)
emilio requested changes to this revision.Nov 24 2021, 12:11 PM
emilio added inline comments.
dom/base/Document.h
3510 ↗(On Diff #511721)

enum class FlushLayout : bool { No, Yes }, though I think with my other comment addressed this change can go.

layout/style/nsAnimationManager.cpp
270 ↗(On Diff #511721)

Instead of this, can we use a nullptr source to mean the scrolling element of the document? It doesn't make sense to snapshot the scrolling element here because on quirks mode it's mutable by definition.

This revision now requires changes to proceed.Nov 24 2021, 12:11 PM
boris retitled this revision from Bug 1676791 - Part 9: Use the scrolling element of document as default scroller. to Bug 1676791 - Part 9: Define Scroller to handle the source of ScrollTimeline..Nov 24 2021, 9:03 PM
boris edited the summary of this revision. (Show Details)
boris marked an inline comment as done.

Note: change reviewer because we don't touch dom/base/Docuemnt now.

layout/style/nsAnimationManager.cpp
270 ↗(On Diff #511721)

I define Scroller for this, but didn't use nullptr in this version. it's still better to know which element holds this scroll timeline, and so I still use the document element for the register.

boris marked an inline comment as done.

Add an error handle

emilio added a project: testing-approved.

Much nicer, thanks!

testing/web-platform/tests/scroll-animations/css/at-scroll-timeline-default-descriptors-quirks-mode.html
2

Maybe

This revision is now accepted and ready to land.Nov 25 2021, 1:21 AM
testing/web-platform/tests/scroll-animations/css/at-scroll-timeline-default-descriptors-quirks-mode.html
2

@emilio: Maybe drop this and add <body class="reftest-wait"> and </body> below?

testing/web-platform/tests/scroll-animations/css/at-scroll-timeline-default-descriptors-quirks-mode.html
2

Oops. it seems reftest-wait doesn't work on body element. Not sure what is the best way to use reftest-wait in quirks mode.

testing/web-platform/tests/scroll-animations/css/at-scroll-timeline-default-descriptors-quirks-mode.html
2

No, sorry, on html is fine. I just wanted to say "maybe drop a comment to note that quirks are intentional", but then realized that the test had quirks mode in the name so probably not needed.

Address final comments and rebased

Code analysis found 4 defects in the diff 513977:

  • 4 build errors found by clang-tidy

You can run this analysis locally with:

  • ./mach static-analysis check --outgoing (C/C++)

If you see a problem in this automated review, please report it here.

You can view these defects on the code-review frontend and on Treeherder.

Move test and ready to land

boris added a commit: Restricted Diffusion Commit.Feb 18 2022, 7:45 PM