Cleanup a bit our GetComputedStyle setup.

RESOLVED FIXED in Firefox 68

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 months ago
a month ago

People

(Reporter: emilio, Assigned: emilio)

Tracking

(Blocks 1 bug)

unspecified
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

Attachments

(4 attachments)

No description provided.
Assignee

Comment 1

2 months ago

Some of these were unused, some of them were only used in combination with
others, so I've unified them.

In particular, Forgetful and ClearAnimationOnlyDirtyDescendants were used only
together for a very specific task (the final animation traversal), so I merged
them into something that has that name.

ClearDirtyBits was unused, so I removed along with some code that would no
longer be called.

Assignee

Comment 2

2 months ago

This switches nsFrameSetFrame's hacky frame construction codepath to operate on
the flattened tree, since it made me a bit more comfortable about it (all layout
should operate on is the flattened tree, though in this cause this should not
cause any web-observable behavior change, since <frameset> can't be a shadow
host per spec, and we no longer support XBL-in-content).

That doesn't need to compute styles lazily. You only need to compute styles
lazily in descendants of display: none elements, and even though this code
doesn't check on display: none on the children, the parent element should
never have display: none (since we're creating an nsFrameSetFrame for it).

There are other places that still call into this (apart from getComputedStyle).

One is nsImageFrame's cursor code. Given the <area> elements referencing an
image map could be anywhere, we need to have lazy computation here.

The other is the viewport style propagation stuff. There shouldn't be a need for
LazyComputeBehavior::Allow on the document element in order to check the
viewport styles propagation, since the root element can't have display: none
ancestors. But we run that code before actually constructing the doc element
containing block, which is when we do the initial document styling.

We could remove that with some more effort, but it's not worth it right now,
since we need to keep using it for the <body>, since the document element could
be display: none itself, and we propagate the overflow styles in that case
still. I filed https://github.com/w3c/csswg-drafts/issues/3779 to potentially
change that.

Depends on D25454

Assignee

Comment 3

2 months ago

There are some that only have one caller, and some slightly confusing naming.
Hopefully make it a bit clearer.

Depends on D25455

Assignee

Comment 4

2 months ago

It's the caller's responsibility to have up-to-date styles, and
nsComputedDOMStyle (which is the only of those callers that could ever care
about EffectCompositor stuff) already does it.

There's no need to explicitly update animation rules from here, since it would
only have a difference for display: none subtrees anyway (otherwise we re-use
the cached style already in the element before having a chance to process the
potential animation restyles).

My guess is that this was just copy-pasta from other functions. This doesn't
break any test.

Depends on D25457

Comment 5

a month ago
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9194b56c65fa
Cleanup unused style traversal flags. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/ae9783be9932
Remove some useless usage of LazyComputeBehavior::Allow. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/07a87bbee9ca
Cleanup a bit the lazy style resolution APIs. r=heycam
https://hg.mozilla.org/integration/autoland/rev/e1fa16165c52
Stop uselessly calling EffectCompositor from ResolveStyleLazily. r=hiro
You need to log in before you can comment on or make changes to this bug.