Closed Bug 1341714 Opened 7 years ago Closed 7 years ago

stylo: Need to support all the nsHTMLStyleSheet bits

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: bzbarsky, Assigned: manishearth)

References

Details

Attachments

(4 files, 2 obsolete files)

Bug 1341714 - Part 1: stylo: Add support for text-align match-parent and the <th> -moz-center-or-inherit behavior ;
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1341714 - Part 2: stylo: Add support for <table> color quirk ;
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1341714 - Part 3: stylo: Add support for <math> default language;
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1341714 - Part 4: stylo: Add support for xml:lang;
59 bytes, text/x-review-board-request
heycam
: review+
Details 
I'm not sure whether we handle all this already and bug 1341712 is the only issue this testcase hits:

  data:text/html,<!DOCTYPE html><table><tr><th>text</th></tr><tr><td>Long text

but chances are we don't handle it.
Boris, can you elaborate a bit more on what needs to be done here?
Flags: needinfo?(bzbarsky)
Priority: -- → P2
Gecko uses nsHTMLStyleSheet to implement various bits, by creating nsIStyleRule instances that are walked by nsHTMLStyleSheet::RulesMatching for certain nodes.  Specifically, these implement:

1) The "link", "alink", and "vlink" attributes on <body>.  These are mapped into style, but not for the body itself.  Instead, during attribute mapping for <body> they're mapped into HTMLColorRules hanging off nsHTMLStyleSheet that are then walked in RulesMatching.  I did just check: these don't work in stylo.  Or servo.  Note that this also needs to trigger restyles on visited/active state transitions for <a> elements, href attribute changes, etc.

2) The default text alignment behavior for <th>, which can't be expressed in parseable CSS.  That is, this is where NS_STYLE_TEXT_ALIGN_MOZ_CENTER_OR_INHERIT specified values of text-align come from.  See TableTHRule::MapRuleInfoInto.  Doesn't work in stylo.

3) Quirks mode tables have interesting color behavior.  See TableQuirkColorRule.  Specifically, if not otherwise specified they get the non-parseable specified value "NS_STYLE_COLOR_INHERIT_FROM_BODY", which then needs to be handled specially when determining computed style.  It does what it says on the tin: inherits the color from the body.  If there is no body, things are a bit complicated.  In practice it starts off as the default color, and after that is changes whenever we compute style for the body, and hence can be stale if the body is removed.  For quirks mode documents the weirdness around the no-body case doesn't obviously matter...  I just checked, and this is broken in both stylo and servo.

4) The "content style rules" of the element.  This is SVG mapped attributes, HTML mapped attributes, HTMLBodyElement::WalkContentStyleRules, HTMLTableCellElement::WalkContentStyleRules, nsXULElement::WalkContentStyleRules.  I think we have these either implemented or with existing bugs covering them.  Again, this includes triggering a restyle when these things change.  Which, by the way, I'm not sure we did right for SVG.  We need to check.  :(

5) Something about the "xml:lang" attribute and how it maps into CSS lang stuff.  I thought we had a bug on this, but I don't see on right now.  Xidorn may know what the state of that is.

6) Some MathML bits, again dealing with lang.
Flags: needinfo?(bzbarsky)
Thanks for the writeup boris. Given that bug 1341647 and bug 1341648 (which are a subset of the issues described above) are already in Manish's bucket, and given that he has experience with the rule mapping stuff, I think it makes sense for him to take it.

Manish, feel free to split this out into sub-bugs or do it all in this bug, whatever's easier.
Assignee: nobody → manishearth
Priority: P2 → P1
Status: NEW → ASSIGNED
I'm working on this next. Marked bug 1341647 and bug 1341648 as dupes of this since they deal with the extra content style rules; I'll do all of them at once.

The visited stuff I think is going to be discussed in taipei, along with bug 1328509.


nsXULElement::WalkContentStyleRules seems to not do anything?
> nsXULElement::WalkContentStyleRules seems to not do anything?

Indeed.  So nothing to do there.  I wonder why it exists.  Maybe a leftover from when nsXULElement had a totally different implementation from all other elements....  We should just kill it off.  ;)
Added patch for the <td> stuff and <body>. Seems to work. Working on the the other bits now.


I removed the TABLE_ATTRS_DIRTY optimization (added in bug 211636). We construct nsMappedAttributes all the time, it's not expensive (may have been 7 years ago). Without this change we can't use the existing lazy resolution framework for handling mapped attributes and would have to add a second pass for these elements.

