Bug 190289 – [macOS] Fix some font attribute conversion bugs in preparation for "Font > Styles…" support in WebKit2

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Wenson Hsieh]

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…"

[Wenson Hsieh]

...we can address these bugs in parallel.

[Wenson Hsieh]

Created attachment 351624 [details]
Patch

[Wenson Hsieh]

Created attachment 351625 [details]
Test tweak

[Radar WebKit Bug Importer]

<rdar://problem/45020807>

[Radar WebKit Bug Importer]

<rdar://problem/45020806>

[Wenson Hsieh]

Created attachment 351626 [details]
Add rdar to ChangeLog

[Ryosuke Niwa]

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 "â¦"?

[Wenson Hsieh]

(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.

[WebKit Commit Bot]

Comment on attachment 351626 [details]
Add rdar to ChangeLog

Clearing flags on attachment: 351626

Committed r236854: <https://trac.webkit.org/changeset/236854>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.