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

VERIFIED FIXED in Firefox 56

Status

()

P3
major
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: kasper93, Assigned: emilio)

Tracking

(Blocks: 1 bug)

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

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
Posted 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)
(Reporter)

Comment 1

2 years 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.
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)
Priority: -- → P3
(Assignee)

Comment 8

2 years 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

2 years 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

2 years 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

2 years 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

2 years ago
Attachment #8886854 - Attachment is obsolete: true
Duplicate of this bug: 1381680

Comment 15

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

Updated

2 years ago
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.