Closed Bug 1367923 Opened 7 years ago Closed 7 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: 7 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: