Closed Bug 1372056 Opened 6 years ago Closed 6 years ago

stylo: text-decoration propagation is broken.


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




Tracking Status
firefox56 --- fixed


(Reporter: emilio, Assigned: emilio)




(1 file)


<!doctype html>
  .test:hover { text-decoration: underline; }
<div class="test">
  Foo <span>@<b>bar</b></span>

Test-decoration should propagate to the <b> element, but doesn't.

This can be seen on twitter. I saw this while trying out my patches for bug 1368240, but also happens without them.

text-decoration is a somewhat special property aiui, because it's reset, but it propagates through the box tree. Boris can correct me here if I'm wrong.

I don't know the difference between dynamic text-decoration changes in Gecko and in Stylo to explain why it's broken in Stylo right now, tbh. We should generate pretty much the same output from styling... Perhaps the relevant style context bits get out of sync somehow?
There are a few conditions there we probably want to check... This is probably easier once we have bug 1368240 and bug 1368236 in tree.

There was a contributor that was working on the inlinization of ruby kids[1], which I recommended to implement adding a few bits to ComputedValues. Not sure it's worth to wait for that or not.

Note that both Blink and WebKit implement it as an inherited property, btw.
text-decoration properties are reset in the spec [1], and they propagate into inner elements rather than inheriting via style system, which enables: 1) nested decoration lines, 2) consecutive decoration line among all inline elements inside.

Blink and WebKit just haven't implemented the right thing.

(IIRC Blink has implemented nested decoration lines somehow?)

Affects twitter - P1, but let's get the dependencies sorted out first.
Depends on: 1368240, 1368236
Priority: -- → P1
nsStyleContext::SetStyleBits sets the NS_STYLE_HAS_TEXT_DECORATION_LINES flag in a way that depends on mParent.  Does that work correctly in stylo?
Er, never mind; comment 1 seems like a solid explanation.
Assignee: nobody → mbrubeck
Whoops, I forgot this was assigned and I submitted a patch[1] with the fix for this to unblock a contributor to fix[2]...

Matt, what's your progress with this? I can close my PR if you want. I also need to submit a test for this to this patch.

Flags: needinfo?(mbrubeck)
You can steal this and land your PR when ready.
Assignee: mbrubeck → emilio+bugs
Flags: needinfo?(mbrubeck)
Comment on attachment 8881528 [details]
Bug 1372056: Test for dynamic text-decoration propagation.
Attachment #8881528 - Flags: review?(xidorn+moz) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 93bd9f27420e -d 36dfc734a106: rebasing 404174:93bd9f27420e "Bug 1372056: Test for dynamic text-decoration propagation. r=xidorn" (tip)
merging layout/reftests/bugs/reftest.list
warning: conflicts while merging layout/reftests/bugs/reftest.list! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by
Test for dynamic text-decoration propagation. r=xidorn
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
This failed the lint in because dynamic is misspelled as dymanic.
Flags: needinfo?(emilio+bugs)
Pushed by
followup: Fix test name so the CSSWG lint passes. r=me
Flags: needinfo?(emilio+bugs)
You need to log in before you can comment on or make changes to this bug.