1417781 - Stylo: Hovering visited links doesn't use the color defined by site's sheet

Status: VERIFIED FIXED

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

Description

[Josh Duck]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20171112125346

Steps to reproduce:

1. Load the page https://amp.abc.net.au/article/9140250 
2. Click on a link "We asked what you thought of Mr Trudeau's sudden "no show". Read the comments below.".
3. Return to https://amp.abc.net.au/article/9140250
4. Hover over previously clicked link


Actual results:

The hovered link appears purple.


Expected results:

The hovered link should appear blue. 

The CSS inspector and computed properties are showing the link as being blue.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Thanks for filing!

The link in question includes the following markup:

<a class="d-1BJ2f d-1_uca d-2WvST" href="http://www.abc.net.au/news/2017-11-10/tpp-talks-stall-after-justin-trudeau-canada-fails-to-show-up/9140250" data-reactid="79">
<strong data-reactid="80"><!-- react-text: 81 -->We asked what you thought of Mr Trudeau's sudden "no show". Read the comments below.<!-- /react-text --></strong>
</a>

The only matching visited rule I could find is:

.d-1_uca:visited{color:#551a8b}

This is a purple color, and the links are correctly shown as purple.  (This is also the default visited link color in Firefox, so it's a bit redundant.)

So, this seems like a site issue with the CSS on the page.

As for the visited rules not appearing in the DevTools inspector, that's a known issue, see bug 713106.

Please re-open if I have misunderstood the issue.

Comment 2

[Josh Duck]

The error occurs itself when hovering over the link. There are two selectors of equal specificity:

Col 19168: .d-1_uca:visited{color:#551a8b}
Col 19326: .d-1_uca:active,.d-1_uca:hover,.d-lMqNe:active,.d-lMqNe:hover,.d-suSrc:active,.d-suSrc:hover{color:#2a7ab0;color:var(--link-colour)}

I would expect the .d-1_uca:hover style that comes later in the stylesheet to override the .d-1_uca:visited style that comes first. 

Sorry about the terrible example, I wasn't able to narrow the example down in Codepen.

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Ah okay, thanks for the update.  I think I missed the "hover" step in your original report, apologies!

It looks like this only happens when Stylo is enabled.

I'll try to see if I can create a reduced test case.

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1417781 - Reduced test case

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

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Emilio, maybe you can help here?  I am about to leave on PTO, so I don't think I'll have time to investigate this issue before then.

I have attached a reduced test case where the issue only happens with Stylo enabled.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Sure, I bet this is us messing the cache for ::-moz-text styles.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #6)
> Sure, I bet this is us messing the cache for ::-moz-text styles.

On hindsight this probably isn't the case... I'll take a look either way.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Oh, ok, nvm, just got my build. I got this. This is a regression from bug 1395227 I'm 99% sure.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1417781: Consider style structs as not equal if visited styles on them are changed.

This change should be also fine for the Gecko callers, but please double-check.

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

Comment 10

[Cameron McCormack (:heycam)]

Comment on attachment 8930295 [details]
Bug 1417781: Consider style structs as not equal if visited styles on them are changed.

https://reviewboard.mozilla.org/r/201430/#review206690

::: layout/style/nsStyleContext.cpp:312
(Diff revision 1)
> -  } else if (thisVis && !NS_IsHintSubset(nsChangeHint_RepaintFrame, hint)) {
> +  } else if (thisVis &&
> +             (IsServo() || !NS_IsHintSubset(nsChangeHint_RepaintFrame, hint))) {

Hmm, I'm curious why the old style system doesn't exhibit this bug as well, since it does have the same optimization of stopping restyling when we detect no style change.  (And we detect that in the same way, using aEqualStructs.)

Anyway, that makes me think we should just remove the RepaintFrame check part of the condition, and just make it

  } else if (thisVis) {

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8930295 [details]
Bug 1417781: Consider style structs as not equal if visited styles on them are changed.

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

Comment 12

[Emilio Cobos Álvarez (:emilio)]

(I tried to add a reftest for this, but I didn't manage to make it fail with my patch reverted because of the subtree restyle that happens when coloring the link... I'll try a bit more)

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8c69e6ae8e4a
Consider style structs as not equal if visited styles on them are changed. r=heycam

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Pretty sure we'll want to uplift to 58 at least.

[Tracking Requested - why for this release]:
Fixes incorrect rendering of some links using :visited styles.

Comment 15

[Julien Cristau [:jcristau]]

This doesn't sound severe enough for a dot release.  Please request uplift to beta once this has been verified in nightly.

Comment 16

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/8c69e6ae8e4a

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8930295 [details]
Bug 1417781: Consider style structs as not equal if visited styles on them are changed.

Approval Request Comment
[Feature/Bug causing the regression]: stylo
[User impact if declined]: wronly styled visited links if they contain nested elements
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: see comment 0
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not risky
[Why is the change risky/not risky?]: small well-isolated change.
[String changes made/needed]: none

Comment 18

[Gerry Chang [:gchang]]

Comment on attachment 8930295 [details]
Bug 1417781: Consider style structs as not equal if visited styles on them are changed.

Fix a stylo: Hovering visited links issue. Beta58+.

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/f9ad55279ee6

Comment 20

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

I have reproduced this issue using Firefox  59.0a1, (build ID:20171115220414) on Win 8.1 x64.
I can confirm this issue is fixed, I verified using Firefox 58.0b8 on Windows 8.1 x64, Ubuntu 14.04 x64 and Mac OS X 10.13.2

Comment 21

[roxleitan]

I have reproduced the issue using Firefox 59.0a1 (2017-11-15) on Windows 10 x64.
Verified fixed using latest Nightly 59.0a1 (2017-12-05) on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.12.