Bug 146379 – Font panel doesn't get updated when bolding text via cmd+b in Mail on OS X

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

[Ryosuke Niwa]

We need to call _updateFontPanel when applying style in addition to when selection is changed.
We're also failing to update underline/strike-through state because we wrongfuly rely on -webkit-text-decorations-in-effect to affect RenderStyle.

[Ryosuke Niwa]

<rdar://problem/21409729>

[Ryosuke Niwa]

Created attachment 255695 [details]
Fixes the bug

[Build Bot]

Comment on attachment 255695 [details]
Fixes the bug

Attachment 255695 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5785875271122944

New failing tests:
editing/style/unbold-in-bold.html

[Build Bot]

Created attachment 255697 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Ryosuke Niwa]

(In reply to comment #3)
> Comment on attachment 255695 [details]
> Fixes the bug
> 
> Attachment 255695 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.appspot.com/results/5785875271122944
> 
> New failing tests:
> editing/style/unbold-in-bold.html

I don't think this failure is related to my change:
--- /Volumes/Data/EWS/WebKit/WebKitBuild/Release/layout-test-results/editing/style/unbold-in-bold-expected.txt
+++ /Volumes/Data/EWS/WebKit/WebKitBuild/Release/layout-test-results/editing/style/unbold-in-bold-actual.txt
@@ -86,5 +86,6 @@
           RenderText {#text} at (164,14) size 78x28
             text run at (164,14) width 78: " xxxxxx"
         RenderInline {SPAN} at (0,0) size 0x28
+      RenderBlock (anonymous) at (0,56) size 784x0
 selection start: position 0 of child 1 {#text} of child 1 {DIV} of body
 selection end:   position 6 of child 1 {#text} of child 1 {DIV} of body

The test also passes locally.

[Ryosuke Niwa]

Created attachment 255702 [details]
Fixed iOS build

[Ryosuke Niwa]

Created attachment 255703 [details]
Reverted irrelevant changes

[WebKit Commit Bot]

Attachment 255703 [details] did not pass style-queue:


ERROR: Source/WebCore/editing/cocoa/EditorCocoa.h:36:  NS_ENUM_AVAILABLE_IOS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/editing/cocoa/EditorCocoa.h:37:  NS_ENUM_AVAILABLE_IOS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/editing/cocoa/EditorCocoa.h:39:  NS_ENUM_AVAILABLE_IOS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/editing/cocoa/EditorCocoa.h:40:  NS_ENUM_AVAILABLE_IOS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/editing/cocoa/EditorCocoa.h:41:  NS_ENUM_AVAILABLE_IOS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/editing/cocoa/EditorCocoa.h:42:  NS_ENUM_AVAILABLE_IOS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/editing/cocoa/EditorCocoa.h:43:  NS_ENUM_AVAILABLE_IOS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/editing/cocoa/EditorCocoa.h:45:  NS_ENUM_AVAILABLE_IOS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 8 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 255703 [details]
Reverted irrelevant changes

Attachment 255703 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6697960163246080

New failing tests:
editing/style/unbold-in-bold.html

[Build Bot]

Created attachment 255705 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Darin Adler]

Comment on attachment 255703 [details]
Reverted irrelevant changes

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

Great patch! I don’t think it builds and passes tests yet on EWS but I have many small suggestions about it.

> Source/WebCore/editing/Editor.cpp:951
> -    // FIXME: This is wrong for text decorations since m_mutableStyle is empty.
>      if (!client() || !client()->shouldApplyStyle(style->styleWithResolvedTextDecorations().ptr(), m_frame.selection().toNormalizedRange().get()))
>          return;
> -
>      applyStyle(WTF::move(style), editingAction);

This change is only the deleting of the comment and whitespace. I don’t mind that, but it could be landed separately.

> Source/WebCore/editing/Editor.cpp:961
> -    if (client() && client()->shouldApplyStyle(style, m_frame.selection().toNormalizedRange().get()))
> -        applyParagraphStyle(style, editingAction);
> +    if (!client() || !client()->shouldApplyStyle(style, m_frame.selection().toNormalizedRange().get()))
> +        return;
> +    applyParagraphStyle(style, editingAction);

This change is a small refactoring only. I don’t mind that, but it could be landed separately.

> Source/WebCore/editing/cocoa/EditorCocoa.h:2
> + * Copyright (C) 2006, 2007, 2008, 2013, 2015 Apple Inc. All rights reserved.

I don’t think this set of NSUnderlineStyle constants has that set of years of copyright.

> Source/WebCore/editing/cocoa/EditorCocoa.h:46
> +typedef NS_ENUM(NSInteger, NSUnderlineStyle) {
> +    NSUnderlineStyleNone                                = 0x00,
> +    NSUnderlineStyleSingle                              = 0x01,
> +    NSUnderlineStyleThick NS_ENUM_AVAILABLE_IOS(7_0)    = 0x02,
> +    NSUnderlineStyleDouble NS_ENUM_AVAILABLE_IOS(7_0)   = 0x09,
> +    
> +    NSUnderlinePatternSolid NS_ENUM_AVAILABLE_IOS(7_0)      = 0x0000,
> +    NSUnderlinePatternDot NS_ENUM_AVAILABLE_IOS(7_0)        = 0x0100,
> +    NSUnderlinePatternDash NS_ENUM_AVAILABLE_IOS(7_0)       = 0x0200,
> +    NSUnderlinePatternDashDot NS_ENUM_AVAILABLE_IOS(7_0)    = 0x0300,
> +    NSUnderlinePatternDashDotDot NS_ENUM_AVAILABLE_IOS(7_0) = 0x0400,
> +    
> +    NSUnderlineByWord NS_ENUM_AVAILABLE_IOS(7_0) = 0x8000
> +};

Peculiar to have this in a header named EditorCocoa.h. It would be more logical for this to be in some file with a name that ends in SPI.h.

> Source/WebCore/editing/cocoa/EditorCocoa.mm:34
> +#import "config.h"
> +#import "Editor.h"
> +
> +#import "EditingStyle.h"
> +#import "Frame.h"
> +#import "FrameSelection.h"
> +#import "RenderElement.h"
> +#import "RenderStyle.h"
> +#import "Text.h"

Forgot to include EditorCocoa.h here.

> Source/WebCore/editing/cocoa/EditorCocoa.mm:69
> +void Editor::getTextDecorationAttributesRespectingTypingStyle(RenderStyle* style, NSMutableDictionary* result) const

Seems like we should write a function that returns textDecorationsInEffect considering typing style. This function could call that one and would then be simpler.

Since this function gets the typing style from the frame, it’s strange that the non-typing style is passed in as a RenderStyle*. I suggest not having that argument at all and getting the style in this function, even though, sadly, the callers all seem to already have it. If we do keep it, then I suggest making it a RenderStyle&. If we really do want to pass in the RenderStyle, then I think we should consider also passing in the typing style. Or changing the name of the argument. It’s really unclear that sometimes the function will just ignore that argument entirely!

> Source/WebCore/editing/cocoa/EditorCocoa.mm:77
> +        String value = typingStyle->style()->getPropertyValue(CSSPropertyWebkitTextDecorationsInEffect);
> +        if (value.contains("line-through"))
> +            [result setObject:[NSNumber numberWithInt:NSUnderlineStyleSingle] forKey:NSStrikethroughStyleAttributeName];
> +        if (value.contains("underline"))
> +            [result setObject:[NSNumber numberWithInt:NSUnderlineStyleSingle] forKey:NSUnderlineStyleAttributeName];

This use of contains is peculiar. I know the existing code was doing it, but it seems really roundabout to convert this style to a string and then look at the string, rather than looking more directly at the parsed style data. Or maybe this is only available in string form?

> Source/WebKit2/ChangeLog:8
> +        Since font panel doesn't open in WebKit2 at the moment, just add an empty implementation of didApplyStyle

The font panel doesn’t work at all with WebKit2? Really? I’m really surprised!

> Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:261
> +static void updateFontPanelIfNeeded(WebView* webView)

I think you should just name this updateFontPanel. Explicitly stating "if needed" doesn’t really add much to clarify what this function does, but it does make it harder to read the function name.

WebKit style says this should be WebView * with a space.

> Source/WebKit/mac/WebView/WebHTMLView.mm:5485
> +    NSDictionary* attributes = nil;

I suggest initializing this to @{ } rather than nil. I am not sure that -[NSFontManager setSelectedAttributes:isMultiple:] supports nil.

WebKit style says this should be NSDictionary * with a space, like the NSFont * on the line above.

> Source/WebKit/mac/WebView/WebHTMLView.mm:5489
> +        attributes = coreFrame->editor().fontAttributesForSelectionStart();

Can this ever return nil? If so, same consideration as above. Probably would like to pass an empty dictionary instead.

> Source/WebKit/mac/WebView/WebHTMLView.mm:5498
> +    NSFontManager* fontManager = [NSFontManager sharedFontManager];

WebKit style says this should be NSFontManager * with a space.

> Source/WebKit/win/ChangeLog:15
> +        * WebCoreSupport/WebEditorClient.cpp:
> +        (WebEditorClient::shouldChangeSelectedRange):
> +        (WebEditorClient::shouldApplyStyle):
> +        (WebEditorClient::didApplyStyle):
> +        (WebEditorClient::shouldMoveRangeAfterDelete):
> +        (WebEditorClient::shouldChangeTypingStyle):
> +        (WebEditorClient::webViewDidChangeTypingStyle):
> +        * WebCoreSupport/WebEditorClient.h:

Listing all those other functions doesn’t make this change log good. Change log should say that you are adding didTypingStyle.

> Source/WebKit/win/WebCoreSupport/WebEditorClient.cpp:351
> +void WebEditorClient::didApplyStyle(StyleProperties* /*style*/, Range* /*toElementsInDOMRange*/)
> +{
> +    notImplemented();
> +}

These commented out argument names don’t do us any good. Should omit them from this new class.

[Ryosuke Niwa]

Created attachment 255707 [details]
Fixed iOS build for real

[WebKit Commit Bot]

Attachment 255707 [details] did not pass style-queue:


ERROR: Source/WebCore/editing/cocoa/EditorCocoa.h:36:  NS_ENUM_AVAILABLE_IOS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/editing/cocoa/EditorCocoa.h:37:  NS_ENUM_AVAILABLE_IOS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/editing/cocoa/EditorCocoa.h:39:  NS_ENUM_AVAILABLE_IOS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/editing/cocoa/EditorCocoa.h:40:  NS_ENUM_AVAILABLE_IOS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/editing/cocoa/EditorCocoa.h:41:  NS_ENUM_AVAILABLE_IOS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/editing/cocoa/EditorCocoa.h:42:  NS_ENUM_AVAILABLE_IOS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/editing/cocoa/EditorCocoa.h:43:  NS_ENUM_AVAILABLE_IOS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/editing/cocoa/EditorCocoa.h:45:  NS_ENUM_AVAILABLE_IOS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 8 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 255707 [details]
Fixed iOS build for real

Please read all the comments I made on the last version of the patch. They almost all still apply to this version.

[Build Bot]

Comment on attachment 255707 [details]
Fixed iOS build for real

Attachment 255707 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6035365626380288

New failing tests:
editing/style/unbold-in-bold.html

[Build Bot]

Created attachment 255709 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Ryosuke Niwa]

Committed r186086: <http://trac.webkit.org/changeset/186086>