Closed Bug 1365159 Opened 3 years ago Closed 2 years ago

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

Categories

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

53 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: bzbarsky, Assigned: heycam)

References

Details

Attachments

(4 files, 2 obsolete files)

Attached file 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.
Priority: -- → P2
This is similar to bug 1365160.
Attachment #8874330 - Flags: review?(emilio+bugs)
Attachment #8874331 - Flags: review?(emilio+bugs)
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 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 on attachment 8874329 [details]
Bug 1365159 - Part 2: Reftest.

https://reviewboard.mozilla.org/r/145678/#review149670
Attachment #8874329 - Flags: review?(emilio+bugs) → 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 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+
(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.
(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. :)
Attachment #8874330 - Attachment is obsolete: true
Attachment #8874331 - Attachment is obsolete: true
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+
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
Blocks: 1365160
Assignee: nobody → cam
You need to log in before you can comment on or make changes to this bug.