Closed Bug 1381276 Opened 3 years ago Closed 3 years ago

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

Categories

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

56 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: kasper93, Assigned: emilio)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Attached file 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)
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.
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.
Status: UNCONFIRMED → NEW
Has STR: --- → yes
Ever confirmed: true
Looks like :visited style is incorrectly applied here.

jryans, thought about this?
Flags: needinfo?(jryans)
Priority: -- → P3
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 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 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+
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.
Attachment #8886854 - Attachment is obsolete: true
Duplicate of this bug: 1381680
https://hg.mozilla.org/mozilla-central/rev/369d5bcd39e5
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Duplicate of this bug: 1380897
Assignee: nobody → emilio+bugs
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.