191341 2018-11-06 17:11:51 -0800 Positioned text underline can look like a strike-through 2018-11-09 06:58:48 -0800 1 1 1 Unclassified WebKit Text WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 mmaxfield mmaxfield darin dbates dino jonlee mmaxfield simon.fraser thorton webkit-bug-importer oldest_to_newest 1475780 0 mmaxfield 2018-11-06 17:11:51 -0800 It allows negative values. We probably shouldn't allow it to look like a strike-through because that could change the semantic meaning of the text. 1475781 1 webkit-bug-importer 2018-11-06 17:12:25 -0800 <rdar://problem/45861658> 1476269 2 354165 mmaxfield 2018-11-07 15:18:47 -0800 Created attachment 354165 Patch 1476274 3 354165 simon.fraser 2018-11-07 15:24:26 -0800 Comment on attachment 354165 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354165&action=review > Source/WebCore/ChangeLog:11 > + We should just clamp the value so it can't go above the baseline. > + > + We shouldn't do this at parse time because it's totally reasonable for text-underline-position: under to want > + a negative text-underline-offset. Instead, we just do it at used value time. Will the spec describe this behavior? > Source/WebCore/style/InlineTextBoxStyle.cpp:62 > + return fontMetrics.ascent() + std::max(0.0f, underlineOffset.lengthValue()); Slightly prefer std::max<float>(0, underlineOffset.lengthValue()) > Source/WebCore/style/InlineTextBoxStyle.cpp:64 > + return fontMetrics.ascent() + std::max(0.0f, fontMetrics.underlinePosition() + underlineOffset.lengthOr(0)); Ditto. 1476347 4 mmaxfield 2018-11-07 17:08:41 -0800 Committed r237955: <https://trac.webkit.org/changeset/237955> 1476878 5 354165 darin 2018-11-09 06:57:02 -0800 Comment on attachment 354165 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354165&action=review > Source/WebCore/style/InlineTextBoxStyle.cpp:38 > + int gap = std::max<int>(1, ceilf(defaultGap / 2.0)); This involves conversion to floating point and back, and also conversion between two different floating point types. Unnecessarily complex and inefficient: defaultGap is an int we divide it by 2.0, a double, which means converting it to a double first then we call ceilf, which takes a float, which means converting the double to a float then we pass it to std::max<int>, which converts it back to an int Assuming that we don’t care about the behavior for std::numeric_limits<int>::max(), and I think we don’t since the rest of the code is written without any allowance for integer overflow, we can get the same result without involving double or float: int gap = std::max(1, (defaultGap + 1) / 2); 1476880 6 354165 darin 2018-11-09 06:58:48 -0800 Comment on attachment 354165 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354165&action=review > Source/WebCore/style/InlineTextBoxStyle.cpp:83 > + desiredOffset = std::max<float>(desiredOffset, fontMetrics.ascent()); > + return desiredOffset; No need for two lines of code here. We should just return the value; no need to write it back into the local variable first. 354165 2018-11-07 15:18:47 -0800 2018-11-07 15:24:26 -0800 Patch bug-191341-20181107151847.patch text/plain 6207 mmaxfield U3VidmVyc2lvbiBSZXZpc2lvbjogMjM3OTQ2CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggOTBiZTNiNjQyY2I1YTI0 M2Q1MWIxMjUxMWQ5NTU2NTI5NWJkMzRiYy4uNGZkN2ExNWUwNjcxZTY5NjdmODhjY2FlYmQ2ZjM2 MTI2YWI2ZGVlMCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIwIEBACisyMDE4LTExLTA3ICBNeWxl cyBDLiBNYXhmaWVsZCAgPG1tYXhmaWVsZEBhcHBsZS5jb20+CisKKyAgICAgICAgUG9zaXRpb25l ZCB0ZXh0IHVuZGVybGluZSBjYW4gbG9vayBsaWtlIGEgc3RyaWtlLXRocm91Z2gKKyAgICAgICAg aHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE5MTM0MQorCisgICAgICAg IFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFdlIHNob3VsZCBqdXN0IGNs YW1wIHRoZSB2YWx1ZSBzbyBpdCBjYW4ndCBnbyBhYm92ZSB0aGUgYmFzZWxpbmUuCisKKyAgICAg ICAgV2Ugc2hvdWxkbid0IGRvIHRoaXMgYXQgcGFyc2UgdGltZSBiZWNhdXNlIGl0J3MgdG90YWxs eSByZWFzb25hYmxlIGZvciB0ZXh0LXVuZGVybGluZS1wb3NpdGlvbjogdW5kZXIgdG8gd2FudAor ICAgICAgICBhIG5lZ2F0aXZlIHRleHQtdW5kZXJsaW5lLW9mZnNldC4gSW5zdGVhZCwgd2UganVz dCBkbyBpdCBhdCB1c2VkIHZhbHVlIHRpbWUuCisKKyAgICAgICAgVGVzdDogZmFzdC9jc3MzLXRl eHQvY3NzMy10ZXh0LWRlY29yYXRpb24vdGV4dC11bmRlcmxpbmUtbmVnYXRpdmUuaHRtbAorCisg ICAgICAgICogc3R5bGUvSW5saW5lVGV4dEJveFN0eWxlLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6 OmNvbXB1dGVVbmRlcmxpbmVPZmZzZXQpOgorCiAyMDE4LTExLTA3ICBTaW1vbiBGcmFzZXIgIDxz aW1vbi5mcmFzZXJAYXBwbGUuY29tPgogCiAgICAgICAgIFJldmVydCAyMzc4NDk6IGl0IGJyZWFr cyBNb3Rpb25NYXJrCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9zdHlsZS9JbmxpbmVUZXh0 Qm94U3R5bGUuY3BwIGIvU291cmNlL1dlYkNvcmUvc3R5bGUvSW5saW5lVGV4dEJveFN0eWxlLmNw cAppbmRleCAxMzU1ZjUyM2NiMmExZTgzNDcxZTFhOWQwYTZiMWU0ZDM3YTBhOTU2Li5lODY5NDRj YmY0YTE5YzMzMzJmOWUxMDQyNzM5NmY3NTE2MTg4OTA1IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2Vi Q29yZS9zdHlsZS9JbmxpbmVUZXh0Qm94U3R5bGUuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL3N0 eWxlL0lubGluZVRleHRCb3hTdHlsZS5jcHAKQEAgLTMyLDEwICszMiwxMCBAQAogCiBuYW1lc3Bh Y2UgV2ViQ29yZSB7CiAgICAgCi1pbnQgY29tcHV0ZVVuZGVybGluZU9mZnNldChUZXh0VW5kZXJs aW5lUG9zaXRpb24gdW5kZXJsaW5lUG9zaXRpb24sIFRleHRVbmRlcmxpbmVPZmZzZXQgdW5kZXJs aW5lT2Zmc2V0LCBjb25zdCBGb250TWV0cmljcyYgZm9udE1ldHJpY3MsIGNvbnN0IElubGluZVRl eHRCb3gqIGlubGluZVRleHRCb3gsIGludCB0ZXh0RGVjb3JhdGlvblRoaWNrbmVzcykKK2ludCBj b21wdXRlVW5kZXJsaW5lT2Zmc2V0KFRleHRVbmRlcmxpbmVQb3NpdGlvbiB1bmRlcmxpbmVQb3Np dGlvbiwgVGV4dFVuZGVybGluZU9mZnNldCB1bmRlcmxpbmVPZmZzZXQsIGNvbnN0IEZvbnRNZXRy aWNzJiBmb250TWV0cmljcywgY29uc3QgSW5saW5lVGV4dEJveCogaW5saW5lVGV4dEJveCwgaW50 IGRlZmF1bHRHYXApCiB7CiAgICAgLy8gVGhpcyByZXByZXNlbnRzIHRoZSBnYXAgYmV0d2VlbiB0 aGUgYmFzZWxpbmUgYW5kIHRoZSBjbG9zZXN0IGVkZ2Ugb2YgdGhlIHVuZGVybGluZS4KLSAgICBp bnQgZ2FwID0gc3RkOjptYXg8aW50PigxLCBjZWlsZih0ZXh0RGVjb3JhdGlvblRoaWNrbmVzcyAv IDIuMCkpOworICAgIGludCBnYXAgPSBzdGQ6Om1heDxpbnQ+KDEsIGNlaWxmKGRlZmF1bHRHYXAg LyAyLjApKTsKICAgICAKICAgICAvLyBGSVhNRTogVGhlIGNvZGUgZm9yIHZpc3VhbCBvdmVyZmxv dyBkZXRlY3Rpb24gcGFzc2VzIGluIGEgbnVsbCBpbmxpbmUgdGV4dCBib3guIFRoaXMgbWVhbnMg aXQgaXMgbm93CiAgICAgLy8gYnJva2VuIGZvciB0aGUgY2FzZSB3aGVyZSBhdXRvIG5lZWRzIHRv IGJlaGF2ZSBsaWtlICJ1bmRlciIuCkBAIC01OSw5ICs1OSw5IEBAIGludCBjb21wdXRlVW5kZXJs aW5lT2Zmc2V0KFRleHRVbmRlcmxpbmVQb3NpdGlvbiB1bmRlcmxpbmVQb3NpdGlvbiwgVGV4dFVu ZGVybGluCiAgICAgY2FzZSBUZXh0VW5kZXJsaW5lUG9zaXRpb246OkF1dG86CiAgICAgICAgIGlm ICh1bmRlcmxpbmVPZmZzZXQuaXNBdXRvKCkpCiAgICAgICAgICAgICByZXR1cm4gZm9udE1ldHJp Y3MuYXNjZW50KCkgKyBnYXA7Ci0gICAgICAgIHJldHVybiBmb250TWV0cmljcy5hc2NlbnQoKSAr IHVuZGVybGluZU9mZnNldC5sZW5ndGhWYWx1ZSgpOworICAgICAgICByZXR1cm4gZm9udE1ldHJp Y3MuYXNjZW50KCkgKyBzdGQ6Om1heCgwLjBmLCB1bmRlcmxpbmVPZmZzZXQubGVuZ3RoVmFsdWUo KSk7CiAgICAgY2FzZSBUZXh0VW5kZXJsaW5lUG9zaXRpb246OkZyb21Gb250OgotICAgICAgICBy ZXR1cm4gZm9udE1ldHJpY3MuYXNjZW50KCkgKyBmb250TWV0cmljcy51bmRlcmxpbmVQb3NpdGlv bigpICsgdW5kZXJsaW5lT2Zmc2V0Lmxlbmd0aE9yKDApOworICAgICAgICByZXR1cm4gZm9udE1l dHJpY3MuYXNjZW50KCkgKyBzdGQ6Om1heCgwLjBmLCBmb250TWV0cmljcy51bmRlcmxpbmVQb3Np dGlvbigpICsgdW5kZXJsaW5lT2Zmc2V0Lmxlbmd0aE9yKDApKTsKICAgICBjYXNlIFRleHRVbmRl cmxpbmVQb3NpdGlvbjo6VW5kZXI6IHsKICAgICAgICAgQVNTRVJUKGlubGluZVRleHRCb3gpOwog ICAgICAgICAvLyBQb3NpdGlvbiB1bmRlcmxpbmUgcmVsYXRpdmUgdG8gdGhlIGJvdHRvbSBlZGdl IG9mIHRoZSBsb3dlc3QgZWxlbWVudCdzIGNvbnRlbnQgYm94LgpAQCAtNzgsNyArNzgsOSBAQCBp bnQgY29tcHV0ZVVuZGVybGluZU9mZnNldChUZXh0VW5kZXJsaW5lUG9zaXRpb24gdW5kZXJsaW5l UG9zaXRpb24sIFRleHRVbmRlcmxpbgogICAgICAgICAgICAgcm9vdEJveC5tYXhMb2dpY2FsQm90 dG9tRm9yVGV4dERlY29yYXRpb25MaW5lKG9mZnNldCwgZGVjb3JhdGlvblJlbmRlcmVyLCBUZXh0 RGVjb3JhdGlvbjo6VW5kZXJsaW5lKTsKICAgICAgICAgICAgIG9mZnNldCAtPSBpbmxpbmVUZXh0 Qm94LT5sb2dpY2FsQm90dG9tKCk7CiAgICAgICAgIH0KLSAgICAgICAgcmV0dXJuIGlubGluZVRl eHRCb3gtPmxvZ2ljYWxIZWlnaHQoKSArIGdhcCArIHN0ZDo6bWF4PGZsb2F0PihvZmZzZXQsIDAp ICsgdW5kZXJsaW5lT2Zmc2V0Lmxlbmd0aE9yKDApOworICAgICAgICBhdXRvIGRlc2lyZWRPZmZz ZXQgPSBpbmxpbmVUZXh0Qm94LT5sb2dpY2FsSGVpZ2h0KCkgKyBnYXAgKyBzdGQ6Om1heDxmbG9h dD4ob2Zmc2V0LCAwKSArIHVuZGVybGluZU9mZnNldC5sZW5ndGhPcigwKTsKKyAgICAgICAgZGVz aXJlZE9mZnNldCA9IHN0ZDo6bWF4PGZsb2F0PihkZXNpcmVkT2Zmc2V0LCBmb250TWV0cmljcy5h c2NlbnQoKSk7CisgICAgICAgIHJldHVybiBkZXNpcmVkT2Zmc2V0OwogICAgIH0KICAgICB9CiAK ZGlmZiAtLWdpdCBhL0xheW91dFRlc3RzL0NoYW5nZUxvZyBiL0xheW91dFRlc3RzL0NoYW5nZUxv ZwppbmRleCBlOGUzODgwZTI0NGVmZDU1ZWNlYTRjYzhhZDI2ZWRlZmY3MzVhNDg4Li4zMDA4NmIz Njg5MjhlYjRhZDEyYjQyMWMyYTkyNDBmZjE3YjNlZmU5IDEwMDY0NAotLS0gYS9MYXlvdXRUZXN0 cy9DaGFuZ2VMb2cKKysrIGIvTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTMgQEAK KzIwMTgtMTEtMDcgIE15bGVzIEMuIE1heGZpZWxkICA8bW1heGZpZWxkQGFwcGxlLmNvbT4KKwor ICAgICAgICBQb3NpdGlvbmVkIHRleHQgdW5kZXJsaW5lIGNhbiBsb29rIGxpa2UgYSBzdHJpa2Ut dGhyb3VnaAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9 MTkxMzQxCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAg KiBmYXN0L2NzczMtdGV4dC9jc3MzLXRleHQtZGVjb3JhdGlvbi90ZXh0LXVuZGVybGluZS1uZWdh dGl2ZS1leHBlY3RlZC5odG1sOiBBZGRlZC4KKyAgICAgICAgKiBmYXN0L2NzczMtdGV4dC9jc3Mz LXRleHQtZGVjb3JhdGlvbi90ZXh0LXVuZGVybGluZS1uZWdhdGl2ZS5odG1sOiBBZGRlZC4KKwog MjAxOC0xMS0wNyAgU2ltb24gRnJhc2VyICA8c2ltb24uZnJhc2VyQGFwcGxlLmNvbT4KIAogICAg ICAgICBSZXZlcnQgMjM3ODQ5OiBpdCBicmVha3MgTW90aW9uTWFyawpkaWZmIC0tZ2l0IGEvTGF5 b3V0VGVzdHMvZmFzdC9jc3MzLXRleHQvY3NzMy10ZXh0LWRlY29yYXRpb24vdGV4dC11bmRlcmxp bmUtbmVnYXRpdmUtZXhwZWN0ZWQuaHRtbCBiL0xheW91dFRlc3RzL2Zhc3QvY3NzMy10ZXh0L2Nz czMtdGV4dC1kZWNvcmF0aW9uL3RleHQtdW5kZXJsaW5lLW5lZ2F0aXZlLWV4cGVjdGVkLmh0bWwK bmV3IGZpbGUgbW9kZSAxMDA2NDQKaW5kZXggMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAw MDAwMDAwMDAwMC4uMjBiNTkzZTdjMzZjZTZmYTJkZTBiMzIwZWRmZjEyYTExMGQzMzUyYgotLS0g L2Rldi9udWxsCisrKyBiL0xheW91dFRlc3RzL2Zhc3QvY3NzMy10ZXh0L2NzczMtdGV4dC1kZWNv cmF0aW9uL3RleHQtdW5kZXJsaW5lLW5lZ2F0aXZlLWV4cGVjdGVkLmh0bWwKQEAgLTAsMCArMSw5 IEBACis8IURPQ1RZUEUgaHRtbD4KKzxodG1sPgorPGhlYWQ+Cis8L2hlYWQ+Cis8Ym9keT4KK1Ro aXMgdGVzdCBtYWtlcyBzdXJlIHlvdSBjYW4ndCBwbGFjZSBhbiB1bmRlcmxpbmUgdG8gbWFrZSBp dCBsb29rIGxpa2UgYSBsaW5lLXRocm91Z2guIFRoZSB0ZXN0IHBhc3NlcyBpZiBhbGwgdGhlIHVu ZGVybGluZXMgYXJlIGluIHRoZSBjb3JyZWN0IHBsYWNlLgorPGRpdiBzdHlsZT0iZm9udDogMTAw cHggJ0FoZW0nOyB0ZXh0LWRlY29yYXRpb246IHVuZGVybGluZTsgLXdlYmtpdC10ZXh0LWRlY29y YXRpb24tY29sb3I6IGdyZWVuOyB0ZXh0LXVuZGVybGluZS1vZmZzZXQ6IDBweDsgLXdlYmtpdC10 ZXh0LWRlY29yYXRpb24tc2tpcDogbm9uZTsiPiYjeGM5OzxzcGFuIHN0eWxlPSJ0ZXh0LXVuZGVy bGluZS1vZmZzZXQ6IDEzcHg7Ij4mI3hjOTs8c3BhbiBzdHlsZT0idGV4dC11bmRlcmxpbmUtb2Zm c2V0OiAyM3B4OyI+JiN4Yzk7PC9zcGFuPjwvc3Bhbj48L2Rpdj4KKzwvYm9keT4KKzwvaHRtbD4K ZGlmZiAtLWdpdCBhL0xheW91dFRlc3RzL2Zhc3QvY3NzMy10ZXh0L2NzczMtdGV4dC1kZWNvcmF0 aW9uL3RleHQtdW5kZXJsaW5lLW5lZ2F0aXZlLmh0bWwgYi9MYXlvdXRUZXN0cy9mYXN0L2NzczMt dGV4dC9jc3MzLXRleHQtZGVjb3JhdGlvbi90ZXh0LXVuZGVybGluZS1uZWdhdGl2ZS5odG1sCm5l dyBmaWxlIG1vZGUgMTAwNjQ0CmluZGV4IDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAw MDAwMDAwMDAuLmQ0YTUxYTI2Nzc1NDM1NzRlZTNmMDczZDhlYTUzYWYwMzhjMzdhM2UKLS0tIC9k ZXYvbnVsbAorKysgYi9MYXlvdXRUZXN0cy9mYXN0L2NzczMtdGV4dC9jc3MzLXRleHQtZGVjb3Jh dGlvbi90ZXh0LXVuZGVybGluZS1uZWdhdGl2ZS5odG1sCkBAIC0wLDAgKzEsOSBAQAorPCFET0NU WVBFIGh0bWw+Cis8aHRtbD4KKzxoZWFkPgorPC9oZWFkPgorPGJvZHk+CitUaGlzIHRlc3QgbWFr ZXMgc3VyZSB5b3UgY2FuJ3QgcGxhY2UgYW4gdW5kZXJsaW5lIHRvIG1ha2UgaXQgbG9vayBsaWtl IGEgbGluZS10aHJvdWdoLiBUaGUgdGVzdCBwYXNzZXMgaWYgYWxsIHRoZSB1bmRlcmxpbmVzIGFy ZSBpbiB0aGUgY29ycmVjdCBwbGFjZS4KKzxkaXYgc3R5bGU9ImZvbnQ6IDEwMHB4ICdBaGVtJzsg dGV4dC1kZWNvcmF0aW9uOiB1bmRlcmxpbmU7IC13ZWJraXQtdGV4dC1kZWNvcmF0aW9uLWNvbG9y OiBncmVlbjsgdGV4dC11bmRlcmxpbmUtb2Zmc2V0OiAtMTBweDsgLXdlYmtpdC10ZXh0LWRlY29y YXRpb24tc2tpcDogbm9uZTsiPiYjeGM5OzxzcGFuIHN0eWxlPSJ0ZXh0LXVuZGVybGluZS1wb3Np dGlvbjogdW5kZXI7Ij4mI3hjOTs8c3BhbiBzdHlsZT0idGV4dC11bmRlcmxpbmUtb2Zmc2V0OiBh dXRvOyI+JiN4Yzk7PC9zcGFuPjwvc3Bhbj48L2Rpdj4KKzwvYm9keT4KKzwvaHRtbD4K