Bug 179787 – [WebKit2] Format > Bold and Format > Italic don't toggle between bold and italic style

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

Steps to reproduce:

1. Open MiniBrowser
2. Choose File > New WebKit2 Editor
3. Click inside the new window and type some text
4. Select some of the text
5. Choose Format > Font > Bold or Italic or Bigger

Results:
The font doesn’t change

Further steps:
6. Choose Format > Font > Show Fonts
7. Repeat step 5

Results:
The font changes

Further steps:
8. Choose some other font (different size and typeface) in the Fonts panel and close it
9. Open a new WebKit2 Editor window and repeat steps 3-5

Results:
The font changes but takes on properties of the font last selected in the now-closed Fonts panel

Notes:
-[WKWebView changeFont:] seems to be all about the Font panel, whereas -[WebView changeFont:] seems to rely more on the font manager.

<rdar://problem/35593389>

[Wenson Hsieh]

r235748 addresses most of this. Before r235748, all of the font-modifying menu items were no-ops; after r235748, B/I/U menu items work the first time, but fail to toggle B/I/U state.

I'll use this to track a followup.

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #2)
> r235748 addresses most of this. Before r235748, all of the font-modifying
> menu items were no-ops; after r235748, B/I/U menu items work the first time,
> but fail to toggle B/I/U state.

Correction: the problem is relevant to bold and italic, but underline works.

[Wenson Hsieh]

Created attachment 351404 [details]
Patch

Comment on attachment 351404 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351404&action=review

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2796
> +    if (m_page->isEditable() || (NSFontPanel.sharedFontPanelExists && NSFontPanel.sharedFontPanel.visible)) {

Can you explain the m_page->isEditable() test here? It seems like it might help with the steps in comment 0, but we’d still have the bug in a MiniBrowser window navigated to data:text/html,<div contenteditable>.

[Wenson Hsieh]

(In reply to mitz from comment #5)
> Comment on attachment 351404 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=351404&action=review
> 
> > Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2796
> > +    if (m_page->isEditable() || (NSFontPanel.sharedFontPanelExists && NSFontPanel.sharedFontPanel.visible)) {
> 
> Can you explain the m_page->isEditable() test here? It seems like it might
> help with the steps in comment 0, but we’d still have the bug in a
> MiniBrowser window navigated to data:text/html,<div contenteditable>.

Yeah, this patch isn't intended fix the bug for a stock WKWebView; only one that's been made editable via SPI, à la Mail.

I'm using the check for editability here as a cue that we should be aggressive in keeping NSFontManager up to date. An alternate solution I'd considered was just checking if we're in a richly editable area instead, which would fix this for a normal WKWebView as well.

Would you suggest this second approach?

(In reply to Wenson Hsieh from comment #6)
> (In reply to mitz from comment #5)
> > Comment on attachment 351404 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=351404&action=review
> > 
> > > Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2796
> > > +    if (m_page->isEditable() || (NSFontPanel.sharedFontPanelExists && NSFontPanel.sharedFontPanel.visible)) {
> > 
> > Can you explain the m_page->isEditable() test here? It seems like it might
> > help with the steps in comment 0, but we’d still have the bug in a
> > MiniBrowser window navigated to data:text/html,<div contenteditable>.
> 
> Yeah, this patch isn't intended fix the bug for a stock WKWebView; only one
> that's been made editable via SPI, à la Mail.
> 
> I'm using the check for editability here as a cue that we should be
> aggressive in keeping NSFontManager up to date. An alternate solution I'd
> considered was just checking if we're in a richly editable area instead,
> which would fix this for a normal WKWebView as well.
> 
> Would you suggest this second approach?

Assuming it makes WKWebView behave as well as WebView does, it seems reasonable.

[Wenson Hsieh]

Created attachment 351423 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 351423 [details]
Patch

Clearing flags on attachment: 351423

Committed r236770: <https://trac.webkit.org/changeset/236770>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.