stylo: Wrong color of elements inside <a> tag.

VERIFIED FIXED in Firefox 56

Status

()

Core
CSS Parsing and Computation
P3
major
VERIFIED FIXED
9 days ago
3 days ago

People

(Reporter: kasper93, Assigned: emilio)

Tracking

(Blocks: 1 bug)

56 Branch
mozilla56
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

9 days ago
Created attachment 8886821 [details]
green.html

Hi, 

I've prepared very primitive example where stylo fails to render proper color. the bug is pretty obvious, just open the attachment ;) 

I've tested with layout.css.servo.enabled set to true with recent nightly build (20170715030206)
(Reporter)

Comment 1

9 days ago
Also forget to mention that if you hold left mouse button on the link, drag (to not actually open the link) and release mouse button text will remain red, while it should go back to purple. This also happens with stylo disabled (see the underline color). Chrome behaves correctly here.

Comment 2

8 days ago
Nice catch! Confirmed in Nightly 56 x64 20170715100214 @ Debian Testing (Linux 4.11.0-1-amd64, Radeon RX480). Without stylo, everything is correct, even though the underline is bigger in Chrome Dev 61.
Blocks: 1375906
Status: UNCONFIRMED → NEW
Has STR: --- → yes
Ever confirmed: true
Looks like :visited style is incorrectly applied here.

jryans, thought about this?
Flags: needinfo?(jryans)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Priority: -- → P3
(Assignee)

Comment 8

7 days ago
mozreview-review
Comment on attachment 8886854 [details]
Bug 1381276: Cascade the visited style with the normal rules if the element is not the relevant link.

https://reviewboard.mozilla.org/r/157600/#review162850

::: commit-message-96b4d:1
(Diff revision 2)
> +Bug 1381276: Cascade the visited style with the normal rules if the element is not the relevant link. r?jryans

Well, I guess the message should say "if we haven't found a relevant link", I'll change it locally.

Comment 9

7 days ago
mozreview-review
Comment on attachment 8886854 [details]
Bug 1381276: Cascade the visited style with the normal rules if the element is not the relevant link.

https://reviewboard.mozilla.org/r/157600/#review162944

Thanks, this looks good to me!

It appears to recreate the changes :bz had made[1] before your refactor.  I am a bit surprised no tests were failing without it... :S

[1]: https://github.com/servo/servo/pull/17570/commits/824139e615d778625f53da7ecaa939ebe5fac3a5#diff-bfe940d87417d0a9a19b8bd96319d3f8R154
Attachment #8886854 - Flags: review?(jryans) → review+

Comment 10

7 days ago
mozreview-review
Comment on attachment 8886855 [details]
Bug 1381276: Reftest.

https://reviewboard.mozilla.org/r/157602/#review162938

Thanks, the test looks good to me. :)

::: layout/reftests/css-visited/visited-inherit-1-ref.html:3
(Diff revision 2)
> +<!DOCTYPE html>
> +<style>
> +a { text-decoration: none; color: initial; display: block; margin: 2px; }

Are the font-size and text-decoration properties needed for the test?  If not, please remove them, so the test is more focused on what it wants to test.

::: layout/style/test/test_visited_reftests.html:88
(Diff revision 2)
>    //"== first-line-1.html first-line-1-ref.html",
>    "== white-to-transparent-1.html white-to-transparent-1-ref.html",
>    "== link-root-1.xhtml link-root-1-ref.xhtml",
>    "== mathml-links.html mathml-links-ref.html",
>    "== placeholder-1.html placeholder-1-ref.html",
> +  "== visited-inherit-1.html visited-inherit-1-ref.html",

Can you confirm this test reports a failure without your fix?  I want to make sure we're getting accurate reporting from these visited tests.
Attachment #8886855 - Flags: review?(jryans) → review+
Flags: needinfo?(jryans)
(Assignee)

Comment 11

7 days ago
mozreview-review-reply
Comment on attachment 8886855 [details]
Bug 1381276: Reftest.

https://reviewboard.mozilla.org/r/157602/#review162938

> Are the font-size and text-decoration properties needed for the test?  If not, please remove them, so the test is more focused on what it wants to test.

Other tests seem to have text-decoration: none too, presumably to avoid differences, since that's meaningless to what we want to test, I'll keep it. The font-size is indeed pointless.

> Can you confirm this test reports a failure without your fix?  I want to make sure we're getting accurate reporting from these visited tests.

As mentioned on IRC, I reverted my patch and the test failed.
Comment hidden (mozreview-request)
(Assignee)

Updated

7 days ago
Attachment #8886854 - Attachment is obsolete: true

Comment 13

7 days ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/369d5bcd39e5
Reftest. r=jryans

Updated

7 days ago
Duplicate of this bug: 1381680

Comment 15

6 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/369d5bcd39e5
Status: NEW → RESOLVED
Last Resolved: 6 days ago
status-firefox56: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(Assignee)

Updated

4 days ago
Duplicate of this bug: 1380897
Assignee: nobody → emilio+bugs

Comment 17

3 days ago
Verified fixed in bug 1380897 comment 10 (and now).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.