<table> elements without <td> or <th> are where we lose out by early computing (also cases where there's a lot of flipping of `cellpadding`), and those should be rare.

For <body>, I cached an extra PDB on the body element. Unlike the mapped attrs, this is not lazily computed -- I figured that since there's usually just one of these it's fine to eagerly compute it instead of creating another caching mechanism. The nsFrameLoader stuff means that changing the margin on the frame will only affect the first <body> element, if the document has multiple bodies.
> <table> elements without <td> or <th> are where we lose out by early computing

That's pretty rare, yeah.

> (also cases where there's a lot of flipping of `cellpadding`

Or just cases when a <table> is repeatedly removed from and inserted into the DOM.  But I guess recreating mapped attributes on BindToTree is the normal thing to do...

I might make more sense to do it on adopt; worth filing a bug on that.  The current behavior where mapped attrs don't affect style unless you're in the doc is kind of strange.

> changing the margin on the frame will only affect the first <body> element,
> if the document has multiple bodies.

That doesn't match either the spec or Gecko's current behavior (which match).  Or Chrome's current behavior (which matches Gecko and the spec).  Or Safari's which doesn't update either the first or any other <body>.  I haven't checked Edge...
heycam thinks you should review.

I will be adding more patches as I go through the remaining bits, but these first two are ready for review (and should be largely independent from the next ones, aside from probably sharing the Gecko_GetExtraContentStyleRules function).
Attachment #8851770 - Flags: review?(cam)
Attachment #8851836 - Flags: review?(cam)
Flags: needinfo?(bzbarsky)
Depends on: 1341648
Depends on: 1341647
Attachment #8851770 - Attachment is obsolete: true
Attachment #8852275 - Attachment is obsolete: true
Attachment #8851836 - Flags: review?(cam)
Stuff needing bz's review split out into bug 1341648 and bug 1341647, what's left is mostly pure servo-side changes.

More patches may be added. r?heycam
Flags: needinfo?(bzbarsky)
We have a test failure on layout/reftests/w3c-css/submitted/text3/text-align-match-parent-root-rtl.html caused by this.

The gist of it is that `<html dir=rtl style="text-align:match-parent">...` should have right-aligned content. match-parent is working in general, but not on the root element.


I'm poking into this and I'm not sure why this works in Gecko, either. When cascading, we do set the initial value of `direction` to be `aContext->GetBidi()`'s bidi-ness, and the nsStyleVisibility constructor does this too, but GetBidi() doesn't seem to have a dependence on the dir of the html element.

In stylo, we already have a dependence on GetBidi() in nsStyleVisibility's constructor (though it's different from the one for `direction: initial` -- `direction: initial` will use `GET_BIDI_OPTION_DIRECTION(presContext->GetBidi()) == IBMBIDI_TEXTDIRECTION_RTL` whereas the constructor uses `presContext->GetBidi() == IBMBIDI_TEXTDIRECTION_RTL`, which seems wrong -- but fixing it does not improve things since in Stylo for the given document `GetBidi()` is still LTR).


So I'm not sure what's missing here. Somehow, gecko gets the initial value for `direction` of that document as RTL (though I can't see where), but Stylo does not, despite it doing basically the same thing here.


dholbert said that jfkthame might have ideas. Thoughts?

I think that the above patch can be landed independently of that bug, however, since at its core it's a bug in how `direction` is cascaded, not text-align (just happens to affect text-align)
Flags: needinfo?(jfkthame)
More curiousness: `direction: initial` sets to LTR on a document with `dir=rtl`.

I am even more confused as to why that match-parent test works in gecko.
Ah, there's an if(parent) there which I missed -- the whole code is skipped. I didn't realize parent was the parent /element/, not style.

We have an is_root_element flag which I can use here.
Flags: needinfo?(jfkthame)
The <table> color quirk isn't quite working yet because the body color is not being set. The gecko code doing this is weird, trying to copy it over.
The <table> quirk works now, but fails layout/reftests/bugs/655549-1.html , because the body style is not updated. I'll have to look in to why GetContext() isn't called in that case and where else this code needs to be kept.
Comment on attachment 8851836 [details]
Bug 1341714 - Part 1: stylo: Add support for text-align match-parent and the <th> -moz-center-or-inherit behavior ;

https://reviewboard.mozilla.org/r/124038/#review127538

::: servo/components/style/properties/longhand/inherited_text.mako.rs:305
(Diff revision 9)
> +    ${helpers.gecko_keyword_conversion(Keyword('text-align',
> +                                               """left right center justify -moz-left -moz-right
> +                                                -moz-center char end""",
> +                                                gecko_strip_moz_prefix=False), type="T")}

Nit: Indent this one level.

::: servo/components/style/properties/longhand/inherited_text.mako.rs:327
(Diff revision 9)
> +            Keyword(computed_value::T),
> +            MatchParent,
> +            MozCenterOrInherit,
> +        }
> +        pub fn parse(_context: &ParserContext, input: &mut Parser) -> Result<SpecifiedValue, ()> {
> +            if let Ok(key) = input.try(computed_value::T::parse) {

Maybe add a comment in here saying that MozCenterOrInherit is value that can't be parsed, but can be set by presentation attributes.

::: servo/components/style/properties/longhand/inherited_text.mako.rs:339
(Diff revision 9)
> +        impl ToCss for SpecifiedValue {
> +            fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
> +                match *self {
> +                    SpecifiedValue::Keyword(key) => key.to_css(dest),
> +                    SpecifiedValue::MatchParent => dest.write_str("match-parent"),
> +                    SpecifiedValue::MozCenterOrInherit => Ok(()),

It might be useful to serialize this anyway, in case we add a way to dump the contents/results of presentation attribute mapping for debugging.

::: servo/components/style/properties/longhand/inherited_text.mako.rs:362
(Diff revision 9)
> +                        // on the root <html> element we should still respect the dir
> +                        // but the parent dir of that element is LTR even if it's <html dir=rtl>
> +                        // and will only be RTL if certain prefs have been set.
> +                        // In that case, the default behavior here will set it to left,
> +                        // but we want to set it to right -- instead set it to `start`, which
> +                        // will do the right thing in this case (but not the general case)
> +                        if context.is_root_element {
> +                            return computed_value::T::start;
> +                        }

Unfortunately I got confused by this comment. :-)

As I understand it from reading nsRuleNode::ComputeVisibilityData, if we have a match-parent value, and no parent styles to inherit from, then we leave mTextAlign set to the value it got during the nsStyleText constuctor, which is the LTR or RTL, based on nsPresContext::GetBidi().

Does that mean that the behaviour here is wrong in the case that the pres context and <html dir> differ, because the start value will be resolved against the <html> element's direction property during layout?

Is the a reason we can't/shouldn't just say copy the value from the initial Visibility struct in this case?
Comment on attachment 8852732 [details]
Bug 1341714 - Part 2: stylo: Add support for <table> color quirk ;

https://reviewboard.mozilla.org/r/124890/#review127564

::: servo/components/style/values/specified/color.rs:94
(Diff revision 2)
>                  Color::MozDefaultColor => dest.write_str("-moz-default-color"),
>                  Color::MozDefaultBackgroundColor => dest.write_str("-moz-default-background-color"),
>                  Color::MozHyperlinktext => dest.write_str("-moz-hyperlinktext"),
>                  Color::MozActiveHyperlinktext => dest.write_str("-moz-activehyperlinktext"),
>                  Color::MozVisitedHyperlinktext => dest.write_str("-moz-visitedhyperlinktext"),
> +                Color::InheritFromBodyQuirk => Ok(()),

And maybe here too.  Not important, though.
Attachment #8852732 - Flags: review?(cam) → review+
Comment on attachment 8852732 [details]
Bug 1341714 - Part 2: stylo: Add support for <table> color quirk ;

https://reviewboard.mozilla.org/r/124890/#review127564

> And maybe here too.  Not important, though.

We've been pretty consistent about not serializing to things that can't be specified, e.g. with the system font stuff and other things. I'd rather not serialize this.
Comment on attachment 8851836 [details]
Bug 1341714 - Part 1: stylo: Add support for text-align match-parent and the <th> -moz-center-or-inherit behavior ;

https://reviewboard.mozilla.org/r/124038/#review127538

> Unfortunately I got confused by this comment. :-)
> 
> As I understand it from reading nsRuleNode::ComputeVisibilityData, if we have a match-parent value, and no parent styles to inherit from, then we leave mTextAlign set to the value it got during the nsStyleText constuctor, which is the LTR or RTL, based on nsPresContext::GetBidi().
> 
> Does that mean that the behaviour here is wrong in the case that the pres context and <html dir> differ, because the start value will be resolved against the <html> element's direction property during layout?
> 
> Is the a reason we can't/shouldn't just say copy the value from the initial Visibility struct in this case?

> mTextAlign set to the value it got during the nsStyleText constuctor, which is the LTR or RTL, based on nsPresContext::GetBidi().

... _almost_ right :)

`mTextAlign` has a default value of start. `mDirection` is what has a bidi-dependent initial value.

Yes, we could copy it from the init struct. Or call `initial_value()` here.
Just coming back to the original list of bits to note the status:

> 1) The "link", "alink", and "vlink" attributes on <body>.

bug 1328509, and will be discussed in taipei

> 2) The default text alignment behavior for <th>
> ...
> 3) Quirks mode tables have interesting color behavior. 

Fixed in this bug

> This is SVG mapped attributes


bug 1329093 (fixed)

> HTML mapped attributes,

bug 1330041 (fixed)


> HTMLBodyElement::WalkContentStyleRules

bug 1341647 (has patches, in review process)

> HTMLTableCellElement::WalkContentStyleRules

bug 1341648 (has patches, in review process)


> nsXULElement::WalkContentStyleRules. 

> Which, by the way, I'm not sure we did right for SVG.

we didn't (in fact, we got it wrong for all pres attrs), but bug 1341647 fixes it on the side

> 5) Something about the "xml:lang" attribute and how it maps into CSS lang stuff. 
> 
> 6) Some MathML bits, again dealing with lang.

I thought xidorn was removing this stuff, but this is a different thing. Apparently mathml gets a default `lang` value of `x_math`, and `xml:lang` overrides regular `lang`. This should be easy to support in this same bug using the same tricks. Will do.
Fixed mathml. Relevant tests may not pass because of bug 1352275 and bug 1352277.


xml:lang is going to be ... fun :) xml:lang is really just a mapped attribute that overrides `lang`, but it's not included in the nsMappedAttributes framework (which seems to only handle HTML mapped attributes). The straightforward solution here is to simply check if there's an xml:lang attr on the element, and if so push a pres hint *after* we push the regular pres hints (so that it has priority). This does mean repeatedly synthesizing PDBs (we cache every other preshint PDB on the stylesheet or in a lazy static), but I suspect xml:lang on HTML is rare enough that it doesn't matter? It also means that synthesize_presentational_blah now needs to query attributes on the element, which might be expensive (I'm not sure). I'll write up this solution unless others have objections. The only alternative I can think of is making the mapped attr framework capable of handling just this one XML namespace attribute. Doable.

