1472567 - Logical border colors don't work in visited links with Stylo

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Oriol Brufau [:Oriol]]

Attachment: testcase.htm

I have this CSS

  a {
    border: 100px solid red;
    border-block-start-color: green;
  }

If the link is visited, then the top border is red and not green. Otherwise it's green as expected.

In the attached testcase, you should only see green and no red.

This seems some confusion with :visited magic, and thus getComputedValue says green but it's rendered red.

Regression-window: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5f721a664bf64fed99184a866b60c24a6afcb3a0&tochange=62cebea1d1578461a423a9ca7848706455db9ea5

Comment 1

[Emilio Cobos Álvarez [:emilio]]

getComputedStyle never honors :visited, fwiw.

Comment 2

[Emilio Cobos Álvarez [:emilio]]

Maybe you're interested in submitting a patch for this? Should be a matter of either adding them to:

  https://searchfox.org/mozilla-central/rev/ed2763bea882619ccb48b0aecc54e523d2bdd2ae/servo/components/style/properties/properties.mako.rs#1023

Or (maybe a bit better) do the visited check after physicalizing the property, that is, moving this line:

  https://searchfox.org/mozilla-central/rev/ed2763bea882619ccb48b0aecc54e523d2bdd2ae/servo/components/style/properties/properties.mako.rs#3771

After this one:

  https://searchfox.org/mozilla-central/rev/ed2763bea882619ccb48b0aecc54e523d2bdd2ae/servo/components/style/properties/properties.mako.rs#3790

Comment 3

[Oriol Brufau [:Oriol]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #1)
> getComputedStyle never honors :visited, fwiw.

Yes, but the testcase doesn't really :visited, so it seems the :visited logic is wrongly used here.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #2)
> Maybe you're interested in submitting a patch for this?

I can take a look, but I don't know rust (yet).

> Should be a matter of either adding them to:
> 
>  https://searchfox.org/mozilla-central/rev/ed2763bea882619ccb48b0aecc54e523d2bdd2ae/servo/components/style/properties/properties.mako.rs#1023
> 
> Or (maybe a bit better) do the visited check after physicalizing the
> property, that is, moving this line:
> 
>  https://searchfox.org/mozilla-central/rev/ed2763bea882619ccb48b0aecc54e523d2bdd2ae/servo/components/style/properties/properties.mako.rs#3771
> 
> After this one:
> 
>  https://searchfox.org/mozilla-central/rev/ed2763bea882619ccb48b0aecc54e523d2bdd2ae/servo/components/style/properties/ properties.mako.rs#3790

But this would allow logical border colors to be used in :visited rules, wouldn't it?
Well, probably that's what should happen, but I would like some confirmation in https://github.com/w3c/csswg-drafts/issues/2844

Comment 4

[Emilio Cobos Álvarez [:emilio]]

(In reply to Oriol Brufau [:Oriol] from comment #3)
> (In reply to Emilio Cobos Álvarez (:emilio) from comment #1)
> > getComputedStyle never honors :visited, fwiw.
> 
> Yes, but the testcase doesn't really :visited, so it seems the :visited
> logic is wrongly used here.

No, I think that's working as expected (given the bug here). getComputedStyle always returns the style as if the link was unvisited, which means that the computed value is 'green'.

Now the issue is that the link is :visited, and that rule gets ignored in :visited (due to this bug) so the link is actually red.

You always need to compute a visited style, because there's a global :visited rule. And even if the selector doesn't specify :visited, it affects the visited style (because `a` matches both visited and unvisited links).

> (In reply to Emilio Cobos Álvarez (:emilio) from comment #2)
> > Maybe you're interested in submitting a patch for this?
> 
> I can take a look, but I don't know rust (yet).
> 
> > Should be a matter of either adding them to:
> > 
> >  https://searchfox.org/mozilla-central/rev/ed2763bea882619ccb48b0aecc54e523d2bdd2ae/servo/components/style/properties/properties.mako.rs#1023
> > 
> > Or (maybe a bit better) do the visited check after physicalizing the
> > property, that is, moving this line:
> > 
> >  https://searchfox.org/mozilla-central/rev/ed2763bea882619ccb48b0aecc54e523d2bdd2ae/servo/components/style/properties/properties.mako.rs#3771
> > 
> > After this one:
> > 
> >  https://searchfox.org/mozilla-central/rev/ed2763bea882619ccb48b0aecc54e523d2bdd2ae/servo/components/style/properties/ properties.mako.rs#3790
> 
> But this would allow logical border colors to be used in :visited rules,
> wouldn't it?
> Well, probably that's what should happen, but I would like some confirmation
> in https://github.com/w3c/csswg-drafts/issues/2844

Yes. Otherwise I'd say this is working as expected. If Blink shows green and doesn't allow logical border colors in :visited, then it's a bug in their :visited logic.

When computing the :visited style, you get that the border-color is red (because if you match that link as visited it still matches that rule, but the logical color is disabled since you're matching as :visited).

Since the visited color is red, and the link is actually visited, that's the color we use.

Comment 5

[Oriol Brufau [:Oriol]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #2)
> Should be a matter of either adding them to:
> Or (maybe a bit better) do the visited check after physicalizing the property

Both approaches worked well, thanks.

In my patch for Chromium (https://chromium-review.googlesource.com/c/chromium/src/+/1126383) I planned to add WPT tests, but Edge does not consider <a href=""></a> to be visited, and if I use link.href=link.href, the problem is with Chromium's content_shell.
So the test will be specific to Chromium, and I guess I will do something similar for Firefox (not exactly the same because some logical shorthands are not supported yet).

Comment 6

[Emilio Cobos Álvarez [:emilio]]

You can probably check out the tests added in bug 1326189 for an example, and for where the rest of the tests for this live. Border colors should be easier to reftest than caret-color even :)

Comment 7

[Oriol Brufau [:Oriol]]

Attachment: Bug 1472567 - Honor logical border colors in visited links

Review commit: https://reviewboard.mozilla.org/r/255310/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255310/

Comment 8

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8990290 [details]
Bug 1472567 - Honor logical border colors in visited links

https://reviewboard.mozilla.org/r/255310/#review262366

Looks great, thanks!

The comments are really minor, so I'd be fine landing as-is if you don't want to address them.

I think you have push access? But if not let me know and I'll push this for you.

Thanks again for fixing this!

::: layout/reftests/css-visited/logical-box-border-color-visited-link-001.html:32
(Diff revision 1)
> +  border-inline-start: 0 none green;
> +  border-inline-end: 0 none green;
> +}
> +</style>
> +<p>Test passes if there is a filled green square and <strong>no red</strong>.</p>
> +<a id="link1"></a><a id="link2"></a><br />

nit: No need to close the `<br>`.

::: layout/reftests/css-visited/logical-box-border-color-visited-link-001.html:36
(Diff revision 1)
> +<p>Test passes if there is a filled green square and <strong>no red</strong>.</p>
> +<a id="link1"></a><a id="link2"></a><br />
> +<a id="link3"></a><a id="link4"></a>
> +<script>
> +document.querySelectorAll("a").forEach(function(link) {
> +  link.href = "visited-page.html";

Maybe putting the href explicitly and avoiding the script would make the tests clearer? I don't think there are other tests that do this. No big deal in any case.

::: layout/reftests/css-visited/logical-box-border-color-visited-link-ref.html:3
(Diff revision 1)
> +<!DOCTYPE html>
> +<html>
> +<meta charset="utf-8" />

nit: no need for `/>`, nor for `<html>` / `<html>`, here and in the other tests.

Comment 9

[Oriol Brufau [:Oriol]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #8)
> I think you have push access? But if not let me know and I'll push this for
> you.

I can push to try, not to mozilla-central.
But I don't know which kind of tests are useful for this.

> nit: no need for `/>`, nor for `<html>` / `<html>`, here and in the other
> tests.

I usually prefer to use HTML which is also valid XHTML, but OK.

Comment 10

[Oriol Brufau [:Oriol]]

Comment on attachment 8990290 [details]
Bug 1472567 - Honor logical border colors in visited links

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255310/diff/1-2/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fc9fd41ca2d3
Honor logical border colors in visited links r=emilio

Comment 12

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/fc9fd41ca2d3

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you're comfortable doing so.

Comment 14

[Emilio Cobos Álvarez [:emilio]]

Why would this be uplifted?

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Per our IRC discussion, wontfixing this for 62. It's not clear that this was really a new regression with Stylo and the newly-added blocker bug makes this too risky to consider for uplift.