Closed Bug 1372056 Opened 7 years ago Closed 7 years ago

stylo: text-decoration propagation is broken.

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(1 file)

Bug 1372056: Test for dynamic text-decoration propagation.
59 bytes, text/x-review-board-request
xidorn
: review+
Details 
Test-case:

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

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.

[1]: https://github.com/servo/servo/issues/16825
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?)


[1] https://drafts.csswg.org/css-text-decor-3/#text-decoration-line-property
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
Status: NEW → ASSIGNED
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.

[1]: https://github.com/servo/servo/pull/17525
[2]: https://github.com/servo/servo/issues/16825
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.

https://reviewboard.mozilla.org/r/152686/#review157810
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 ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/bb3d1acd76b9
Test for dynamic text-decoration propagation. r=xidorn
https://hg.mozilla.org/mozilla-central/rev/bb3d1acd76b9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
This failed the lint in https://github.com/w3c/web-platform-tests/pull/6357 because dynamic is misspelled as dymanic.
Flags: needinfo?(emilio+bugs)
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8da73ff3a19f
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.

Attachment

General

Creator:
Created:
Updated:
Size: