Closed Bug 1406254 Opened 7 years ago Closed 7 years ago

stylo: incorrect style when ::first-line is used on anchor

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Version:
53 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- disabled
firefox57 + fixed
firefox58 + fixed

People

(Reporter: bzbarsky, Assigned: jryans)

Details

Attachments

(2 files, 1 obsolete file)

Bug 1406254 - Visited reftest for ::first-line inheritance.
59 bytes, text/x-review-board-request
jryans
: review+
ritu
: approval-mozilla-beta+
Details
Bug 1406254 - Clear visited rules for text inheritance.
88 bytes, text/plain
jryans
: review+
ritu
: approval-mozilla-beta+
Details 
Consider this testcase:

  <!DOCTYPE html>
  <style>
    a { display: block; }
    a::first-line { color: green; }
  </style>
  <a href>Some text</a>

In Gecko, Safari, and Chrome the text is green.  In Stylo it's the default visited color.

jryans, any idea what's going on here?  This is specific to text; if I have a <span> inside the <a> it gets the right styles...
Flags: needinfo?(jryans)
Priority: -- → P2
I am away at a team meetup this week.  I plan to look into this on Monday.
Assignee: nobody → jryans
I can't seem to reproduce the issue here using the latest from autoland today.

I tried using the following data URI:

data:text/html,<!DOCTYPE html><style>a { display: block; }a::first-line { color: green; }</style><a href>Some text</a>

Stylo and Gecko modes appear identical for me, and they match other browsers as well (green text, link underline).

:bz, can you still reproduce?
Flags: needinfo?(jryans) → needinfo?(bzbarsky)
Ah, never mind, using a data URI changes the test apparently.  Saving the test case in comment 0 to a HTML file loaded via HTTP triggers the problem behavior for me.
Flags: needinfo?(bzbarsky)
Right, you need to have an actual visited link (<a href> is <a href=""> which is pointing to the same page), and we don't save data: URIs in history.
Not sure yet about the root cause here...  

I have verified that after the cascade on the Servo side, the color value is green for first line (both visited and unvisited).

Perhaps Gecko is reading the color value from the wrong style context for first line?  Or maybe the default visited color is overriding the author color in this case?

I'll keep investigating tomorrow.
Comment on attachment 8917568 [details]
Bug 1406254 - Clear visited rules for text inheritance.

Reviewing this from here because mozreview went down mid-review.

Looks good, with the comments below addressed:

So, I think it's safe enough to drop the argument and do the visited style builder and such in for_inheritance, dropping the `adjust_for_text` call.

This would need a comment saying that we rely on adjust_for_text and such not using visited-dependent properties, or flags that are read from the visited style, but that's the case already (and if it wasn't we'd already be in trouble).

Also, the patch would be slightly smaller, which is nice (and easier to uplift), and contain less boilerplate / duplicated code to care about when wanting to inherit styles from other.

Last, but not least, can you file a bug on our usage of `parent.flags` in that function and ni? me (or assign me directly)? Some of the flags aren't supposed to be "inherited", like INHERITS_RESET_STYLE / INHERITS_CONTENT. That's fishy, but it's not a problem right now because we only use them for element styles. It should get fixed though.
Attachment #8917568 - Flags: review?(emilio) → review+
Comment on attachment 8917569 [details]
Bug 1406254 - Visited reftest for ::first-line inheritance.

Thanks for fixing this Ryan!

