1390694 - stylo: :-moz-window-inactive doesn't respond to window state change dynamically

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Xidorn Quan [:xidorn] UTC+10]

Attachment: testcase

See the testcase.

You would see a green background initially, and when you activate other window, the background should turn red, because now :-moz-window-inactive should match. However, in Stylo, it doesn't.

We need to invalidate selector matching somehow when window changes its state.

This causes test browser/base/content/test/general/browser_windowactivation.js to timeout.

Comment 2

[Bobby Holley (:bholley)]

Probably worth fixing this before shipping.

Comment 3

[Bobby Holley (:bholley)]

Xidorn makes a case for this being P3 in bug 1290285 comment 4.

Comment 4

[Cameron McCormack (:heycam)]

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

Comment 5

[Cameron McCormack (:heycam)]

Attachment: style: Remove Stylist::might_have_state_dependency.

We always know for sure whether we have a dependency on particular
event state bits, so just use has_state_dependency.

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

Comment 6

[Cameron McCormack (:heycam)]

Attachment: style: Remove unused SelectorImpl::pseudo_class_state_flag.

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

Comment 7

[Cameron McCormack (:heycam)]

Attachment: style: Record DocumentState dependencies.

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

Comment 8

[Cameron McCormack (:heycam)]

Attachment: geckolib: Add FFI function for checking document state dependencies.

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

Comment 9

[Cameron McCormack (:heycam)]

Attachment: Bug 1390694 - Part 1: Add ServoStyleSet function for checking document state dependencies.

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

Comment 10

[Cameron McCormack (:heycam)]

Attachment: Bug 1390694 - Part 2: Allow EnumerateBoundContentBindings callbacks to stop enumeration.

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

Comment 11

[Cameron McCormack (:heycam)]

Attachment: Bug 1390694 - Part 3: Add nsBindingManager function to check for document state dependencies.

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

Comment 12

[Cameron McCormack (:heycam)]

Attachment: Bug 1390694 - Part 4: Restyle the document when document state dependencies change.

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

Comment 13

[Cameron McCormack (:heycam)]

Originally I wanted to track the dependencies for invalidation just like we do for element states, but this proved to be more annoying than I'd hoped.  We would need to post an invalidation from the root of the document, since we don't have a specific element whose state has updated to start from.  (Or, you can think of it as some intrinsic state of every element in the document has changed, and some of those might generate invalidations.)  I managed to get the invalidation processing working even when starting from the very left of the selector, but then I realized I'd need to do something to allow the :-moz-window-inactive to match regardless of the actual current (or snapshotted) document state value.  That seemed a bit much just to make this one (or two) pseudo-class work, so I fell back to just posting a subtree restyle like Gecko does currently.

Comment 14

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8919652 [details]
style: Remove Stylist::might_have_state_dependency.

https://reviewboard.mozilla.org/r/190562/#review195784

Comment 15

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8919653 [details]
style: Remove unused SelectorImpl::pseudo_class_state_flag.

https://reviewboard.mozilla.org/r/190564/#review195786

Comment 16

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8919655 [details]
geckolib: Add FFI function for checking document state dependencies.

https://reviewboard.mozilla.org/r/190568/#review195788

