Bug 17594 – queryCommandState returns false for Underline command when no selection is made

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

[Daniel Ruspini]

Rich text editor implementations typically want to highlight which command is being used, such as Bold and Underline.  In WebKit most are done correctly (Bold) when there is no selection in the text editor made, i.e. empty textarea and user clicks Bold, the Bold button highlights.  But with Underline, when there is no selection, the queryCommandState returns false.

[Daniel Ruspini]

Should also note that this works as expected in IE and FF browsers

[Alessandro Zifiglio]

Created attachment 63461 [details]
underline/strikethrough not working as expected when there is no text selected selected.

You can note from the attachment included the manifestation of the bug. This is also only half the problem. The other half consists of having the document in underline/strikethrough and bold/italic state. when this condition is met, you cannot toggle back underline/strikethrough without first toggling back bold/italic. This is easy to reproduce.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
Total errors found: 0 in 0 files


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

[Alessandro Zifiglio]

when there is no text selected and an execCommand for underline is performed, immediately after, executing queryCommandState('underline') returns false. However the document is in the underline state because if you type you can see the results are underlined. You can observe the same behavior when doing an execCommand on strikethrough as well.

This is inconsistent behavior and : 
1) it's inconsistent with the bold/italic commands because those work as expected under these conditions
2) when you have performed execCommand on underline/strikethrough and then furhter performing an execCommand on bold/italic, you cannot toggle back underline/strikethrough, without toggling back bold/italic first. So as you can note, it gets even more worse and I cannot think of a simple workaround to fix this problem on my end.
3)This works just fine on all other browsers that support contenteditable.

Infact if you login to gmail on google chrome or safari, clicking underline in the editor without selecting text shows this problem.

[Alessandro Zifiglio]

Created attachment 63507 [details]
underline/strikethrough not working as expected when there is no text selected selected

[Ryosuke Niwa]

(In reply to comment #4)
> when there is no text selected and an execCommand for underline is performed, immediately after, executing queryCommandState('underline') returns false.

This is correct behavior because if you do execCommand('underline', false, null) inside a underlined content, then we remove the underline from the typing style.  If you typed text without moving caret or clicking anywhere, you should be able to text without underline.

>However the document is in the underline state because if you type you can see the results are underlined. You can observe the same behavior when doing an execCommand on strikethrough as well.

I don't think this is an accurate description.  Yes, you do get underlined text if you moved the caret or clicked some other place (even the same location) to type text.  But that's consistent with other browsers such as Firefox.

> This is inconsistent behavior and : 
> 1) it's inconsistent with the bold/italic commands because those work as expected under these conditions
> 2) when you have performed execCommand on underline/strikethrough and then furhter performing an execCommand on bold/italic, you cannot toggle back underline/strikethrough, without toggling back bold/italic first. So as you can note, it gets even more worse and I cannot think of a simple workaround to fix this problem on my end.

I think you're encountering the bug 44560, which has been fixed recently.

> 3)This works just fine on all other browsers that support contenteditable.

I don't really understand what kind of problems you're experiencing.  As far as I checked the test cases on this page, WebKit (TOT) is behaving exactly as I expect it to behave.

[Alessandro Zifiglio]

(In reply to comment #6)
Hello Ryosuke, thanks for responding. I've already included a test case. You can test that the behavior is not the same on firefox, ie and opera where it works correctly. All you need to do is click in the yellow area and type. 

Clicking in the yellow area toggles underline. when queryCommandState('underline') is supposed to return true, it returns false and vice versa ( only in webkit ).

In addition, you can easily test this in gmails editor and how it behaves incorrectly when toggling underline. This incorrect behavior happens only in webkit.

Regards,

Alessandro
 
> (In reply to comment #4)
> > when there is no text selected and an execCommand for underline is performed, immediately after, executing queryCommandState('underline') returns false.
> 
> This is correct behavior because if you do execCommand('underline', false, null) inside a underlined content, then we remove the underline from the typing style.  If you typed text without moving caret or clicking anywhere, you should be able to text without underline.
> 
> >However the document is in the underline state because if you type you can see the results are underlined. You can observe the same behavior when doing an execCommand on strikethrough as well.
> 
> I don't think this is an accurate description.  Yes, you do get underlined text if you moved the caret or clicked some other place (even the same location) to type text.  But that's consistent with other browsers such as Firefox.
> 
> > This is inconsistent behavior and : 
> > 1) it's inconsistent with the bold/italic commands because those work as expected under these conditions
> > 2) when you have performed execCommand on underline/strikethrough and then furhter performing an execCommand on bold/italic, you cannot toggle back underline/strikethrough, without toggling back bold/italic first. So as you can note, it gets even more worse and I cannot think of a simple workaround to fix this problem on my end.
> 
> I think you're encountering the bug 44560, which has been fixed recently.
> 
> > 3)This works just fine on all other browsers that support contenteditable.
> 
> I don't really understand what kind of problems you're experiencing.  As far as I checked the test cases on this page, WebKit (TOT) is behaving exactly as I expect it to behave.

