Bug 94510 – Inspector Bar does not reflect selected font styles correctly, when multiple <div>s are selected.

• Status: UNCONFIRMED
• Component: HTML Editing

[Alice Cheng]

1) Type multiple paragraphs of text while bold is selected. Such as the following:

"First line
Second line

Third line"

2) Select the text you have just typed and notice that the B[old] formatting button is greyed out.
3) Become annoyed when you realize that in order to remove the bold formatting, you must select each paragraph  of text and individually remove the formatting from it.

[Alice Cheng]

<rdar://problem/11745163>

[Alice Cheng]

Created attachment 159487 [details]
proposed patch

A proposed patch to fix the problem.

Inspector Bar were not reflecting the selected font styles correctly, because new line characters got their styles from <div> renderers, which do not reflect their children's font styles correctly. Thus, we merge styles for new line characters for NSAttributedString.

A TestWebkitAPI test was also included to test if the patch works.

[Darin Adler]

Comment on attachment 159487 [details]
proposed patch

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

Why are we patching up the attributed string after the fact rather than making the style correct when we originally construct the string? It seems strange to “patch around” this bug instead of fixing it.

> Source/WebKit/mac/WebView/WebHTMLView.mm:5724
> +static NSAttributedString * mergeNewLineCharAttributes(NSAttributedString *string)

There should be no space after the * before the word merge.

The name of thus function is not good. This function doesn’t “merge” anything. I believe what it does is to overwrite newline character attributes with the attributes of the previous character. Also, a function that returns a value should not be given a name that is a “verb”. It should be a noun. Maybe stringWithNewlineAttributesFixed would be a good name, even though “fixed” is not a clear concept.

This function needs a comment explaining why it does what it does. It’s good that the change log explains, but the code needs to explain too, because it’s non-obvious why this would be a good thing to do.

> Source/WebKit/mac/WebView/WebHTMLView.mm:5726
> +    NSMutableAttributedString *attributedString = [[NSMutableAttributedString alloc] init];

This algorithm looks quite inefficient. We're copying all sorts of substrings. We’re not even changing the string, so building an entirely new attributed string is not right. If we do need to do this, rather than making the style correct when we originally construct the string, we should make a mutable copy of the attributed string and then modify only the attributes, leaving the textual part of the string alone.

Use of the name attributedString for the new one and the name string for the original one is unnecessarily confusing. Those are two different type names, and yet both are attributed strings. The names need to make clear the difference between these two rather than being arbitrarily different in a way that does not make clear what’s different.

> Source/WebKit/mac/WebView/WebHTMLView.mm:5728
> +    NSUInteger firstNoneNewLineCharIndex = 0;

In this variable name it should be NonNewline rather than NoneNewLine.

> Source/WebKit/mac/WebView/WebHTMLView.mm:5733
> +        return string;

We leak attributedString here.

> Source/WebKit/mac/WebView/WebHTMLView.mm:5752
> +    return attributedString;

This result will leak. We need to call autorelease on it.

[Alice Cheng]

Comment on attachment 159487 [details]
proposed patch

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

Thanks for your review =) I did not modify HTMLConverter directly, because I thought it might be too risky. Let me know if it would not be too risky to just not produce new line characters in HTMLConverter::editingAttributedStringFromRange. Or maybe do the merge over there?

>> Source/WebKit/mac/WebView/WebHTMLView.mm:5724
>> +static NSAttributedString * mergeNewLineCharAttributes(NSAttributedString *string)
> 
> There should be no space after the * before the word merge.
> 
> The name of thus function is not good. This function doesn’t “merge” anything. I believe what it does is to overwrite newline character attributes with the attributes of the previous character. Also, a function that returns a value should not be given a name that is a “verb”. It should be a noun. Maybe stringWithNewlineAttributesFixed would be a good name, even though “fixed” is not a clear concept.
> 
> This function needs a comment explaining why it does what it does. It’s good that the change log explains, but the code needs to explain too, because it’s non-obvious why this would be a good thing to do.

fixed. Changed the name for the function. Added proper comments.

>> Source/WebKit/mac/WebView/WebHTMLView.mm:5726
>> +    NSMutableAttributedString *attributedString = [[NSMutableAttributedString alloc] init];
> 
> This algorithm looks quite inefficient. We're copying all sorts of substrings. We’re not even changing the string, so building an entirely new attributed string is not right. If we do need to do this, rather than making the style correct when we originally construct the string, we should make a mutable copy of the attributed string and then modify only the attributes, leaving the textual part of the string alone.
> 
> Use of the name attributedString for the new one and the name string for the original one is unnecessarily confusing. Those are two different type names, and yet both are attributed strings. The names need to make clear the difference between these two rather than being arbitrarily different in a way that does not make clear what’s different.

fixed. Left alone its textual part. Changed naming for two strings

>> Source/WebKit/mac/WebView/WebHTMLView.mm:5728
>> +    NSUInteger firstNoneNewLineCharIndex = 0;
> 
> In this variable name it should be NonNewline rather than NoneNewLine.

fixed

>> Source/WebKit/mac/WebView/WebHTMLView.mm:5733
>> +        return string;
> 
> We leak attributedString here.

fixed

>> Source/WebKit/mac/WebView/WebHTMLView.mm:5752
>> +    return attributedString;
> 
> This result will leak. We need to call autorelease on it.

fixed

[Alice Cheng]

Created attachment 159809 [details]
proposed patch

[Ryosuke Niwa]

Comment on attachment 159809 [details]
proposed patch

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

> Source/WebKit/mac/WebView/WebHTMLView.mm:5725
> +// Inspector Bar did not reflect selected font styles correctly, because new line characters got their attributes from <div> renderers, which do not refelect their children's attributes correctly. <rdar://problem/11745163> Thus we overwrite newline character attributes with the attributes of the previous character here
> +static NSAttributedString *stringWithNewLineAttributesFixed(NSAttributedString *oldString)

Did you look at EditingStyle::triStateOfStyle? It seems like what you're trying to do is very similar.
Maybe there's a way to share some code there.

[Andreas Kling]

Comment on attachment 159809 [details]
proposed patch

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.