::: servo/ports/geckolib/glue.rs:4053
(Diff revision 1)
>  #[no_mangle]
> +pub extern "C" fn Servo_StyleSet_HasDocumentStateDependency(
> +    raw_data: RawServoStyleSetBorrowed,
> +    state: u64,
> +) -> bool {
> +    let state = ElementState::from_bits_truncate(state);

Shouldn't this be `DocumentState`, and `has_document_state_dependency`?

Comment 17

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8919654 [details]
style: Record DocumentState dependencies.

https://reviewboard.mozilla.org/r/190566/#review195796

Comment 18

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8919655 [details]
geckolib: Add FFI function for checking document state dependencies.

https://reviewboard.mozilla.org/r/190568/#review195798

::: servo/ports/geckolib/glue.rs:4053
(Diff revision 1)
>  #[no_mangle]
> +pub extern "C" fn Servo_StyleSet_HasDocumentStateDependency(
> +    raw_data: RawServoStyleSetBorrowed,
> +    state: u64,
> +) -> bool {
> +    let state = ElementState::from_bits_truncate(state);

This fixed, r=me

Comment 19

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8919656 [details]
Bug 1390694 - Part 1: Add ServoStyleSet function for checking document state dependencies.

https://reviewboard.mozilla.org/r/190570/#review195800

Comment 20

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8919657 [details]
Bug 1390694 - Part 2: Allow EnumerateBoundContentBindings callbacks to stop enumeration.

https://reviewboard.mozilla.org/r/190572/#review195802

Comment 21

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8919658 [details]
Bug 1390694 - Part 3: Add nsBindingManager function to check for document state dependencies.

https://reviewboard.mozilla.org/r/190574/#review195804

Comment 22

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8919659 [details]
Bug 1390694 - Part 4: Restyle the document when document state dependencies change.

https://reviewboard.mozilla.org/r/190576/#review195808

r=me. Maybe file a bug on how we can make it better / more fine-grained and reference it from there? I agree that if Gecko was doing full doc restyles anyway it's unlikely to matter that much.

Comment 23

[Cameron McCormack (:heycam)]

Comment on attachment 8919656 [details]
Bug 1390694 - Part 1: Add ServoStyleSet function for checking document state dependencies.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190570/diff/1-2/

Comment 24

[Cameron McCormack (:heycam)]

Comment on attachment 8919657 [details]
Bug 1390694 - Part 2: Allow EnumerateBoundContentBindings callbacks to stop enumeration.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190572/diff/1-2/

Comment 25

[Cameron McCormack (:heycam)]

Comment on attachment 8919658 [details]
Bug 1390694 - Part 3: Add nsBindingManager function to check for document state dependencies.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190574/diff/1-2/

Comment 26

[Cameron McCormack (:heycam)]

Comment on attachment 8919659 [details]
Bug 1390694 - Part 4: Restyle the document when document state dependencies change.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190576/diff/1-2/

Comment 27

[Cameron McCormack (:heycam)]

Attachment: Servo PR

Comment 28

[Cameron McCormack (:heycam)]

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

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/484a2644318f
Part 1: Add ServoStyleSet function for checking document state dependencies. r=emilio
https://hg.mozilla.org/integration/autoland/rev/83ff9e245757
Part 2: Allow EnumerateBoundContentBindings callbacks to stop enumeration. r=emilio
https://hg.mozilla.org/integration/autoland/rev/e13dd2a3651f
Part 3: Add nsBindingManager function to check for document state dependencies. r=emilio
https://hg.mozilla.org/integration/autoland/rev/b9cff3469bfd
Part 4: Restyle the document when document state dependencies change. r=emilio

Comment 30

[Xidorn Quan [:xidorn] UTC+10]

We probably should re-enable browser/base/content/test/general/browser_windowactivation.js?

Comment 31

[Attila Craciun [:arny]]

Backed out 4 changesets (bug 1390694) for failing crash test and in reftest/tests/layout/reftests/xul/mac-tab-toolbar.xul 

https://hg.mozilla.org/integration/autoland/rev/ca6138ea6ef6800a6c4ff7625c2644d366ebcf3b

Comment 32

[Attila Craciun [:arny]]

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=b9cff3469bfd3a419116d623cff5e72019632b28

Comment 33

[Cameron McCormack (:heycam)]

I fixed the crash (needed a null check for the document root element), but the mac-tab-toolbar.xul failure is confusing.  I don't think I'll get a chance to look into this properly before I'm leave, so I'll leave the ni for myself.

Comment 34

[Chris Peterson [:cpeterson]]

status-firefox57=wontfix unless someone thinks this bug should block 57

Comment 35

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Cameron McCormack (:heycam) (away 24 Oct – 12 Nov) from comment #33)
> I fixed the crash (needed a null check for the document root element), but
> the mac-tab-toolbar.xul failure is confusing.  I don't think I'll get a
> chance to look into this properly before I'm leave, so I'll leave the ni for
> myself.

This test was initially introduced in bug 1340627 for catching tab switching regression. That regression was because of not popping layer correctly as indicated in bug 1340627 comment 35.

Looking at the patches, I'm suspecting this may be related to part 4, where it skips the final SchedulePaint() call in some cases where the old code didn't skip. I can have a try run and see if this is really the problem.

Comment 36

[Xidorn Quan [:xidorn] UTC+10]

It passes now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a02751235ca2414293c21c3a4e1380bc5dafc87

Comment 37

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Part 4: Restyle the document when document state dependencies change.

This is based on the original part 4. The difference is that this doesn't use early return, so the paint may be scheduled like what the function does before.

Comment 38

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Part 5: Enable browser_windowactivation.js test

Comment 40

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8924048 [details] [diff] [review]
Part 4: Restyle the document when document state dependencies change.

Review of attachment 8924048 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/PresShell.cpp
@@ +4305,5 @@
>    NS_PRECONDITION(aDocument == mDocument, "Unexpected aDocument");
>  
> +  if (mDidInitialize) {
> +    Element* rootElement = aDocument->GetRootElement();
> +    bool needRestyle = false;

Maybe a function like:

static bool NeedRestyleForDocumentStateChange(nsIDocument*, EventStates, RestyleManager*)

or a method looks nicer?

Your call, no big deal.

Comment 41

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #40)
> Comment on attachment 8924048 [details] [diff] [review]
> Part 4: Restyle the document when document state dependencies change.
> 
> Review of attachment 8924048 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/PresShell.cpp
> @@ +4305,5 @@
> >    NS_PRECONDITION(aDocument == mDocument, "Unexpected aDocument");
> >  
> > +  if (mDidInitialize) {
> > +    Element* rootElement = aDocument->GetRootElement();
> > +    bool needRestyle = false;
> 
> Maybe a function like:
> 
> static bool NeedRestyleForDocumentStateChange(nsIDocument*, EventStates,
> RestyleManager*)
> 
> or a method looks nicer?

It doesn't seem to me that would make it much nicer... I'll just leave it as-is.

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/faafb9307cb1
Part 1: Add ServoStyleSet function for checking document state dependencies. r=emilio
https://hg.mozilla.org/integration/autoland/rev/68e5971edabb
Part 2: Allow EnumerateBoundContentBindings callbacks to stop enumeration. r=emilio
https://hg.mozilla.org/integration/autoland/rev/4617a3675f58
Part 3: Add nsBindingManager function to check for document state dependencies. r=emilio
https://hg.mozilla.org/integration/autoland/rev/6b2e7817128b
Part 4: Restyle the document when document state dependencies change. r=emilio
https://hg.mozilla.org/integration/autoland/rev/0238eee84c25
Part 5: Enable browser_windowactivation.js test. r=emilio

Comment 43

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/faafb9307cb1
https://hg.mozilla.org/mozilla-central/rev/68e5971edabb
https://hg.mozilla.org/mozilla-central/rev/4617a3675f58
https://hg.mozilla.org/mozilla-central/rev/6b2e7817128b
https://hg.mozilla.org/mozilla-central/rev/0238eee84c25