[Ryosuke Niwa]

Created attachment 67872 [details]
demo

In this demo, the typing style after execCommand('underline') is incorrectly reported not to have underline.

[Ryosuke Niwa]

I finally got what you are talking about.  I was focusing too much in the toggling part but you were talking about the typing style.

[Ryosuke Niwa]

Created attachment 67875 [details]
work in progress

The problem was that selectionComputedStyle just set the style attribute of a dummy node to the typing style even though only -webkit-text-decorations-in-effect had the actual value for the text-decoration.  We should be calling prepareEditingStyleAt to begin with because there is no point in applying non-editing styles here.  Hopefully I can fix this bug tomorrow.

[Ryosuke Niwa]

(In reply to comment #10)
> Created an attachment (id=67875) [details]
> work in progress
> 
> The problem was that selectionComputedStyle just set the style attribute of a dummy node to the typing style even though only -webkit-text-decorations-in-effect had the actual value for the text-decoration.  We should be calling prepareEditingStyleAt to begin with because there is no point in applying non-editing styles here.  Hopefully I can fix this bug tomorrow.

Oops, my current approach won't work in the case of removing underline because if selectionComputedStyle adds a node inside underlined element, no matter what text-decoration has, it'll always be underlined.  We need to avoid inserting a node in selectionComputedStyle when adding a typing style.

[Ryosuke Niwa]

Created attachment 67935 [details]
Patch

[Ryosuke Niwa]

Darin, it'll be great if you could review this patch since you seem to be the original author of selectionComputedStyle according to http://trac.webkit.org/changeset/6808.

[Darin Adler]

Comment on attachment 67935 [details]
Patch

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

This change seems OK, and I am going to say r=me. But I am concerned that we are missing some edge case. It’s great that this fixes underlining, but I am not sure that merging two style objects captures the same behavior that inheritance does. Adding only one test case worries me. I suspect this will cause regressions.

> WebCore/ChangeLog:13
> +        Fixed the bug by making selectionComputedStyle directly marge the computed style of the current

Typo here: "marge".

> WebCore/editing/ApplyStyleCommand.cpp:391
> -RefPtr<CSSMutableStyleDeclaration> getPropertiesNotInComputedStyle(CSSStyleDeclaration* style, CSSComputedStyleDeclaration* computedStyle)
> +RefPtr<CSSMutableStyleDeclaration> getPropertiesNotIn(CSSStyleDeclaration* style, CSSStyleDeclaration* computedStyle)

If this argument is allowed to be something other than a computed styled, then computedStyle seems like a bad name for the argument.

> WebCore/editing/ApplyStyleCommand.h:129
> -RefPtr<CSSMutableStyleDeclaration> getPropertiesNotInComputedStyle(CSSStyleDeclaration* style, CSSComputedStyleDeclaration* computedStyle);
> +RefPtr<CSSMutableStyleDeclaration> getPropertiesNotIn(CSSStyleDeclaration* style, CSSStyleDeclaration* computedStyle);

Same comment about the computed style argument name.

The function is now easy to use wrong because both arguments have the same type.

> WebCore/editing/Editor.cpp:865
> -static TriState triStateOfStyleInComputedStyle(CSSStyleDeclaration* desiredStyle, CSSComputedStyleDeclaration* computedStyle, bool ignoreTextOnlyProperties = false)
> +static TriState triStateOfStyleInComputedStyle(CSSStyleDeclaration* desiredStyle, CSSStyleDeclaration* computedStyle, bool ignoreTextOnlyProperties = false)

Same comment about the computed style argument name. But also, it’s not appropriate to have ComputedStyle in the function name given the change.

> WebCore/editing/Editor.cpp:3264
> +    mutableStyle->merge(typingStyle.get());

Is a merge function call here really the same thing as inheriting style? I assume the old code must have handled at least one case differently from calling merge.

[Ryosuke Niwa]

(In reply to comment #14)
> (From update of attachment 67935 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67935&action=review
> 
> This change seems OK, and I am going to say r=me. But I am concerned that we are missing some edge case. It’s great that this fixes underlining, but I am not sure that merging two style objects captures the same behavior that inheritance does. Adding only one test case worries me. I suspect this will cause regressions.

It doesn't.  I thought about it for a while but I couldn't think of the situation where you have non-inheritable typing style and does not want to include that style in selectionComputedStyle.  If there is such a case, I will gladly fix my patch.

> > WebCore/ChangeLog:13
> > +        Fixed the bug by making selectionComputedStyle directly marge the computed style of the current
> 
> Typo here: "marge".

Oops, will fix.

> > WebCore/editing/ApplyStyleCommand.cpp:391
> > -RefPtr<CSSMutableStyleDeclaration> getPropertiesNotInComputedStyle(CSSStyleDeclaration* style, CSSComputedStyleDeclaration* computedStyle)
> > +RefPtr<CSSMutableStyleDeclaration> getPropertiesNotIn(CSSStyleDeclaration* style, CSSStyleDeclaration* computedStyle)
> 
> If this argument is allowed to be something other than a computed styled, then computedStyle seems like a bad name for the argument.

Right, I should have changed that as well.

> > WebCore/editing/ApplyStyleCommand.h:129
> > -RefPtr<CSSMutableStyleDeclaration> getPropertiesNotInComputedStyle(CSSStyleDeclaration* style, CSSComputedStyleDeclaration* computedStyle);
> > +RefPtr<CSSMutableStyleDeclaration> getPropertiesNotIn(CSSStyleDeclaration* style, CSSStyleDeclaration* computedStyle);
> 
> Same comment about the computed style argument name.
> 
> The function is now easy to use wrong because both arguments have the same type.

Yeah, we probably need to rename it something like styleWithRedundantProperties (or simply style) and baseStyle.  Do they sound good to you?

> > WebCore/editing/Editor.cpp:865
> > -static TriState triStateOfStyleInComputedStyle(CSSStyleDeclaration* desiredStyle, CSSComputedStyleDeclaration* computedStyle, bool ignoreTextOnlyProperties = false)
> > +static TriState triStateOfStyleInComputedStyle(CSSStyleDeclaration* desiredStyle, CSSStyleDeclaration* computedStyle, bool ignoreTextOnlyProperties = false)
> 
> Same comment about the computed style argument name. But also, it’s not appropriate to have ComputedStyle in the function name given the change.

Oops, right.  Will change it to something like triStateOfStyle.

> > WebCore/editing/Editor.cpp:3264
> > +    mutableStyle->merge(typingStyle.get());
> 
> Is a merge function call here really the same thing as inheriting style? I assume the old code must have handled at least one case differently from calling merge.

No, as noted above.

[Ryosuke Niwa]

Created attachment 68326 [details]
avoids merging inheritable properties per Darin's comment

[Ryosuke Niwa]

(In reply to comment #16)
> Created an attachment (id=68326) [details]
> avoids merging inheritable properties per Darin's comment

Darin, I added ApplyStyleCommand::removeNonEditingProperties (maybe better to call it getEditingProperties?) and removed all non-inheritable properties before merging to address your concern.

[WebKit Review Bot]

Attachment 68326 [details] did not build on win:
Build output: http://queues.webkit.org/results/3995096

[Ryosuke Niwa]

Committed r68059: <http://trac.webkit.org/changeset/68059>