stylo: need to be able to match :-moz-table-border-nonzero on snapshots

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P2
normal
RESOLVED FIXED
8 months ago
7 months ago

People

(Reporter: bz, Assigned: heycam)

Tracking

(Blocks: 1 bug)

53 Branch
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

(4 attachments, 2 obsolete attachments)

Created attachment 8867987 [details]
baz.html

Right now we delegate it to the element (and eventually to nsCSSPseudoClasses::MatchesElement), but it depends on the value of the "border" attribute.

Also, we should ensure snapshots capture this attribute on <table> elements, though that might be happening by accident already.  Or not.

See attached testcase, which fails in stylo.

Updated

8 months ago
Priority: -- → P2
This is similar to bug 1365160.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

8 months ago
Attachment #8874330 - Flags: review?(emilio+bugs)
Attachment #8874331 - Flags: review?(emilio+bugs)
(Assignee)

Comment 10

8 months ago
mozreview-review
Comment on attachment 8874328 [details]
Bug 1365159 - Part 1: Record :-moz-browser-frame and :-moz-table-border-nonzero state on snapshots.

https://reviewboard.mozilla.org/r/145676/#review149610

::: layout/base/ServoRestyleManager.cpp:773
(Diff revision 2)
>  
> +  if ((aAttribute == nsGkAtoms::mozbrowser &&
> +       aElement->IsAnyOfHTMLElements(nsGkAtoms::iframe, nsGkAtoms::frame)) ||
> +      (aAttribute == nsGkAtoms::border &&
> +       aElement->IsHTMLElement(nsGkAtoms::table))) {
> +    // Add state for :-mozbrowser-frame and :-moz-table-border-nonzero.

This should be ":-moz-browser-frame", will fix locally.

Comment 11

8 months ago
mozreview-review
Comment on attachment 8874328 [details]
Bug 1365159 - Part 1: Record :-moz-browser-frame and :-moz-table-border-nonzero state on snapshots.

https://reviewboard.mozilla.org/r/145676/#review149668

::: layout/base/ServoRestyleManager.cpp:769
(Diff revision 2)
>    }
>  
>    ServoElementSnapshot& snapshot = SnapshotFor(aElement);
>    snapshot.AddAttrs(aElement);
>  
> +  if ((aAttribute == nsGkAtoms::mozbrowser &&

Can we move this condition to a static function in this file? It's kind of dense :-)

::: layout/base/ServoRestyleManager.cpp:774
(Diff revision 2)
> +  if ((aAttribute == nsGkAtoms::mozbrowser &&
> +       aElement->IsAnyOfHTMLElements(nsGkAtoms::iframe, nsGkAtoms::frame)) ||
> +      (aAttribute == nsGkAtoms::border &&
> +       aElement->IsHTMLElement(nsGkAtoms::table))) {
> +    // Add state for :-mozbrowser-frame and :-moz-table-border-nonzero.
> +    snapshot.AddOtherPseudoClassState(aElement);

Do we really need to record this for `:-moz-table-border-nonzero`?

I'd expect we could just use the attributes to match `:-moz-table-border-nonzero` using the recorded attributes...

I guess mozbrowser is harder...
Attachment #8874328 - Flags: review?(emilio+bugs) → review+

Comment 12

8 months ago
mozreview-review
Comment on attachment 8874329 [details]
Bug 1365159 - Part 2: Reftest.

https://reviewboard.mozilla.org/r/145678/#review149670
Attachment #8874329 - Flags: review?(emilio+bugs) → review+

Comment 13

8 months ago
mozreview-review
Comment on attachment 8874330 [details]
style: Minor restyle_hints.rs code reorganization.

https://reviewboard.mozilla.org/r/145680/#review149672
Attachment #8874330 - Flags: review?(emilio+bugs) → review+

Comment 14

8 months ago
mozreview-review
Comment on attachment 8874331 [details]
style: Support matching :-moz-browser-frame and :-moz-table-border-nonzero against snapshots.

https://reviewboard.mozilla.org/r/145682/#review149674

::: servo/components/style/gecko/selector_parser.rs:198
(Diff revision 2)
> +    /// Returns true if the evaluation of the pseudo-class depends on the
> +    /// element's attributes.
> +    pub fn is_attr_based(&self) -> bool {
> +        matches!(*self,
> +                 NonTSPseudoClass::MozTableBorderNonzero |
> +                 NonTSPseudoClass::MozBrowserFrame)

This also needs `:lang`, but I'm guessing we'll take care of that one in another bug.
Attachment #8874331 - Flags: review?(emilio+bugs) → review+
(Assignee)

Comment 15

8 months ago
(In reply to Emilio Cobos Álvarez [:emilio] from comment #11)
> ::: layout/base/ServoRestyleManager.cpp:774
> (Diff revision 2)
> > +  if ((aAttribute == nsGkAtoms::mozbrowser &&
> > +       aElement->IsAnyOfHTMLElements(nsGkAtoms::iframe, nsGkAtoms::frame)) ||
> > +      (aAttribute == nsGkAtoms::border &&
> > +       aElement->IsHTMLElement(nsGkAtoms::table))) {
> > +    // Add state for :-mozbrowser-frame and :-moz-table-border-nonzero.
> > +    snapshot.AddOtherPseudoClassState(aElement);
> 
> Do we really need to record this for `:-moz-table-border-nonzero`?
> 
> I'd expect we could just use the attributes to match
> `:-moz-table-border-nonzero` using the recorded attributes...

We could do it that way, if we factored out the work that CSSPseudoClasses::MatchesElement poking at the nsAttrValue.  It's an FFI call either way.
(Assignee)

Comment 16

8 months ago
(In reply to Emilio Cobos Álvarez [:emilio] from comment #14)
> This also needs `:lang`, but I'm guessing we'll take care of that one in
> another bug.

Yeah, I'm not sure whether we need to handle :lang in the same way here.  You can see when you review bug 1365162. :)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

8 months ago
Attachment #8874330 - Attachment is obsolete: true
(Assignee)

Updated

8 months ago
Attachment #8874331 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 23

8 months ago
mozreview-review
Comment on attachment 8874667 [details]
Bug 1365159 - Part 3: Bindings config update.

https://reviewboard.mozilla.org/r/146016/#review149922
Attachment #8874667 - Flags: review?(cam) → review+

Comment 24

8 months ago
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ce1f1e2027ef
Part 1: Record :-moz-browser-frame and :-moz-table-border-nonzero state on snapshots. r=emilio
https://hg.mozilla.org/integration/autoland/rev/5c9f9457ae2a
Part 2: Reftest. r=emilio
https://hg.mozilla.org/integration/autoland/rev/bb505b001f5f
Part 3: Bindings config update. r=heycam

Comment 25

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ce1f1e2027ef
https://hg.mozilla.org/mozilla-central/rev/5c9f9457ae2a
https://hg.mozilla.org/mozilla-central/rev/bb505b001f5f
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Updated

7 months ago
Blocks: 1365160
(Assignee)

Updated

7 months ago
Assignee: nobody → cam
You need to log in before you can comment on or make changes to this bug.