The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
a month ago
41 minutes ago

People

(Reporter: bholley, Assigned: mbrubeck)

Tracking

(Blocks: 4 bugs)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 1 obsolete attachment)

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.
Depends on: 1336646
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
(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.
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.
(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.
(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.
Blocks: 1321197
See bug 1341781 comment 0 for a simple testcase.
Assignee: nobody → mbrubeck
Blocks: 1324348, 1345200
Priority: -- → P1
Duplicate of this bug: 1341781
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.
Summary: stylo: implement support for :-moz-only-whitespace in rust-selectors → stylo: implement support for :-moz-only-whitespace and :-moz-first-node in rust-selectors
(Assignee)

Updated

7 days ago
Status: NEW → ASSIGNED
(Assignee)

Comment 9

6 days ago
> 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.
(Assignee)

Updated

5 days ago
Blocks: 1343496
Comment hidden (mozreview-request)
Comment hidden (obsolete)
Comment hidden (mozreview-request)
(Assignee)

Comment 13

5 days ago
Fixed a logic error, new Try push:

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

Comment 14

5 days ago
mozreview-review
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.
Attachment #8848664 - Flags: review?(emilio+bugs)
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 19

2 days ago
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
Depends on: 1341086

Comment 20

2 days ago
mozreview-review
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.
Attachment #8849164 - Flags: review?(emilio+bugs)

Comment 21

2 days ago
mozreview-review
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
Attachment #8848664 - Flags: review?(emilio+bugs) → review+

Comment 22

2 days ago
mozreview-review
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.
Attachment #8849165 - Flags: review?(emilio+bugs) → review+
(Assignee)

Comment 23

a day ago
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
(Assignee)

Comment 24

8 hours ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=034d26f8d9ec5d6b463a2a4499e831e48619fef4
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

8 hours ago
Attachment #8849164 - Attachment is obsolete: true

Comment 27

7 hours ago
Pushed by manishearth@gmail.com:
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
Duplicate of this bug: 1343496
(Assignee)

Comment 29

6 hours ago
Created attachment 8850136 [details] [diff] [review]
Part 3: Update more test expectations

Comment 30

6 hours ago
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/a1ee487645c3
stylo: Update more test expectations. r=test-expectations-update

Comment 31

41 minutes ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1a170dacf4a6
https://hg.mozilla.org/mozilla-central/rev/4e46b5d133d1
https://hg.mozilla.org/mozilla-central/rev/a1ee487645c3
Status: ASSIGNED → RESOLVED
Last Resolved: 41 minutes ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.