Bug 127331

Summary: Turn text-decoration-skip: ink on for all underlines
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: New BugsAssignee: Myles C. Maxfield <mmaxfield>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dino, esprehn+autocc, glenn, gyuyoung.kim, jonlee, koivisto, kondapallykalyan, macpherson, menard, mjs, sam, simon.fraser, thorton, webkit-bug-importer, yuki.sekiguchi
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch koivisto: review+

Myles C. Maxfield
Reported 2014-01-20 19:40:45 PST
Turn text-decoration-skip: ink on for all underlines
Attachments
Patch (1.20 KB, patch)
2014-01-20 19:42 PST, Myles C. Maxfield 		no flags
DetailsFormatted DiffDiff
Patch (1.60 KB, patch)
2014-01-23 15:54 PST, Myles C. Maxfield 		koivisto: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Myles C. Maxfield
Comment 1 2014-01-20 19:42:58 PST
Created attachment 221713 [details] Patch
Myles C. Maxfield
Comment 2 2014-01-20 19:45:41 PST
<rdar://problem/15820629>
Brent Fulgham
Comment 3 2014-01-22 15:42:30 PST
Comment on attachment 221713 [details] Patch This seems fine, but are there any test implications? Do we need new results, or will existing results change because of this?
Myles C. Maxfield
Comment 4 2014-01-22 17:08:53 PST
Nope!
Simon Fraser (smfr)
Comment 5 2014-01-22 17:35:20 PST
Antti, is this OK? Do we need to add this to the list of CSS properties that come first and get special treatment?
Antti Koivisto
Comment 6 2014-01-23 15:03:28 PST
I don't understand this patch. Why is this property set as UA style for <body>?
Antti Koivisto
Comment 7 2014-01-23 15:05:12 PST
Comment on attachment 221713 [details] Patch Setting r- until I understand.
Myles C. Maxfield
Comment 8 2014-01-23 15:54:15 PST
Created attachment 222036 [details] Patch
Antti Koivisto
Comment 9 2014-01-23 15:58:33 PST
Comment on attachment 222036 [details] Patch r=me
Myles C. Maxfield
Comment 10 2014-01-23 16:19:16 PST
http://trac.webkit.org/changeset/162660
Yuki Sekiguchi
Comment 11 2014-02-06 17:57:44 PST
IIUC, the initial value of text-decoration-skip property is "objects". Why did this commit change to ink? http://dev.w3.org/csswg/css-text-decor-3/#text-decoration-skip-property If there is no problem to change the initial value to "objects", I can create a patch.
Myles C. Maxfield
Comment 12 2014-02-07 11:19:28 PST
text-decoration-skip: objects isn't implemented (yet). Regarding the difference between html.css and RenderStyle.h, this is what antti has to say: 3:25:19 PM myles: antti: that's fine with me. just curious, why is one way better than the other? 3:38:16 PM antti: myles: at minimum it looks ugly to have non-body properties defined there 3:39:26 PM antti: myles: i believe it will make style code less efficient since we every element will have non-default inherited style 3:39:41 PM antti: and i think it will break in some css edge cases 3:41:03 PM antti: myles: they can also change based on platform/settings etc 3:43:29 PM antti: myles: at least text-decoration-skip:initial will be different 3:44:32 PM antti: myles: 'initial' is a css value that resolves to the default for the property
Yuki Sekiguchi
Comment 13 2014-02-09 18:05:25 PST
(In reply to comment #12) > text-decoration-skip: objects isn't implemented (yet). Theoretically, the newer CSS spec is backward compatible, so initial value should be same as old behavior. Therefore, "objects" should be same as original behavior. We no need to implement rendering code. > Regarding the difference between html.css and RenderStyle.h, this is what antti has to say: > 3:25:19 PM myles: antti: that's fine with me. just curious, why is one way better than the other? > 3:38:16 PM antti: myles: at minimum it looks ugly to have non-body properties defined there > 3:39:26 PM antti: myles: i believe it will make style code less efficient since we every element will have non-default inherited style > 3:39:41 PM antti: and i think it will break in some css edge cases > 3:41:03 PM antti: myles: they can also change based on platform/settings etc > 3:43:29 PM antti: myles: at least text-decoration-skip:initial will be different > 3:44:32 PM antti: myles: 'initial' is a css value that resolves to the default for the property There is no spec which defines html.css, so adding "ink" to html.css doesn't illegal. For "initial" and RenderStyle.h, CSS spec defines it, so initialTextDecorationSkip() should return "objects". IMO, adding "ink" to html.css changes the default behavior, so it may break the web. I drew underline to a Japanese character "国" in vertical writing mode, and it looked odd for my Japanese eyes. I think we should not add "ink" to html.css. WDYT?
Dean Jackson
Comment 14 2014-02-09 21:54:34 PST
(In reply to comment #13) > IMO, adding "ink" to html.css changes the default behavior, so it may break the web. I drew underline to a Japanese character "国" in vertical writing mode, and it looked odd for my Japanese eyes. I think we should not add "ink" to html.css. > > WDYT? I agree with Yuki. If the default is "objects" but we don't support it yet, we should leave it as it was for now. And yes, the way to define the default CSS value is in RenderStyle::initialTextDecorationSkip().
Dean Jackson
Comment 15 2014-02-09 21:57:13 PST
I'd also be interested to see what happened in vertical Japanese text for skip:ink. Is this a bug?
Yuki Sekiguchi
Comment 16 2014-02-09 22:56:13 PST
(In reply to comment #14) > I agree with Yuki. If the default is "objects" but we don't support it yet, we should leave it as it was for now. And yes, the way to define the default CSS value is in RenderStyle::initialTextDecorationSkip(). I didn't realize that CSS parser didn't support "objects". If there is no objection, I'll add "objects" to the parser and change the initial value. (In reply to comment #15) > I'd also be interested to see what happened in vertical Japanese text for skip:ink. Is this a bug? I checked some Japanese texts. Most of them are no problem, but I found some bugs. I filed the Bug 128518 If the bug is fixed, I think most Japanese don't like these underlines.
Yuki Sekiguchi
Comment 17 2014-02-12 21:46:27 PST
(In reply to comment #16) > (In reply to comment #14) > > I agree with Yuki. If the default is "objects" but we don't support it yet, we should leave it as it was for now. And yes, the way to define the default CSS value is in RenderStyle::initialTextDecorationSkip(). > > I didn't realize that CSS parser didn't support "objects". If there is no objection, I'll add "objects" to the parser and change the initial value. Filed Bug 128723 and uploaded the patch.
Note You need to log in before you can comment on or make changes to this bug.