Bug 128723 – [css3-text] Support text-decoration-skip-self property

• Status: REOPENED
• Component: Layout and Rendering

[Yuki Sekiguchi]

Support 'objects' value of text-decoration-skip.
http://www.w3.org/TR/2013/CR-css-text-decor-3-20130801/#text-decoration-skip

[Yuki Sekiguchi]

Created attachment 224045 [details]
Patch

[Myles C. Maxfield]

<rdar://problem/15346113>

[Dean Jackson]

Comment on attachment 224045 [details]
Patch

Great!

[WebKit Commit Bot]

Comment on attachment 224045 [details]
Patch

Clearing flags on attachment: 224045

Committed r164061: <http://trac.webkit.org/changeset/164061>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Myles C. Maxfield]

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.

[Myles C. Maxfield]

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

[Yuki Sekiguchi]

Reopening to attach new patch.

[Yuki Sekiguchi]

Created attachment 224167 [details]
Patch

[Yuki Sekiguchi]

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.

[Myles C. Maxfield]

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.

[Simon Fraser (smfr)]

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.

[Yuki Sekiguchi]

Created attachment 224462 [details]
Patch

[Yuki Sekiguchi]

(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.

[Michael Catanzaro]

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.

[Ahmad Saleem]

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!

[Myles C. Maxfield]

Yep, this bug is still needed.

[Myles C. Maxfield]

Retitling to update to the new CSS property.