Closed Bug 1417781 Opened 2 years ago Closed 2 years ago

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

Categories

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

57 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- wontfix
firefox58 - verified
firefox59 --- verified

People

(Reporter: github, Assigned: emilio)

References

Details

(Keywords: regression)

Attachments

(2 files)

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.
Component: Untriaged → CSS Parsing and Computation
Product: Firefox → Core
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.
Status: UNCONFIRMED → RESOLVED
Closed: 2 years ago
Resolution: --- → INVALID
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.
Status: RESOLVED → UNCONFIRMED
Resolution: INVALID → ---
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.
Summary: Visited links are rendering purple, instead of color defined in stylesheet → Stylo: Hovering visited links doesn't use the color defined by site's sheet
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
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.
Flags: needinfo?(emilio)
Sure, I bet this is us messing the cache for ::-moz-text styles.
(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.
Oh, ok, nvm, just got my build. I got this. This is a regression from bug 1395227 I'm 99% sure.
Assignee: nobody → emilio
Blocks: 1395227
Flags: needinfo?(emilio)
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) {
Attachment #8930295 - Flags: review?(cam) → review+
(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)
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8c69e6ae8e4a
Consider style structs as not equal if visited styles on them are changed. r=heycam
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.
This doesn't sound severe enough for a dot release.  Please request uplift to beta once this has been verified in nightly.
https://hg.mozilla.org/mozilla-central/rev/8c69e6ae8e4a
Status: NEW → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
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
Attachment #8930295 - Flags: approval-mozilla-beta?
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+.
Attachment #8930295 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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
Flags: qe-verify+
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.