127331 2014-01-20 19:40:45 -0800 Turn text-decoration-skip: ink on for all underlines 2014-02-12 21:46:27 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 mmaxfield mmaxfield commit-queue dino esprehn+autocc glenn gyuyoung.kim jonlee koivisto kondapallykalyan macpherson menard mjs sam simon.fraser thorton webkit-bug-importer yuki.sekiguchi oldest_to_newest 970140 0 mmaxfield 2014-01-20 19:40:45 -0800 Turn text-decoration-skip: ink on for all underlines 970141 1 221713 mmaxfield 2014-01-20 19:42:58 -0800 Created attachment 221713 Patch 970143 2 mmaxfield 2014-01-20 19:45:41 -0800 <rdar://problem/15820629> 970916 3 221713 bfulgham 2014-01-22 15:42:30 -0800 Comment on attachment 221713 Patch This seems fine, but are there any test implications? Do we need new results, or will existing results change because of this? 970939 4 mmaxfield 2014-01-22 17:08:53 -0800 Nope! 970952 5 simon.fraser 2014-01-22 17:35:20 -0800 Antti, is this OK? Do we need to add this to the list of CSS properties that come first and get special treatment? 971353 6 koivisto 2014-01-23 15:03:28 -0800 I don't understand this patch. Why is this property set as UA style for <body>? 971355 7 221713 koivisto 2014-01-23 15:05:12 -0800 Comment on attachment 221713 Patch Setting r- until I understand. 971373 8 222036 mmaxfield 2014-01-23 15:54:15 -0800 Created attachment 222036 Patch 971376 9 222036 koivisto 2014-01-23 15:58:33 -0800 Comment on attachment 222036 Patch r=me 971384 10 mmaxfield 2014-01-23 16:19:16 -0800 http://trac.webkit.org/changeset/162660 977667 11 yuki.sekiguchi 2014-02-06 17:57:44 -0800 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. 977928 12 mmaxfield 2014-02-07 11:19:28 -0800 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 978681 13 yuki.sekiguchi 2014-02-09 18:05:25 -0800 (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? 978729 14 dino 2014-02-09 21:54:34 -0800 (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(). 978730 15 dino 2014-02-09 21:57:13 -0800 I'd also be interested to see what happened in vertical Japanese text for skip:ink. Is this a bug? 978742 16 yuki.sekiguchi 2014-02-09 22:56:13 -0800 (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. 980164 17 yuki.sekiguchi 2014-02-12 21:46:27 -0800 (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. 221713 2014-01-20 19:42:58 -0800 2014-01-23 15:54:12 -0800 Patch bug-127331-20140120194257.patch text/plain 1228 mmaxfield U3VidmVyc2lvbiBSZXZpc2lvbjogMTYyMzg1CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYTBlYWFkZTFlODk5NjMw NjI4NTEzYWY0ZDExYmU4YmNjZmM4M2M1Zi4uOWFjOTcxMGY1OTk2ZGY4Zjc4ZmNmMzQ3OTRhYTNh NzZmODVhMjgxOSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE3IEBACisyMDE0LTAxLTIwICBNeWxl cyBDLiBNYXhmaWVsZCAgPG1tYXhmaWVsZEBhcHBsZS5jb20+CisKKyAgICAgICAgVHVybiB0ZXh0 LWRlY29yYXRpb24tc2tpcDogaW5rIG9uIGZvciBhbGwgdW5kZXJsaW5lcworICAgICAgICBodHRw czovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTI3MzMxCisKKyAgICAgICAgUmV2 aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgTm8gbmV3IHRlc3RzIGFyZSBuZWNl c3NhcnkgYmVjYXVzZSB0ZXN0cyBhbHJlYWR5IGV4aXN0CisKKyAgICAgICAgKiBjc3MvaHRtbC5j c3M6CisgICAgICAgIChib2R5KToKKyAgICAgICAgKHUsIGlucyk6CisgICAgICAgIChhOi13ZWJr aXQtYW55LWxpbms6YWN0aXZlKToKKwogMjAxNC0wMS0yMCAgR3l1eW91bmcgS2ltICA8Z3l1eW91 bmcua2ltQHNhbXN1bmcuY29tPgogCiAgICAgICAgIERvIHJlZmFjdG9yIGluIGNvbGxlY3RHcmFk aWVudEF0dHJpYnV0ZXMoKSBhbmQgcmVuZGVyU3R5bGVGb3JMZW5ndGhSZXNvbHZlKCkKZGlmZiAt LWdpdCBhL1NvdXJjZS9XZWJDb3JlL2Nzcy9odG1sLmNzcyBiL1NvdXJjZS9XZWJDb3JlL2Nzcy9o dG1sLmNzcwppbmRleCBmMmNiODFiZGFlYjNjOWM5MWRlNTljMTgxNzNiZTcyNzc3OGM2YzgxLi4w MWNlNmEzNjllYTQzZDc0MzUzOGRmYjdhOWJmYTVmYWJjM2U1Yzk5IDEwMDY0NAotLS0gYS9Tb3Vy Y2UvV2ViQ29yZS9jc3MvaHRtbC5jc3MKKysrIGIvU291cmNlL1dlYkNvcmUvY3NzL2h0bWwuY3Nz CkBAIC01Niw3ICs1Niw4IEBAIHNjcmlwdCB7CiAKIGJvZHkgewogICAgIGRpc3BsYXk6IGJsb2Nr OwotICAgIG1hcmdpbjogOHB4CisgICAgbWFyZ2luOiA4cHg7CisgICAgLXdlYmtpdC10ZXh0LWRl Y29yYXRpb24tc2tpcDogaW5rOwogfQogCiAjaWYgZGVmaW5lZChFTkFCTEVfSUZSQU1FX1NFQU1M RVNTKSAmJiBFTkFCTEVfSUZSQU1FX1NFQU1MRVNTCg== 222036 2014-01-23 15:54:15 -0800 2014-01-23 15:58:33 -0800 Patch bug-127331-20140123155414.patch text/plain 1641 mmaxfield U3VidmVyc2lvbiBSZXZpc2lvbjogMTYyNTg4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYjY4NThiMzE5NDkyNmE3 ZDdlZmU0ZmU3MDZiY2FkMjE2OTFjNWRhYy4uYzc4YjhiOTY1ZTY1NGVjMGFmYjdjNjRjODM5NjA3 OWRjZTI3ZTQ3YyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE0IEBACisyMDE0LTAxLTIwICBNeWxl cyBDLiBNYXhmaWVsZCAgPG1tYXhmaWVsZEBhcHBsZS5jb20+CisKKyAgICAgICBUdXJuIHRleHQt ZGVjb3JhdGlvbi1za2lwOiBpbmsgb24gZm9yIGFsbCB1bmRlcmxpbmVzCisgICAgICAgaHR0cHM6 Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEyNzMzMQorCisgICAgICAgUmV2aWV3 ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICBObyBuZXcgdGVzdHMgYXJlIG5lY2Vzc2Fy eSBiZWNhdXNlIHRlc3RzIGFscmVhZHkgZXhpc3QKKworICAgICAgICAqIHJlbmRlcmluZy9zdHls ZS9SZW5kZXJTdHlsZS5oOgorCiAyMDE0LTAxLTIyICBNeWxlcyBDLiBNYXhmaWVsZCAgPG1tYXhm aWVsZEBhcHBsZS5jb20+CiAKICAgICAgICAgQVNTRVJUSU9OIEZBSUxFRDogdi5pc0ZpeGVkKCkg aW4gV2ViQ29yZTo6UmVuZGVyU3R5bGU6OnNldFdvcmRTcGFjaW5nCmRpZmYgLS1naXQgYS9Tb3Vy Y2UvV2ViQ29yZS9yZW5kZXJpbmcvc3R5bGUvUmVuZGVyU3R5bGUuaCBiL1NvdXJjZS9XZWJDb3Jl L3JlbmRlcmluZy9zdHlsZS9SZW5kZXJTdHlsZS5oCmluZGV4IDA5NjMxMTVjNjViNjcwYjQ0NTc4 OTU5MjI1MjIyMWEzMzQwNjVhOTcuLjVmM2YzZWY1Zjc4ZmMxNjBjMzgwMTdhZDE5ZDdjZDk5NmRi NDRmYjAgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9zdHlsZS9SZW5kZXJT dHlsZS5oCisrKyBiL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9zdHlsZS9SZW5kZXJTdHlsZS5o CkBAIC0xNjg1LDcgKzE2ODUsNyBAQCBwdWJsaWM6CiAgICAgc3RhdGljIFRleHRKdXN0aWZ5IGlu aXRpYWxUZXh0SnVzdGlmeSgpIHsgcmV0dXJuIFRleHRKdXN0aWZ5QXV0bzsgfQogI2VuZGlmIC8v IENTUzNfVEVYVAogICAgIHN0YXRpYyBUZXh0RGVjb3JhdGlvblN0eWxlIGluaXRpYWxUZXh0RGVj b3JhdGlvblN0eWxlKCkgeyByZXR1cm4gVGV4dERlY29yYXRpb25TdHlsZVNvbGlkOyB9Ci0gICAg c3RhdGljIFRleHREZWNvcmF0aW9uU2tpcCBpbml0aWFsVGV4dERlY29yYXRpb25Ta2lwKCkgeyBy ZXR1cm4gVGV4dERlY29yYXRpb25Ta2lwTm9uZTsgfQorICAgIHN0YXRpYyBUZXh0RGVjb3JhdGlv blNraXAgaW5pdGlhbFRleHREZWNvcmF0aW9uU2tpcCgpIHsgcmV0dXJuIFRleHREZWNvcmF0aW9u U2tpcEluazsgfQogICAgIHN0YXRpYyBUZXh0VW5kZXJsaW5lUG9zaXRpb24gaW5pdGlhbFRleHRV bmRlcmxpbmVQb3NpdGlvbigpIHsgcmV0dXJuIFRleHRVbmRlcmxpbmVQb3NpdGlvbkF1dG87IH0K ICAgICBzdGF0aWMgZmxvYXQgaW5pdGlhbFpvb20oKSB7IHJldHVybiAxLjBmOyB9CiAgICAgc3Rh dGljIGludCBpbml0aWFsT3V0bGluZU9mZnNldCgpIHsgcmV0dXJuIDA7IH0K