Closed Bug 1337068 Opened 7 years ago Closed 7 years ago

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

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: bholley, Assigned: mbrubeck)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

Bug 1337068 - stylo: :empty, :-moz-first-node, :-moz-last-node, and :-moz-only-whitespace.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1337068 - stylo: Update test expectations.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Part 3: Update more test expectations
2.10 KB, patch
Details | Diff | Splinter Review 
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.
See bug 1341781 comment 0 for a simple testcase.
Assignee: nobody → mbrubeck
Blocks: stylo-reftest, stylo-syntax
Priority: -- → P1
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
Status: NEW → ASSIGNED
> 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.
Blocks: 1343496
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.
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 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 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 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+
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
Attachment #8849164 - Attachment is obsolete: true
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
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/a1ee487645c3
stylo: Update more test expectations. r=test-expectations-update
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: