Closed Bug 1367923 Opened 8 years ago Closed 8 years ago

Stylo: Support link preshints (mapped attributes) on <body>

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

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

People

(Reporter: jryans, Assigned: jryans)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Bug 1367923 - Store Servo decls when link preshints change.
59 bytes, text/x-review-board-request
manishearth
: review+
Details
Bug 1367923 - Pull decls from Gecko for link preshints.
59 bytes, text/x-review-board-request
manishearth
: review+
Details
Bug 1367923 - Enable link attr tests for Stylo.
59 bytes, text/x-review-board-request
manishearth
: review+
Details
Setting `document.linkColor` or changing the body's link attribute seems to be ignored with Stylo. (There's also `alinkColor` / `aLink` and `vlinkColor` / `vLink`.) This appears to impact at least the following tests: layout/reftests/bugs/99850-1a.html layout/reftests/bugs/99850-1c.html layout/reftests/bugs/99850-1d.html
Also these too: layout/reftests/bugs/589615-1a.xhtml layout/reftests/bugs/589615-1b.html
Summary: Stylo: Need to restyle when `document.linkColor` / body link attr changes → Stylo: `document.linkColor` / body link attr colors not applied
In Gecko's style system, these link / vlink / alink attributes are applied in `HTMLBodyElement::MapAttributesIntoRule`[1]. While the rest of that method has been converted to work with Stylo, these link ones haven't been so far. They are a bit different, since they each create one-off style rules[2] that the Gecko style system manually matches[3] based on element state of a given link. :bz, do you think we could potentially unship support for link / vlink / alink attributes on the body in Gecko? HTML 4.01 lists them as deprecated. Looking at other browsers: Chrome 60: link attr ignored Safari 10.2: link attr initially ignored, but if you close the tab and restore it, they appear applied Edge 15: link attr applied [1]: http://searchfox.org/mozilla-central/rev/a14524a72de6f4ff738a5e784970f0730cea03d8/dom/html/HTMLBodyElement.cpp#318-345 [2]: http://searchfox.org/mozilla-central/rev/c195cc1698da2d3116fd8594db44614bb1304719/layout/style/nsHTMLStyleSheet.cpp#491 [3]: http://searchfox.org/mozilla-central/rev/c195cc1698da2d3116fd8594db44614bb1304719/layout/style/nsHTMLStyleSheet.cpp#307-329
Flags: needinfo?(bzbarsky)
> :bz, do you think we could potentially unship support No. > HTML 4.01 lists them as deprecated HTML 4.01 says many things, only some of which make sense. ;) Anyway, the relevant spec here at this point is https://html.spec.whatwg.org/multipage/rendering.html#the-page these bits: When a body element has a link attribute, its value is expected to be parsed using the rules for parsing a legacy color value, and if that does not return an error, the user agent is expected to treat the attribute as a presentational hint setting the 'color' property of any element in the Document matching the :link pseudo-class to the resulting color. and similar for vlink/alink. As far as I know this is all interoperably implemented by every single browser. > Chrome 60: link attr ignored That's not what I see at all. Testcase: <!DOCTYPE html> <body link="lime" vlink="pink"> <a href="http://ibetthisisnotahostnameyouvisited">Unvisited link</a> <a href="https://bugzilla.mozilla.org/show_bug.cgi?id=1367923">Visited link</a> </body> Those render as lime and pink respectively for me in Chrome 60 dev after I visit this bug page in it. > Safari 10.2: link attr initially ignored, but if you close the tab and restore it, they appear applied Again, not what I see (in Safari 10.1 or WebKit nightly). The above testcase works just fine there.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (work week until 5/26) (if a patch has no decent message, automatic r-) from comment #3) > > HTML 4.01 lists them as deprecated > > HTML 4.01 says many things, only some of which make sense. ;) Ah sorry, this is what I get for blindly trusting the MDN spec links... > and similar for vlink/alink. As far as I know this is all interoperably > implemented by every single browser. Yes, using your test case, it looks like the static attributes are well supported. I was loading layout/reftests/bugs/99850-1a.html as my test case, which changes the color dynamically via `document.linkColor`. So, I guess the static attribute is better supported than dynamic changes. In any case, looks like we'll need to port this for Stylo. Thanks for helping with the analysis. :)
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Summary: Stylo: `document.linkColor` / body link attr colors not applied → Stylo: Support link preshints (mapped attributes) on <body>
Comment on attachment 8873641 [details] Bug 1367923 - Store Servo decls when link preshints change. https://reviewboard.mozilla.org/r/145018/#review148984 ::: layout/style/ServoBindings.cpp:502 (Diff revision 1) > + nsHTMLStyleSheet* sheet = aElement->OwnerDoc()->GetAttributeStyleSheet(); > + if (!sheet) { > + return nullptr; > + } > + > + return reinterpret_cast<const RawServoDeclarationBlockStrong*> would be nice if we had a helper function for doing this conversion.
Attachment #8873641 - Flags: review?(manishearth) → review+
Comment on attachment 8873642 [details] Bug 1367923 - Pull decls from Gecko for link preshints. https://reviewboard.mozilla.org/r/145020/#review148988 ::: servo/components/style/gecko/wrapper.rs:1110 (Diff revision 1) > } > > + // Support for link, vlink, and alink presentation hints on <body> > + if self.is_link() { > + // Unvisited vs. visited styles are computed up-front based on the > + // visited mode (not the element's actual state). So I guess this is called twice per visit-able element, regardless of its state?
Attachment #8873642 - Flags: review?(manishearth) → review+
Comment on attachment 8873643 [details] Bug 1367923 - Enable link attr tests for Stylo. https://reviewboard.mozilla.org/r/145022/#review148990
Attachment #8873643 - Flags: review?(manishearth) → review+
Comment on attachment 8873641 [details] Bug 1367923 - Store Servo decls when link preshints change. https://reviewboard.mozilla.org/r/145018/#review148984 > would be nice if we had a helper function for doing this conversion. Good idea, I'll add one!
Comment on attachment 8873642 [details] Bug 1367923 - Pull decls from Gecko for link preshints. https://reviewboard.mozilla.org/r/145020/#review148988 > So I guess this is called twice per visit-able element, regardless of its state? Yes, that's right. If a relevant link is found, match and cascade is done a second time in visited mode to compute the visited styles.
Pushed by jryans@gmail.com: https://hg.mozilla.org/integration/autoland/rev/7b8fcdae113f Store Servo decls when link preshints change. r=Manishearth https://hg.mozilla.org/integration/autoland/rev/5092c00facfa Enable link attr tests for Stylo. r=Manishearth
https://hg.mozilla.org/mozilla-central/rev/7b8fcdae113f https://hg.mozilla.org/mozilla-central/rev/5092c00facfa
Status: ASSIGNED → RESOLVED
Closed: 8 years 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.

Attachment

General

Creator:
Created:
Updated:
Size: