1337068 - stylo: implement support for :-moz-only-whitespace and :-moz-first-node in rust-selectors

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Bobby Holley (:bholley)]

When we do this, we'll want to make sure that we set the AFFECTED_BY_EMPTY StyleRelation, so that we end up setting NODE_HAS_EMPTY_SELECTOR on the nsINode appropriately.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

I guess we need to have support for custom tree-structural pseudo-classes then, which kinda sucks :(.

Also related: https://drafts.csswg.org/selectors-4/#the-blank-pseudo

Comment 2

[Simon Sapin (:SimonSapin)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #1)
> I guess we need to have support for custom tree-structural pseudo-classes

Because Element::match_non_ts_pseudo_class doesn’t have access to &mut StyleRelations? We could add that if needed.

Also, the split between pseudo-classes implemented entirely in rust-selectors and those generic through NonTSPseudoClass is really a split between:

* Those we can match with just methods in the Element trait already needed to deal with combinators
* Those of which different users (Servo/Gecko/Kuchiki) might want to support (and parse) a different set

I don’t mind having a "tree structural" pseudo-class (that is based on the shape of the tree) in the latter kind. We can always rename NonTSPseudoClass.

Comment 3

[Simon Sapin (:SimonSapin)]

We can also implement :blank full in rust-selectors, but CSSWG seems unsure about its name.

Which reminds me: before shipping Stylo in Firefox, in addition to looking for regressions (and fixing them), we should also look for every new features that Stylo implement and wasn’t in Gecko before. And for each of these, case by case, either gate it on a pref or send an Intent to Ship.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

(In reply to Simon Sapin (:SimonSapin) from comment #2)
> I don’t mind having a "tree structural" pseudo-class (that is based on the
> shape of the tree) in the latter kind. We can always rename NonTSPseudoClass.

Heh, I thought you would be somewhat more hesitant to do so, and wanted to support all the standard tree-structural stuff in rust-selectors itself. Then I guess renaming it to CustomPseudoClass or something is fine too.

Comment 5

[Bobby Holley (:bholley)]

(In reply to Simon Sapin (:SimonSapin) from comment #3)
> Which reminds me: before shipping Stylo in Firefox, in addition to looking
> for regressions (and fixing them), we should also look for every new
> features that Stylo implement and wasn’t in Gecko before. And for each of
> these, case by case, either gate it on a pref or send an Intent to Ship.

Filed bug 1337166.

Comment 6

[Bobby Holley (:bholley)]

See bug 1341781 comment 0 for a simple testcase.

Comment 8

[Bobby Holley (:bholley)]

Throwing :-moz-first-node in here so that this properly subsumes bug 1341781. Feel free to break it out if it's additional complex work.

Comment 9

[Matt Brubeck (:mbrubeck)]

> Because Element::match_non_ts_pseudo_class doesn’t have access to &mut StyleRelations? We could add that if needed.

https://github.com/servo/servo/pull/15966 adds this.

Comment 10

[Matt Brubeck (:mbrubeck)]

Attachment: Bug 1337068 - stylo: :empty, :-moz-first-node, :-moz-last-node, and :-moz-only-whitespace.

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

Comment 11

[Matt Brubeck (:mbrubeck)]

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

Comment 12

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8848664 [details]
Bug 1337068 - stylo: :empty, :-moz-first-node, :-moz-last-node, and :-moz-only-whitespace.

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

Comment 13

[Matt Brubeck (:mbrubeck)]

Fixed a logic error, new Try push:

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

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8848664 [details]
Bug 1337068 - stylo: :empty, :-moz-first-node, :-moz-last-node, and :-moz-only-whitespace.

https://reviewboard.mozilla.org/r/121566/#review123650

I'd probably ask a DOM peer to sign-off the CACHED_TEXT_IS_ONLY_WHITESPACE flag change though, and I'd like to do another pass once the comments are addressed, but looks good overall!

Thank you! All that unexpected pass orange looks pretty exciting :)

::: dom/base/FragmentOrElement.h:145
(Diff revision 2)
>      return SetText(aStr.BeginReading(), aStr.Length(), aNotify);
>    }
>    virtual nsresult AppendText(const char16_t* aBuffer, uint32_t aLength,
>                                bool aNotify) override;
>    virtual bool TextIsOnlyWhitespace() override;
> +  virtual bool TextIsOnlyWhitespaceConst() const override;

nit: I believe we use `ThreadSafe` as a prefix instead of `Const` as a suffix in other places (read: nsStyleContext). Also, I believe it's more descriptive of the requirements. So please rename to `ThreadSafeTextIsOnlyWhitespace`.

::: dom/base/nsGenericDOMDataNode.cpp:997
(Diff revision 2)
>  }
>  
>  bool
>  nsGenericDOMDataNode::TextIsOnlyWhitespace()
>  {
> +  if (!TextIsOnlyWhitespaceConst()) {

This has the side effect of making another virtual call. Please mark the relevant method as `final`, or directly use `nsGenericDOMDataNode::ThreadSafeTextIsOnlyWhitespace`.

::: dom/base/nsGenericDOMDataNode.cpp:1011
(Diff revision 2)
> +
> +bool
> +nsGenericDOMDataNode::TextIsOnlyWhitespaceConst() const
> +{
>    // FIXME: should this method take content language into account?
>    if (mText.Is2b()) {

This bits concerns me. We were not setting the CACHED_TEXT_IS_ONLY_WHITESPACE flag if the text is non-8bit, but we are doing that now.

Have you looked at the possible implications of this? I don't know if it can change under the hood. It seems to, but I think it is catched in SetTextInternal, isn't it? Probably a more experienced DOM peer can sign it off instead.

::: layout/style/ServoBindings.cpp:97
(Diff revision 2)
> + *
> + * This has the same semantics as nsStyleUtil::IsSignificantChild, but can be
> + * called on a `const nsINode*` safely.
> + */
> +bool
> +Gecko_IsSignificantChild(RawGeckoNodeBorrowed aNode, bool aTextIsSignificant,

It's slightly unfortunate to make this duplication, but I guess it's fine.

::: layout/style/ServoBindings.cpp:100
(Diff revision 2)
> + */
> +bool
> +Gecko_IsSignificantChild(RawGeckoNodeBorrowed aNode, bool aTextIsSignificant,
> +                         bool aWhitespaceIsSignificant)
> +{
> +  const nsIContent *child = aNode->AsContent();

nit: Keep the star near the type: const nsIContent*.

::: layout/style/ServoBindings.cpp:101
(Diff revision 2)
> +bool
> +Gecko_IsSignificantChild(RawGeckoNodeBorrowed aNode, bool aTextIsSignificant,
> +                         bool aWhitespaceIsSignificant)
> +{
> +  const nsIContent *child = aNode->AsContent();
> +  NS_ASSERTION(!aWhitespaceIsSignificant || aTextIsSignificant,

Let's just make this a `MOZ_ASSERT`.

::: servo/components/style/gecko/wrapper.rs:648
(Diff revision 2)
>              Gecko_IsRootElement(self.0)
>          }
>      }
>  
>      fn is_empty(&self) -> bool {
> -        // XXX(emilio): Implement this properly.
> +        !self.as_node().children().any(|child| unsafe {

You don't want to use `children()` here. `children()` is a `LayoutIterator`, which skips processing instructions, and also has different semantics for insertion points in Gecko IIRC.

You should instead either use `next_sibling()`/`prev_sibling()` directly, or extract the `GeckoChildrenIterator::Current` logic into its own iterator (and call it, let's say, `dom_children()`, which is what you want here.

This is caught by layout/reftests/bugs/315620-2b.xhtml, which in Stylo paints a green box, and in every other browser a red one, because there's a significant CDATA section.

Comment 15

[Manish Goregaokar [:manishearth]]

I've extracted the IsSignificantChild stuff into a separate patch in bug 1341086. It adds nsStyleUtil::ThreadSafeIsSignificantChild, which you can call from Gecko_IsSignificantChild. Feel free to cherry-pick.

Comment 16

[Matt Brubeck (:mbrubeck)]

Attachment: Bug 1337068 - Part 3: stylo: Add thread-safe version of nsStyleUtil::IsSignificantChild();

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

Comment 17

[Matt Brubeck (:mbrubeck)]

Attachment: Bug 1337068 - stylo: Update test expectations.

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

Comment 18

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8848664 [details]
Bug 1337068 - stylo: :empty, :-moz-first-node, :-moz-last-node, and :-moz-only-whitespace.

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

Comment 19

[Matt Brubeck (:mbrubeck)]

Rebased, addressed review comments, and triggered a new try build.  This now builds on top of Manish's patch from bug 1341086.  (A copy of that patch is included in this mozreview/try push, but it is already reviewed and will land with bug 1341086.)

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

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8849164 [details]
Bug 1337068 - Part 3: stylo: Add thread-safe version of nsStyleUtil::IsSignificantChild();

https://reviewboard.mozilla.org/r/121668/#review124148

I'm assuming this won't land, in favor of the version with review comments at bug 1341086, so clearing the flag for now.

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8848664 [details]
Bug 1337068 - stylo: :empty, :-moz-first-node, :-moz-last-node, and :-moz-only-whitespace.

https://reviewboard.mozilla.org/r/121566/#review124168

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8849165 [details]
Bug 1337068 - stylo: Update test expectations.

https://reviewboard.mozilla.org/r/122004/#review124146

r=me with the failures annotated, and assuming the new assertion is legit.

::: layout/generic/crashtests/crashtests.list:346
(Diff revision 1)
>  load 473894-1.html
>  load 476241-1.html
>  load 477731-1.html
>  load 477928.html
>  load 478131-1.html
> -load 478170-1.html
> +asserts-if(stylo,4) load 478170-1.html

Why is this asserting? Could you file a bug?

::: layout/reftests/bugs/reftest-stylo.list:355
(Diff revision 1)
>  fails == 311822-1.html 311822-1.html # bug 1341712, bug 1341714
>  fails == 311822-1.html 311822-1.html # bug 1341712, bug 1341714
> -fails == 315620-1a.html 315620-1a.html
> +== 315620-1a.html 315620-1a.html
>  == 315620-1b.html 315620-1b.html
> -fails == 315620-2a.xhtml 315620-2a.xhtml
> -== 315620-2b.xhtml 315620-2b.xhtml
> +== 315620-2a.xhtml 315620-2a.xhtml
> +fails == 315620-2b.xhtml 315620-2b.xhtml

Please file bugs for the new failing test cases, or annotate with the correct bug number.

Comment 23

[Matt Brubeck (:mbrubeck)]

Try push with new test failures annotated, rebased on top of current mozilla-central and latest patches from bug 1341086:

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

Comment 24

[Matt Brubeck (:mbrubeck)]

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

Comment 25

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8848664 [details]
Bug 1337068 - stylo: :empty, :-moz-first-node, :-moz-last-node, and :-moz-only-whitespace.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121566/diff/3-4/

Comment 26

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8849165 [details]
Bug 1337068 - stylo: Update test expectations.

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

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1a170dacf4a6
stylo: :empty, :-moz-first-node, :-moz-last-node, and :-moz-only-whitespace. r=emilio
https://hg.mozilla.org/integration/autoland/rev/4e46b5d133d1
stylo: Update test expectations. r=emilio

Comment 29

[Matt Brubeck (:mbrubeck)]

Attachment: Part 3: Update more test expectations

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a1ee487645c3
stylo: Update more test expectations. r=test-expectations-update

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1a170dacf4a6
https://hg.mozilla.org/mozilla-central/rev/4e46b5d133d1
https://hg.mozilla.org/mozilla-central/rev/a1ee487645c3