| Summary: | Turn text-decoration-skip: ink on for all underlines | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||
| Component: | New Bugs | Assignee: | 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
Myles C. Maxfield
2014-01-20 19:40:45 PST
Created attachment 221713 [details]
Patch
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?
Nope! Antti, is this OK? Do we need to add this to the list of CSS properties that come first and get special treatment? I don't understand this patch. Why is this property set as UA style for <body>? Comment on attachment 221713 [details]
Patch
Setting r- until I understand.
Created attachment 222036 [details]
Patch
Comment on attachment 222036 [details]
Patch
r=me
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. 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 (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? (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(). I'd also be interested to see what happened in vertical Japanese text for skip:ink. Is this a bug? (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. (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. |