There's also a bug where the body base color isn't updated when dynamically set. Apparently we can't do what gecko does here; bholley suggested instead just walking up the tree till a <body> is found. This will change behavior for malformed documents containing multiple bodies or elements outside of bodies, but that probably doesn't matter.
(In reply to Manish Goregaokar [:manishearth] from comment #42)
> There's also a bug where the body base color isn't updated when dynamically
> set. Apparently we can't do what gecko does here; bholley suggested instead
> just walking up the tree till a <body> is found. This will change behavior
> for malformed documents containing multiple bodies or elements outside of
> bodies, but that probably doesn't matter.

I think we can be precise without much effort. We can just add a Gecko_IsBodyElement predicate, or just duplicate the logic of nsDocument::GetBodyElement.
> bug 1328509, and will be discussed in taipei

I see no mention of these attributes in that bug.  Please file a separate bug for them, depending on bug 1328509; otherwise they will just slip through the cracks.

> There's also a bug where the body base color isn't updated when dynamically set

What do you mean by "base color"?

> Apparently we can't do what gecko does here

Why not?

> This will change behavior for malformed documents containing multiple bodies or elements outside of bodies,
> but that probably doesn't matter.

You mean apart from being a spec violation?
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #45)
> > bug 1328509, and will be discussed in taipei
> 
> I see no mention of these attributes in that bug.  Please file a separate
> bug for them, depending on bug 1328509; otherwise they will just slip
> through the cracks.
> 
> > There's also a bug where the body base color isn't updated when dynamically set
> 
> What do you mean by "base color"?

The color of the body, which ends up being used as the inherited color for tables in quirks mode.

> 
> > Apparently we can't do what gecko does here
> 
> Why not?

Because gecko caches the current color on the prescontext in GetContext, which is guaranteed to happen before any table style structs are computed in gecko, but generally happens long after table style structs are computed in servo (since they're computed eagerly, during the traversal).

> 
> > This will change behavior for malformed documents containing multiple bodies or elements outside of bodies,
> > but that probably doesn't matter.
> 
> You mean apart from being a spec violation?

As I mentioned in comment 44, I think we can do the right thing here.
> You mean apart from being a spec violation?

Oh, no, quite the opposite, if anything we would be more correct than Gecko here. I'm not 100% aware of how Gecko does this calculation, but it feels like in a document with multiple bodies you'd have issues with the color sometimes being picked off the wrong body (whichever was restyled the most recently). With the proposed solution for servo we'd be more consistent and correct.
Fixed the color thing.
Added support for xml:lang. This will query the attributes each time looking for xml:lang. It feels expensive to me, but Gecko does this too -- Gecko will query the attributes each time nsHTMLStyleSheet::RulesMatching(elem) is called. Gecko does cache lang rules in some sort of hashtable -- this means that it doesn't have to clone the string each time. We could do something similar with the PropertyDeclarationBlock if we wish. I'm not sure if any of this is performance-sensitive enough to matter, the way we do things may mean that this is completely unnecessary.

Kind of feel like it would be nicer if we just let the mapped attr functionality deal with xml:lang; the only issue here is the namespace.
Comment on attachment 8851836 [details]
Bug 1341714 - Part 1: stylo: Add support for text-align match-parent and the <th> -moz-center-or-inherit behavior ;

https://reviewboard.mozilla.org/r/124038/#review128384

::: servo/components/style/properties/longhand/inherited_text.mako.rs:363
(Diff revisions 9 - 10)
>                          // on the root <html> element we should still respect the dir
>                          // but the parent dir of that element is LTR even if it's <html dir=rtl>
>                          // and will only be RTL if certain prefs have been set.
>                          // In that case, the default behavior here will set it to left,
>                          // but we want to set it to right -- instead set it to `start`, which
>                          // will do the right thing in this case (but not the general case)

This comment needs updating now that we're not returning "start".
Attachment #8851836 - Flags: review?(cam) → review+
Comment on attachment 8853193 [details]
Bug 1341714 - Part 3: stylo: Add support for <math> default language;

https://reviewboard.mozilla.org/r/125276/#review128390
Attachment #8853193 - Flags: review?(cam) → review+
Comment on attachment 8853648 [details]
Bug 1341714 - Part 4: stylo: Add support for xml:lang;

https://reviewboard.mozilla.org/r/125744/#review128392

Gecko seems to make the |-x-lang: x-math| rule take precedence over one mapped by xml:lang="".  Seems like we should do the same, unless there's a specific reason to?

::: layout/style/ServoBindings.h:163
(Diff revision 1)
>  bool Gecko_MatchesElement(mozilla::CSSPseudoClassType type, RawGeckoElementBorrowed element);
>  nsIAtom* Gecko_LocalName(RawGeckoElementBorrowed element);
>  nsIAtom* Gecko_Namespace(RawGeckoElementBorrowed element);
>  nsIAtom* Gecko_GetElementId(RawGeckoElementBorrowed element);
>  
> +nsIAtom* Gecko_GetXMLLangRule(RawGeckoElementBorrowed element);

"Rule" sounds like the wrong word here.  "Value" or "AttrValue"?
Updated.
Comment on attachment 8853648 [details]
Bug 1341714 - Part 4: stylo: Add support for xml:lang;

https://reviewboard.mozilla.org/r/125744/#review129196
Attachment #8853648 - Flags: review?(cam) → review+
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7f2767f80f13
Part 1: stylo: Add support for text-align match-parent and the <th> -moz-center-or-inherit behavior ; r=heycam
https://hg.mozilla.org/integration/autoland/rev/083b02023bc2
Part 2: stylo: Add support for <table> color quirk ; r=heycam
https://hg.mozilla.org/integration/autoland/rev/6e5447e93e85
Part 3: stylo: Add support for <math> default language; r=heycam
https://hg.mozilla.org/integration/autoland/rev/5175ea846fe0
Part 4: stylo: Add support for xml:lang; r=heycam
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: