176995 2017-09-15 05:31:04 -0700 [Harfbuzz] Material icons not rendered correctly when using the web font 2017-09-27 12:30:25 -0700 1 1 1 Unclassified WebKit Platform WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED Gtk, InRadar P2 Normal --- 1 cgarcia webkit-unassigned bugs-noreply mcatanzaro svillar webkit-bug-importer oldest_to_newest 1349265 0 cgarcia 2017-09-15 05:31:04 -0700 You can see it here: https://material.io/icons/ Only a few of them are correctly rendered and some others are wrong. We only render correctly the ones that doesn't have an underscore in its name (or that start with a number like 3d_rotation. In the cases where the name before the underscore is also an icon, we render that icon instead, that's why some of them are wrong. This is happening because the underscore is causing the HarfbuffShaper to split the text in 3 runs, one for the word before the underscore, another one for the underscore and another one for the word after the underscore. So, we end up trying to shape the 3 runs independently and we fail when the icon doesn't exist, or when it exists but it's not the one we are looking for. The cause of this is that the underscore has a different script (Common) than the rest of characters (Latin) which is a condition in HarfbuffShaper to create a different run. The unicode spec says that characters with Common script should be handled differently, but we are just ignoring it. The spec proposes to use an heuristic based on simply inheriting the script of the previous character which should work in most of the cases. We could take a more conservative approach and do that only if both character are ASCII. We should also consider handling other cases mentioned by the spec like brackets and quotation marks, but that belongs to a different bug/commit. 1349267 1 320891 cgarcia 2017-09-15 05:36:34 -0700 Created attachment 320891 Patch 1349285 2 320891 mcatanzaro 2017-09-15 06:42:22 -0700 Comment on attachment 320891 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320891&action=review > Source/WebCore/ChangeLog:20 > + are ASCII. We should also consider handling other cases mentioned by the spec like brackets and quotation marks, > + but that belongs to a different bug/commit. r=me if you file a new bug report. > Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaper.cpp:379 > + if (isASCII(character) && isASCII(previousCharacter)) Why the isASCII check? Did this fix something? Surely that's not going to work for e.g. curly quotation marks. 1349287 3 cgarcia 2017-09-15 06:44:46 -0700 (In reply to Michael Catanzaro from comment #2) > Comment on attachment 320891 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=320891&action=review > > > Source/WebCore/ChangeLog:20 > > + are ASCII. We should also consider handling other cases mentioned by the spec like brackets and quotation marks, > > + but that belongs to a different bug/commit. > > r=me if you file a new bug report. Sure, I will. > > Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaper.cpp:379 > > + if (isASCII(character) && isASCII(previousCharacter)) > > Why the isASCII check? Did this fix something? Surely that's not going to > work for e.g. curly quotation marks. Just tried to be conservative here. Why do you think curly quotation marks are going to be broken? 1349288 4 mcatanzaro 2017-09-15 06:49:56 -0700 I mean if this heuristic is extended to more characters besides underscores, like quotation marks, as you indicated needs to be done in the FIXME, then obviously it's not going to work for curly quotes as you have an ASCII check. I don't understand what the ASCII check is for. Why is it relevant that both characters be ASCII? 1349289 5 cgarcia 2017-09-15 06:55:13 -0700 (In reply to Michael Catanzaro from comment #4) > I mean if this heuristic is extended to more characters besides underscores, > like quotation marks, as you indicated needs to be done in the FIXME, then > obviously it's not going to work for curly quotes as you have an ASCII > check. I don't understand what the ASCII check is for. Why is it relevant > that both characters be ASCII? Because if both are ascii it's safer to use the same run, in all other cases we are using a different run, like current code does. Note also, that this check only happens when the next character script is different than the current one and it's Common. 1349295 6 svillar 2017-09-15 07:34:01 -0700 The fix is awesome but we should try hard to have test cases for this, otherwise we're going to eventually break it. 1349304 7 cgarcia 2017-09-15 08:24:45 -0700 (In reply to Sergio Villar Senin from comment #6) > The fix is awesome but we should try hard to have test cases for this, > otherwise we're going to eventually break it. I could add the material design icons to wkgtk fonts test repo, and add a test case. I think the license is apache2, I don't know if we can add it to WebKit to make a cross-platform test instead. A good ref test could be to use the font names in the test and the unicode code directly in the reference, since the unicode code always worked, the problem was only with icon names. <i class="material-icons">3d_rotation</i> <- doesn't work <i class="material-icons">&#xE84D;</i> <- works 1349312 8 cgarcia 2017-09-15 08:37:12 -0700 Committed r222090: <http://trac.webkit.org/changeset/222090> 1353433 9 webkit-bug-importer 2017-09-27 12:30:25 -0700 <rdar://problem/34693390> 320891 2017-09-15 05:36:34 -0700 2017-09-15 06:42:22 -0700 Patch wk-material-icons.diff text/plain 5767 cgarcia ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCAxODc0MWViNDY4Ny4uNTNlMjgzZjlmZjggMTAwNjQ0Ci0tLSBhL1NvdXJj ZS9XZWJDb3JlL0NoYW5nZUxvZworKysgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwzMCBAQAorMjAxNy0wOS0xNSAgQ2FybG9zIEdhcmNpYSBDYW1wb3MgIDxjZ2FyY2lhQGln YWxpYS5jb20+CisKKyAgICAgICAgW0hhcmZidXp6XSBNYXRlcmlhbCBpY29ucyBub3QgcmVuZGVy ZWQgY29ycmVjdGx5IHdoZW4gdXNpbmcgdGhlIHdlYiBmb250CisgICAgICAgIGh0dHBzOi8vYnVn cy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNzY5OTUKKworICAgICAgICBSZXZpZXdlZCBi eSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBPbmx5IGEgZmV3IG9mIHRoZW0gYXJlIGNvcnJl Y3RseSByZW5kZXJlZCBhbmQgc29tZSBvdGhlcnMgYXJlIHdyb25nLiBXZSBvbmx5IHJlbmRlciBj b3JyZWN0bHkgdGhlIG9uZXMgdGhhdAorICAgICAgICBkb24ndCBoYXZlIGFuIHVuZGVyc2NvcmUg aW4gdGhlaXIgbmFtZSAob3IgdGhhdCBzdGFydCB3aXRoIGEgbnVtYmVyIGxpa2UgM2Rfcm90YXRp b24pLiBJbiB0aGUgY2FzZXMgd2hlcmUgdGhlCisgICAgICAgIG5hbWUgYmVmb3JlIHRoZSB1bmRl cnNjb3JlIGlzIGFsc28gYW4gaWNvbiwgd2UgcmVuZGVyIHRoYXQgaWNvbiBpbnN0ZWFkLCB0aGF0 J3Mgd2h5IHNvbWUgb2YgdGhlbSBhcmUgd3JvbmcuIFRoaXMKKyAgICAgICAgaXMgaGFwcGVuaW5n IGJlY2F1c2UgdGhlIHVuZGVyc2NvcmUgaXMgY2F1c2luZyB0aGUgSGFyZmJ1ZmZTaGFwZXIgdG8g c3BsaXQgdGhlIHRleHQgaW4gMyBydW5zLCBvbmUgZm9yIHRoZSB3b3JkCisgICAgICAgIGJlZm9y ZSB0aGUgdW5kZXJzY29yZSwgYW5vdGhlciBvbmUgZm9yIHRoZSB1bmRlcnNjb3JlIGFuZCBhbm90 aGVyIGZvciB0aGUgd29yZCBhZnRlciB0aGUgdW5kZXJzY29yZS4gU28sIHdlCisgICAgICAgIGVu ZCB1cCB0cnlpbmcgdG8gc2hhcGUgdGhlIDMgcnVucyBpbmRlcGVuZGVudGx5IGFuZCB3ZSBmYWls IHdoZW4gdGhlIGljb24gZG9lc24ndCBleGlzdCwgb3Igd2hlbiBpdCBleGlzdHMgYnV0CisgICAg ICAgIGl0J3Mgbm90IHRoZSBvbmUgd2UgYXJlIGxvb2tpbmcgZm9yLiBUaGUgY2F1c2Ugb2YgdGhp cyBpcyB0aGF0IHRoZSB1bmRlcnNjb3JlIGhhcyBhIGRpZmZlcmVudCBzY3JpcHQgKENvbW1vbikK KyAgICAgICAgdGhhbiB0aGUgcmVzdCBvZiBjaGFyYWN0ZXJzIChMYXRpbikgd2hpY2ggaXMgYSBj b25kaXRpb24gaW4gSGFyZmJ1ZmZTaGFwZXIgdG8gY3JlYXRlIGEgZGlmZmVyZW50IHJ1bi4gVGhl CisgICAgICAgIHVuaWNvZGUgc3BlYyBzYXlzIHRoYXQgY2hhcmFjdGVycyB3aXRoIENvbW1vbiBz Y3JpcHQgc2hvdWxkIGJlIGhhbmRsZWQgZGlmZmVyZW50bHksIGJ1dCB3ZSBhcmUganVzdCBpZ25v cmluZworICAgICAgICBpdC4gVGhlIHNwZWMgcHJvcG9zZXMgdG8gdXNlIGFuIGhldXJpc3RpYyBi YXNlZCBvbiBzaW1wbHkgaW5oZXJpdGluZyB0aGUgc2NyaXB0IG9mIHRoZSBwcmV2aW91cyBjaGFy YWN0ZXIsIHdoaWNoCisgICAgICAgIHNob3VsZCB3b3JrIGluIG1vc3Qgb2YgdGhlIGNhc2VzLiBX ZSBjb3VsZCB0YWtlIGEgbW9yZSBjb25zZXJ2YXRpdmUgYXBwcm9hY2ggYW5kIGRvIHRoYXQgb25s eSBpZiBib3RoIGNoYXJhY3RlcnMKKyAgICAgICAgYXJlIEFTQ0lJLiBXZSBzaG91bGQgYWxzbyBj b25zaWRlciBoYW5kbGluZyBvdGhlciBjYXNlcyBtZW50aW9uZWQgYnkgdGhlIHNwZWMgbGlrZSBi cmFja2V0cyBhbmQgcXVvdGF0aW9uIG1hcmtzLAorICAgICAgICBidXQgdGhhdCBiZWxvbmdzIHRv IGEgZGlmZmVyZW50IGJ1Zy9jb21taXQuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9ncmFwaGljcy9o YXJmYnV6ei9IYXJmQnV6elNoYXBlci5jcHA6CisgICAgICAgIChXZWJDb3JlOjpzY3JpcHRzQXJl Q29tcGF0aWJsZUZvckNoYXJhY3RlcnMpOiBIZWxwZXIgZnVuY3Rpb24gdG8gY2hlY2sgaWYgdGhl IGN1cnJlbnQgYW5kIHByZXZpb3VzIHNjcmlwdHMgYXJlCisgICAgICAgIGNvbXBhdGlibGUsCisg ICAgICAgIChXZWJDb3JlOjpIYXJmQnV6elNoYXBlcjo6Y29sbGVjdEhhcmZCdXp6UnVucyk6IFVz ZSBzY3JpcHRzQXJlQ29tcGF0aWJsZUZvckNoYXJhY3RlcnMoKSB0byBkZWNpZGVkIHdoZXRoZXIg dG8KKyAgICAgICAgZmluaXNoIHRoZSBjdXJyZW50IHJ1biBvciBub3QuIEluIGNhc2Ugb2YgQ29t bW9uIHNjcmlwdCwgaW5oZXJpdCBhbHNvIHRoZSBzY3JpcHQgZnJvbSB0aGUgcHJldmlvdXMgY2hh cmFjdGVyLgorCiAyMDE3LTA5LTE1ICBDYXJsb3MgR2FyY2lhIENhbXBvcyAgPGNnYXJjaWFAaWdh bGlhLmNvbT4KIAogICAgICAgICBbSGFyZmJ1enpdIEZpeCBpbmNvcnJlY3QgZm9udCByZW5kZXJp bmcgd2hlbiBzZWxlY3RpbmcgdGV4dHMgaW4gcGFnZXMgd2hpY2ggc3BlY2lmaWVzIHRleHQtcmVu ZGVyaW5nOiBvcHRpbWl6ZUxlZ2liaWxpdHkKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3Bs YXRmb3JtL2dyYXBoaWNzL2hhcmZidXp6L0hhcmZCdXp6U2hhcGVyLmNwcCBiL1NvdXJjZS9XZWJD b3JlL3BsYXRmb3JtL2dyYXBoaWNzL2hhcmZidXp6L0hhcmZCdXp6U2hhcGVyLmNwcAppbmRleCBi YzE2OWM3OTRlNi4uMWMzYjQzYjE2YWYgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3BsYXRm b3JtL2dyYXBoaWNzL2hhcmZidXp6L0hhcmZCdXp6U2hhcGVyLmNwcAorKysgYi9Tb3VyY2UvV2Vi Q29yZS9wbGF0Zm9ybS9ncmFwaGljcy9oYXJmYnV6ei9IYXJmQnV6elNoYXBlci5jcHAKQEAgLTM1 OSw2ICszNTksMzAgQEAgRmxvYXRQb2ludCBIYXJmQnV6elNoYXBlcjo6YWRqdXN0U3RhcnRQb2lu dChjb25zdCBGbG9hdFBvaW50JiBwb2ludCkKICAgICByZXR1cm4gcG9pbnQgKyBtX3N0YXJ0T2Zm c2V0OwogfQogCitzdGF0aWMgYm9vbCBzY3JpcHRzQXJlQ29tcGF0aWJsZUZvckNoYXJhY3RlcnMo VVNjcmlwdENvZGUgc2NyaXB0LCBVU2NyaXB0Q29kZSBwcmV2aW91c1NjcmlwdCwgVUNoYXIzMiBj aGFyYWN0ZXIsIFVDaGFyMzIgcHJldmlvdXNDaGFyYWN0ZXIpCit7CisgICAgaWYgKHNjcmlwdCA9 PSBwcmV2aW91c1NjcmlwdCkKKyAgICAgICAgcmV0dXJuIHRydWU7CisKKyAgICBpZiAoc2NyaXB0 ID09IFVTQ1JJUFRfSU5IRVJJVEVEIHx8IHByZXZpb3VzU2NyaXB0ID09IFVTQ1JJUFRfQ09NTU9O KQorICAgICAgICByZXR1cm4gdHJ1ZTsKKworICAgIGlmIChzY3JpcHQgPT0gVVNDUklQVF9DT01N T04pIHsKKyAgICAgICAgLy8gwqc1LjEgSGFuZGxpbmcgQ2hhcmFjdGVycyB3aXRoIHRoZSBDb21t b24gU2NyaXB0IFByb3BlcnR5LgorICAgICAgICAvLyBQcm9ncmFtcyBtdXN0IHJlc29sdmUgYW55 IG9mIHRoZSBzcGVjaWFsIFNjcmlwdCBwcm9wZXJ0eSB2YWx1ZXMsIHN1Y2ggYXMgQ29tbW9uLAor ICAgICAgICAvLyBiYXNlZCBvbiB0aGUgY29udGV4dCBvZiB0aGUgc3Vycm91bmRpbmcgY2hhcmFj dGVycy4gQSBzaW1wbGUgaGV1cmlzdGljIHVzZXMgdGhlCisgICAgICAgIC8vIHNjcmlwdCBvZiB0 aGUgcHJlY2VkaW5nIGNoYXJhY3Rlciwgd2hpY2ggd29ya3Mgd2VsbCBpbiBtYW55IGNhc2VzLgor ICAgICAgICAvLyBodHRwOi8vd3d3LnVuaWNvZGUub3JnL3JlcG9ydHMvdHIyNC8jQ29tbW9uLgor ICAgICAgICAvLyBGSVhNRTogY292ZXIgYWxsIG90aGVyIGNhc2VzIG1lbnRpb25lZCBpbiB0aGUg c3BlYyAoaWUuIGJyYWNrZXRzIG9yIHF1b3RhdGlvbiBtYXJrcykuCisgICAgICAgIC8vIFdlIHVz ZSBhIHNsaWdodGx5IG1vcmUgY29uc2VydmF0aXZlIGhldXJpc3RpYyB0aGFuIHRoZSBvbmUgcHJv cG9zZWQgaW4gdGhlIHNwZWMsCisgICAgICAgIC8vIHVzaW5nIHRoZSBzY3JpcHQgb2YgdGhlIHBy ZXZpb3VzIGNoYXJhY3RlciBvbmx5IGlmIGJvdGggYXJlIEFTQ0lJLgorICAgICAgICBpZiAoaXNB U0NJSShjaGFyYWN0ZXIpICYmIGlzQVNDSUkocHJldmlvdXNDaGFyYWN0ZXIpKQorICAgICAgICAg ICAgcmV0dXJuIHRydWU7CisgICAgfQorCisgICAgcmV0dXJuIHVzY3JpcHRfaGFzU2NyaXB0KGNo YXJhY3RlciwgcHJldmlvdXNTY3JpcHQpOworfQorCiBib29sIEhhcmZCdXp6U2hhcGVyOjpjb2xs ZWN0SGFyZkJ1enpSdW5zKCkKIHsKICAgICBjb25zdCBVQ2hhciogbm9ybWFsaXplZEJ1ZmZlckVu ZCA9IG1fbm9ybWFsaXplZEJ1ZmZlci5nZXQoKSArIG1fbm9ybWFsaXplZEJ1ZmZlckxlbmd0aDsK QEAgLTM4MSw2ICs0MDUsNyBAQCBib29sIEhhcmZCdXp6U2hhcGVyOjpjb2xsZWN0SGFyZkJ1enpS dW5zKCkKICAgICAgICAgaWYgKCFjdXJyZW50Rm9udERhdGEpCiAgICAgICAgICAgICBjdXJyZW50 Rm9udERhdGEgPSAmbV9mb250LT5wcmltYXJ5Rm9udCgpOwogICAgICAgICBVU2NyaXB0Q29kZSBj dXJyZW50U2NyaXB0ID0gbmV4dFNjcmlwdDsKKyAgICAgICAgVUNoYXIzMiBwcmV2aW91c0NoYXJh Y3RlciA9IGNoYXJhY3RlcjsKIAogICAgICAgICBmb3IgKGl0ZXJhdG9yLmFkdmFuY2UoY2x1c3Rl ckxlbmd0aCk7IGl0ZXJhdG9yLmNvbnN1bWUoY2hhcmFjdGVyLCBjbHVzdGVyTGVuZ3RoKTsgaXRl cmF0b3IuYWR2YW5jZShjbHVzdGVyTGVuZ3RoKSkgewogICAgICAgICAgICAgaWYgKEZvbnRDYXNj YWRlOjp0cmVhdEFzWmVyb1dpZHRoU3BhY2UoY2hhcmFjdGVyKSkKQEAgLTQxMCwxMSArNDM1LDE4 IEBAIGJvb2wgSGFyZkJ1enpTaGFwZXI6OmNvbGxlY3RIYXJmQnV6elJ1bnMoKQogICAgICAgICAg ICAgbmV4dFNjcmlwdCA9IHVzY3JpcHRfZ2V0U2NyaXB0KGNoYXJhY3RlciwgJmVycm9yQ29kZSk7 CiAgICAgICAgICAgICBpZiAoVV9GQUlMVVJFKGVycm9yQ29kZSkpCiAgICAgICAgICAgICAgICAg cmV0dXJuIGZhbHNlOwotICAgICAgICAgICAgaWYgKChuZXh0Rm9udERhdGEgIT0gY3VycmVudEZv bnREYXRhKSB8fCAoKGN1cnJlbnRTY3JpcHQgIT0gbmV4dFNjcmlwdCkgJiYgKG5leHRTY3JpcHQg IT0gVVNDUklQVF9JTkhFUklURUQpICYmICghdXNjcmlwdF9oYXNTY3JpcHQoY2hhcmFjdGVyLCBj dXJyZW50U2NyaXB0KSkpKQorCisgICAgICAgICAgICBpZiAobmV4dEZvbnREYXRhICE9IGN1cnJl bnRGb250RGF0YSkKKyAgICAgICAgICAgICAgICBicmVhazsKKworICAgICAgICAgICAgaWYgKCFz Y3JpcHRzQXJlQ29tcGF0aWJsZUZvckNoYXJhY3RlcnMobmV4dFNjcmlwdCwgY3VycmVudFNjcmlw dCwgY2hhcmFjdGVyLCBwcmV2aW91c0NoYXJhY3RlcikpCiAgICAgICAgICAgICAgICAgYnJlYWs7 Ci0gICAgICAgICAgICBpZiAobmV4dFNjcmlwdCA9PSBVU0NSSVBUX0lOSEVSSVRFRCkKKworICAg ICAgICAgICAgaWYgKG5leHRTY3JpcHQgPT0gVVNDUklQVF9JTkhFUklURUQgfHwgbmV4dFNjcmlw dCA9PSBVU0NSSVBUX0NPTU1PTikKICAgICAgICAgICAgICAgICBuZXh0U2NyaXB0ID0gY3VycmVu dFNjcmlwdDsKKwogICAgICAgICAgICAgY3VycmVudENoYXJhY3RlclBvc2l0aW9uID0gaXRlcmF0 b3IuY2hhcmFjdGVycygpOworICAgICAgICAgICAgcHJldmlvdXNDaGFyYWN0ZXIgPSBjaGFyYWN0 ZXI7CiAgICAgICAgIH0KICAgICAgICAgdW5zaWduZWQgbnVtQ2hhcmFjdGVyc09mQ3VycmVudFJ1 biA9IGl0ZXJhdG9yLmN1cnJlbnRJbmRleCgpIC0gc3RhcnRJbmRleE9mQ3VycmVudFJ1bjsKICAg ICAgICAgaGJfc2NyaXB0X3Qgc2NyaXB0ID0gaGJfaWN1X3NjcmlwdF90b19zY3JpcHQoY3VycmVu dFNjcmlwdCk7Cg==