Support 'objects' value of text-decoration-skip. http://www.w3.org/TR/2013/CR-css-text-decor-3-20130801/#text-decoration-skip
Created attachment 224045 [details] Patch
<rdar://problem/15346113>
Comment on attachment 224045 [details] Patch Great!
Comment on attachment 224045 [details] Patch Clearing flags on attachment: 224045 Committed r164061: <http://trac.webkit.org/changeset/164061>
All reviewed patches have been landed. Closing bug.
Comment on attachment 224045 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=224045&action=review I know this is probably too late since the patch has already been submitted, but you should also have a test that does something like <u><img src="....."></u> compared to just the image alone to make sure it doesn't get underlined. That test should also test all the "atomic inline" objects that the spec describes > Source/WebCore/rendering/InlineTextBox.cpp:1173 > + // FIXME: Need to support text-decoration-skip: none. This is the wrong place for such a comment. InlineTextBoxes always get underlined; it's the other InlineBoxes that don't. > Source/WebCore/rendering/style/RenderStyle.h:1674 > + static TextDecorationSkip initialTextDecorationSkip() { return TextDecorationSkipObjects; } Should probably test this as well.
Comment on attachment 224045 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=224045&action=review >> Source/WebCore/rendering/style/RenderStyle.h:1674 >> + static TextDecorationSkip initialTextDecorationSkip() { return TextDecorationSkipObjects; } > > Should probably test this as well. You can probably do this by calling getComputedStyle on an unstyled element
Reopening to attach new patch.
Created attachment 224167 [details] Patch
Hi Myles, I know you are not reviewer. If this patch seems good, I'll ask some reviewers to review or rubber stamp this. (In reply to comment #6) > (From update of attachment 224045 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=224045&action=review > > I know this is probably too late since the patch has already been submitted, but you should also have a test that does something like <u><img src="....."></u> compared to just the image alone to make sure it doesn't get underlined. That test should also test all the "atomic inline" objects that the spec describes Added test case. I created the test to normal CSS test because this is default behavior even if text-decoration-skip: auto is implemented. > > Source/WebCore/rendering/InlineTextBox.cpp:1173 > > + // FIXME: Need to support text-decoration-skip: none. > > This is the wrong place for such a comment. InlineTextBoxes always get underlined; it's the other InlineBoxes that don't. Moved. > > Source/WebCore/rendering/style/RenderStyle.h:1674 > > + static TextDecorationSkip initialTextDecorationSkip() { return TextDecorationSkipObjects; } > > Should probably test this as well. I think "stylesheet.insertRule(".p { }", 0);" test case in text-decoration-skip-roundtrip.html covers this.
Comment on attachment 224045 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=224045&action=review r=me >>>> Source/WebCore/rendering/style/RenderStyle.h:1674 >>>> + static TextDecorationSkip initialTextDecorationSkip() { return TextDecorationSkipObjects; } >>> >>> Should probably test this as well. >> >> You can probably do this by calling getComputedStyle on an unstyled element > > I think "stylesheet.insertRule(".p { }", 0);" test case in text-decoration-skip-roundtrip.html covers this. I agree.
Comment on attachment 224167 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=224167&action=review > LayoutTests/fast/css/text-decoration-skip-atomic-inline.html:3 > +.underline { text-decoration: underline; color: red; } Don't you want to explicitly say text-decoration-skip: objects in case we change the default to 'auto'? > LayoutTests/fast/css/text-decoration-skip-atomic-inline.html:7 > +<div class="underline"><img class="margin-border-padding" src="resources/greenbox.png" /><table style="display: inline-table; "><td class="margin-border-padding">inline-table</table><div class="margin-border-padding" style="display: inline-block; ">inline-block</div><input class="margin-border-padding" type="button" value="button" /><video class="margin-border-padding" src="dummy.mp4"></video><canvas class="margin-border-padding" width="10" height="10"></canvas><iframe class="margin-border-padding"></iframe></div> Do these have to all be squished onto one line? I think it would be better for the test to show at least some underlined text so we can see if any underlining was done.
Created attachment 224462 [details] Patch
(In reply to comment #12) > (From update of attachment 224167 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=224167&action=review > > > LayoutTests/fast/css/text-decoration-skip-atomic-inline.html:3 > > +.underline { text-decoration: underline; color: red; } > > Don't you want to explicitly say text-decoration-skip: objects in case we change the default to 'auto'? Changed to text-decoration-skip: objects only test because the spec doesn't say that text-decoration-skip: ink should skip atomic inlines. Since there is no definition of 'auto', I assumed that 'auto' is equivalent to 'ink'. > > LayoutTests/fast/css/text-decoration-skip-atomic-inline.html:7 > > +<div class="underline"><img class="margin-border-padding" src="resources/greenbox.png" /><table style="display: inline-table; "><td class="margin-border-padding">inline-table</table><div class="margin-border-padding" style="display: inline-block; ">inline-block</div><input class="margin-border-padding" type="button" value="button" /><video class="margin-border-padding" src="dummy.mp4"></video><canvas class="margin-border-padding" width="10" height="10"></canvas><iframe class="margin-border-padding"></iframe></div> > > Do these have to all be squished onto one line? Split into 3 lines. > I think it would be better for the test to show at least some underlined text so we can see if any underlining was done. Made description underlined.
Comment on attachment 224462 [details] Patch Hi, Apologies that your patch was not reviewed in a timely manner. Since it's now quite old, I am removing it from the review request queue. Please consider rebasing it on trunk and resubmitting. To increase the chances of getting a review, consider using 'Tools/Scripts/webkit-patch upload --suggest-reviewers' to CC reviewers who might be interested in this bug.
From reading through the bug, it seems post-fixing this bug, some issues were identified and the patch was added to address those, which was not reviewed on time. Is this still required or needed? Appreciate if someone can comment and mark this bug accordingly. Thanks!
Yep, this bug is still needed.
Retitling to update to the new CSS property.