After fixing <https://bugs.webkit.org/show_bug.cgi?id=146381> locally (along with the "Styles…" and "Show Colors" options in the context menu), I noticed a couple of bugs in our font attribute conversion code: - Setting a font shadow with a blur radius of 0 and an offset of (X, Y) where X < 0 or Y < 0 does not work. - We hit a debug assertion when removing background text colors using the modal sheet presented via "Font > Styles…"
...we can address these bugs in parallel.
Created attachment 351624 [details] Patch
Created attachment 351625 [details] Test tweak
<rdar://problem/45020807>
<rdar://problem/45020806>
Created attachment 351626 [details] Add rdar to ChangeLog
Comment on attachment 351626 [details] Add rdar to ChangeLog View in context: https://bugs.webkit.org/attachment.cgi?id=351626&action=review Nice bug fixes! > Source/WebCore/ChangeLog:19 > + Currently, we bail from adding a font shadow if the shadow's offset is empty. However, valid shadow offsets may > + have negative dimensions, so a check for `isZero()` should be used instead. It's very strange / unintuitive that isEmpty() returns true for negative offsets... > Source/WebCore/ChangeLog:26 > + transparent color (this scenario is exercised when using "Font > Stylesâ¦" to specify a font style without a What's up with "â¦"?
(In reply to Ryosuke Niwa from comment #7) > Comment on attachment 351626 [details] > Add rdar to ChangeLog > > View in context: > https://bugs.webkit.org/attachment.cgi?id=351626&action=review > > Nice bug fixes! > > > Source/WebCore/ChangeLog:19 > > + Currently, we bail from adding a font shadow if the shadow's offset is empty. However, valid shadow offsets may > > + have negative dimensions, so a check for `isZero()` should be used instead. > > It's very strange / unintuitive that isEmpty() returns true for negative > offsets... Yeah... :/ I think this makes it easier than it should be to make this kind of mistake for FloatSizes that represent offsets, like this. > > > Source/WebCore/ChangeLog:26 > > + transparent color (this scenario is exercised when using "Font > Stylesâ¦" to specify a font style without a > > What's up with "â¦"? This is actually an ellipsis '…' character, but the Bugzilla patch review tool apparently displays this as "â¦". It looks normal outside of the Bugzilla patch review page.
Comment on attachment 351626 [details] Add rdar to ChangeLog Clearing flags on attachment: 351626 Committed r236854: <https://trac.webkit.org/changeset/236854>
All reviewed patches have been landed. Closing bug.