00:59 <jryans> emilio: ok! thanks for the review :)
01:00 <jryans> emilio: (i assume separate test patch also looks good? not sure if you saw it before mozreview died...)
01:00 <emilio> jryans: no worries, nice catch! (I wasn't ccd on that bug, I could've helped out, maybe). I'm sorry I didn't got to the tests before mozreview went down :/
01:01 <jryans> emilio: it's fine, i learned more about reparenting and such with this one :)
01:01 <emilio> jryans: I can review them in the morning and land them if you want. Since they don't go through Servo code you can put the first patch on the servo queue meanwhile :P
01:02 <emilio> jryans: yeah, that stuff is nasty... :)
01:02 <jryans> emilio: here's the patch for tests https://github.com/jryans/gecko-dev/commit/76a8cc4eebff07563701552fb26a314d8fee7edb
01:04 <emilio> jryans: looks good. Maybe it'd be a good idea to try it with a nested visited link inside the first-line too, but feel free to not write that if you think it'd be too much churn.
01:04 <jryans> ok!
01:06 <emilio> jryans: oh, one question, is there something missing in `color-on-visited-text-1.html`? why does the `<span>` need an ID, did you intend to change its color?
01:07 <jryans> emilio: i just threw in the IDs while debugging, so it would be easier to identify them in some debug logs. can prune from the test, they aren't actually needed there.
01:08 <emilio> jryans: cool, r=me with that, will comment on the bug, thanks!
Attachment #8917569 - Flags: review?(emilio) → review+
(In reply to Emilio Cobos Álvarez [:emilio] from comment #9)
> Comment on attachment 8917568 [details]
> Bug 1406254 - Clear visited rules for text inheritance.
> 
> Reviewing this from here because mozreview went down mid-review.
> 
> Looks good, with the comments below addressed:
> 
> So, I think it's safe enough to drop the argument and do the visited style
> builder and such in for_inheritance, dropping the `adjust_for_text` call.
> 
> This would need a comment saying that we rely on adjust_for_text and such
> not using visited-dependent properties, or flags that are read from the
> visited style, but that's the case already (and if it wasn't we'd already be
> in trouble).
> 
> Also, the patch would be slightly smaller, which is nice (and easier to
> uplift), and contain less boilerplate / duplicated code to care about when
> wanting to inherit styles from other.

Sure, I'll go with that approach!  I wasn't sure if it was better allow the caller of `for_inheritance` to process unvisited and visited similarly.  But, you're right, the text adjustments don't actually affect visited properties, so there's no reason to do the extra work.

I've changed to this approach and added a comment about it.
 
> Last, but not least, can you file a bug on our usage of `parent.flags` in
> that function and ni? me (or assign me directly)? Some of the flags aren't
> supposed to be "inherited", like INHERITS_RESET_STYLE / INHERITS_CONTENT.
> That's fishy, but it's not a problem right now because we only use them for
> element styles. It should get fixed though.

Sure, I filed bug 1407832 and ni?ed you.
Attachment #8917568 - Flags: review?(emilio) → review+
Attachment #8917569 - Flags: review?(emilio) → review+
jryans, thank you!
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e921df5cd9b9
Visited reftest for ::first-line inheritance. r=emilio
https://hg.mozilla.org/mozilla-central/rev/e921df5cd9b9
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Approval Request Comment
[Feature/Bug causing the regression]: Stylo regression in ::first-line handling with visited links
[User impact if declined]: If declined, some visited links with ::first-line styles won't have the correct styles applied.
[Is this code covered by automated tests?]: Yes, this bug adds a test.
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: Only patches in this bug are needed.
[Is the change risky?]: No, I don't believe so.
[Why is the change risky/not risky?]: This only affects visited link style handling where selectors such as ::first-line are also used, which I would imagine is fairly rare (but have no data to be sure).  The change is also well contained.
[String changes made/needed]: None
Attachment #8918024 - Flags: review+
Attachment #8918024 - Flags: approval-mozilla-beta?
Comment on attachment 8917569 [details]
Bug 1406254 - Visited reftest for ::first-line inheritance.

Approval Request Comment

See comment 19.
Attachment #8917569 - Flags: approval-mozilla-beta?
Comment on attachment 8918024 [details]
Bug 1406254 - Clear visited rules for text inheritance.

Recent regression due to Stylo, Beta57+
Attachment #8918024 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8917569 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #19)
> [Is this code covered by automated tests?]: Yes, this bug adds a test.
> [Has the fix been verified in Nightly?]: Yes
> [Needs manual test from QE? If yes, steps to reproduce]: No

Setting qe-verify- based on jryans's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: