Bug 144670 – Toggling underline on font panel removes strike through

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

On Mail, toggling underline on font panel removes strike through and toggling strike through removes underline.

[Ryosuke Niwa]

Created attachment 252453 [details]
Fixes the bug

[Ryosuke Niwa]

<rdar://problem/3790443>

[Darin Adler]

Comment on attachment 252453 [details]
Fixes the bug

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

> Source/WebKit/mac/WebView/WebHTMLView.mm:5052
> +    auto editingStyle = EditingStyle::create(core(style));

This approach, where we express things with EditingStyle that we can’t express with a CSSStyleDeclaration, is a much cleaner way to add editing operations like the one that makes text larger and smaller that we implemented using a strange CSS style property named "font size delta". There is a downside with this approach, though, and that is that WebKit’s -[WebView applyStyle:] can’t be used to perform these editing operations and also that CSSStyleDeclaration can’t be used to communicate what’s going on to the editing delegate method webView:shouldApplyStyle:toElementsInDOMRange: either, although I’m not sure exactly how that works.

It’s not great that we have one approach for "font size delta" and a different approach for these new operations. This is something we should come back to and improve at some point, although I am not sure it’s worth the time to do further refinement on this right now.

> Source/WebKit/mac/WebView/WebHTMLView.mm:5059
> +    int strikeThroughA = [[a objectForKey:NSStrikethroughStyleAttributeName] intValue];
> +    int strikeThroughB = [[b objectForKey:NSStrikethroughStyleAttributeName] intValue];
> +    if (strikeThroughA == strikeThroughB) {
> +        bool shouldRemoveStrikeThrough = strikeThroughA == NSUnderlineStyleNone;
> +        editingStyle->setStrikeThroughChange(shouldRemoveStrikeThrough ? TextDecorationChange::Remove : TextDecorationChange::Add);
> +    }

Cocoa names use a single word for “strikethrough”; our WebKit names use two words “strike through”. It would be nicer to be consistent.

[WebKit Commit Bot]

Comment on attachment 252453 [details]
Fixes the bug

Clearing flags on attachment: 252453

Committed r183871: <http://trac.webkit.org/changeset/183871>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

(In reply to comment #3)
> Comment on attachment 252453 [details]
> Fixes the bug
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=252453&action=review
> 
> > Source/WebKit/mac/WebView/WebHTMLView.mm:5052
> > +    auto editingStyle = EditingStyle::create(core(style));
> 
> This approach, where we express things with EditingStyle that we can’t
> express with a CSSStyleDeclaration, is a much cleaner way to add editing
> operations like the one that makes text larger and smaller that we
> implemented using a strange CSS style property named "font size delta".
> There is a downside with this approach, though, and that is that WebKit’s
> -[WebView applyStyle:] can’t be used to perform these editing operations and
> also that CSSStyleDeclaration can’t be used to communicate what’s going on
> to the editing delegate method
> webView:shouldApplyStyle:toElementsInDOMRange: either, although I’m not sure
> exactly how that works.
> 
> It’s not great that we have one approach for "font size delta" and a
> different approach for these new operations. This is something we should
> come back to and improve at some point, although I am not sure it’s worth
> the time to do further refinement on this right now.

Oh, we already use EditingStyle to deal with font delta. We just haven't hooked up WebKit layer to set it manually. So right now, WebKit sets -webkit-font-size-delta on CSSStyleDeclaration and then we'll immeditely parse it into m_fontSizeDelta in EditingStyle's constructor in WebCore.

It'll be a nice cleanup to do.