Bug 128723 - [css3-text] Support -webkit-text-decoration-skip: objects
Summary: [css3-text] Support -webkit-text-decoration-skip: objects
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-02-12 21:34 PST by Yuki Sekiguchi
Modified: 2016-09-17 07:17 PDT (History)
9 users (show)

See Also:


Attachments
Patch (18.90 KB, patch)
2014-02-12 21:42 PST, Yuki Sekiguchi
no flags Details | Formatted Diff | Diff
Patch (5.75 KB, patch)
2014-02-13 23:31 PST, Yuki Sekiguchi
no flags Details | Formatted Diff | Diff
Patch (6.61 KB, patch)
2014-02-17 20:31 PST, Yuki Sekiguchi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yuki Sekiguchi 2014-02-12 21:34:52 PST
Support 'objects' value of text-decoration-skip.
http://www.w3.org/TR/2013/CR-css-text-decor-3-20130801/#text-decoration-skip
Comment 1 Yuki Sekiguchi 2014-02-12 21:42:20 PST
Created attachment 224045 [details]
Patch
Comment 2 Myles C. Maxfield 2014-02-13 12:19:09 PST
<rdar://problem/15346113>
Comment 3 Dean Jackson 2014-02-13 14:26:40 PST
Comment on attachment 224045 [details]
Patch

Great!
Comment 4 WebKit Commit Bot 2014-02-13 14:57:26 PST
Comment on attachment 224045 [details]
Patch

Clearing flags on attachment: 224045

Committed r164061: <http://trac.webkit.org/changeset/164061>
Comment 5 WebKit Commit Bot 2014-02-13 14:57:29 PST
All reviewed patches have been landed.  Closing bug.
Comment 6 Myles C. Maxfield 2014-02-13 15:38:58 PST
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 7 Myles C. Maxfield 2014-02-13 15:40:05 PST
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
Comment 8 Yuki Sekiguchi 2014-02-13 23:30:57 PST
Reopening to attach new patch.
Comment 9 Yuki Sekiguchi 2014-02-13 23:31:00 PST
Created attachment 224167 [details]
Patch
Comment 10 Yuki Sekiguchi 2014-02-13 23:41:40 PST
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 11 Myles C. Maxfield 2014-02-14 11:18:47 PST
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 12 Simon Fraser (smfr) 2014-02-14 11:36:55 PST
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.
Comment 13 Yuki Sekiguchi 2014-02-17 20:31:08 PST
Created attachment 224462 [details]
Patch
Comment 14 Yuki Sekiguchi 2014-02-17 20:47:00 PST
(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 15 Michael Catanzaro 2016-09-17 07:17:18 PDT
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.