Bug 27818 – Toggling underline or strike through affects each other

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

[Ryosuke Niwa]

Created attachment 33746 [details]
demonstrates the bug

Toggling text decorations by execCommand(underline) or execCommand(strikeThrough) would remove all other text decorations applied.
e.g. exeCommand(underline) to <u>hello <s>world</s></u> would produce hello world without s around world.

[Ryosuke Niwa]

Because we currently pass CSSMutableStyleDeclaration to ApplyStyleCommand, ApplyStyleCommand cannot tell whether we are removing some text decorations or adding them, or replacing all text decorations with new ones.

We either need to add new argument to ApplyStyleCommand, or add some member variables & functions to CSSMutableStyleDeclaration.  We could derive a new class from CSSMutableStyleDeclaration that stores those extra information & takes care of a lot of things for editing.

[Justin Garcia]

> We could derive a new class from CSSMutableStyleDeclaration that stores those extra 
> information & takes care of a lot of things for editing.

Without knowing what those other things are it's hard to say if it's worth mucking with CSSMutableStyleDeclaration.  I'd just add an argument to ApplyStyleCommand.

[Ryosuke Niwa]

This bug no longer blocks 23892.

[Ryosuke Niwa]

Created attachment 34862 [details]
my attempt to add parameter

This is my attempt to add parameter. It contains a lot of cleanup and bug fixes are that splitted into a separate patch. It currently causes some regression to occur and probably require 2-3 cleanup patches before being able to fix this bug. I'll add this patch for people who will work on this bug near future. The big idea is:

1. I added extra parameter to ApplyStyleCommand, which requires adding extra parameters to a bunch of functions in Editor.cpp
  a. ReplaceAllTextDecorations mode is what WebKit currently implements. We need to preserve the behavior if we're using this default mode
  b. RemoveTextDecoration mode will remove all text decoration styles that are PRESENT in the style passed to ApplyStyleCommand
       This is very tricky because the style we get is different from the style we apply. We have to be very careful about how we check the emptiness of style and how we add inline style
  c. AddTextDecoration mode will add new text decoration present in the style passed to ApplyStyleCommand. This is less tricker than RemoveTextDecoration but whenever we're adding style, we need to merge text decorations rather than replacing them.

2. I modified how we push-down text decorations to remove text-decorations. This is where we remove text-decoration from the selected nodes (we reapply the style later)  For RemoveTextDecoration, we shouldn't be removing any text decorations except the ones we're removing; i.e. we should remove text decorations present in the style passed to ApplyStyleCommand.  For AddTextDecoration, we shouldn't remove any text-decorations.

3. We need to change how we add style.  For RemoveTextDecoration, we should ignore text-decorations all together.  For AddTextDecoration, we need to merge text decorations.

[Ryosuke Niwa]

Created attachment 68185 [details]
work in progress

The same approach but this one works.  I need to add a test and probably split it into two patches.

[Ryosuke Niwa]

Created attachment 68345 [details]
work in progress 2 (works but needs a test)

[Ryosuke Niwa]

(In reply to comment #2)
> > We could derive a new class from CSSMutableStyleDeclaration that stores those extra 
> > information & takes care of a lot of things for editing.
> 
> Without knowing what those other things are it's hard to say if it's worth mucking with CSSMutableStyleDeclaration.  I'd just add an argument to ApplyStyleCommand.

We can put functions like getPropertiesNotInComputedStyle, editingStyleAtPosition, and prepareEditingStyleToApplyAt into this class.  We can even merge StyleChange with it.

[Ryosuke Niwa]

Created attachment 252032 [details]
WIP3

With this patch, editing/execCommand/merge-text-decoration-with-typing-style.html is the only failing test.

[Ryosuke Niwa]

Created attachment 252141 [details]
WIP4

Now passes all the tests but there are some edge cases we're not getting right still.
Will finish it tomorrow.

[Ryosuke Niwa]

Created attachment 252209 [details]
Fixes the bug

[Ryosuke Niwa]

<rdar://problem/3790443>

[Darin Adler]

Comment on attachment 252209 [details]
Fixes the bug

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

> Source/WebCore/editing/ApplyStyleCommand.cpp:1457
> +    if (const StyleProperties* styleToMerge = styleChange.cssStyle()) {

I would use auto here.

> Source/WebCore/editing/ApplyStyleCommand.cpp:1460
> +                Ref<EditingStyle> inlineStyle = EditingStyle::create(existingStyle);

I would use auto here.

> Source/WebCore/editing/EditingStyle.cpp:256
> +    bool matches(const Element& elem) const { return HTMLElementEquivalent::matches(elem) && elem.hasAttribute(m_attrName); }

Should expand "elem" to "element".

> Source/WebCore/editing/EditingStyle.cpp:336
>      : m_shouldUseFixedDefaultFontSize(false)
> +    , m_underlineChange(static_cast<unsigned>(TextDecorationChange::None))
> +    , m_strikeThroughChange(static_cast<unsigned>(TextDecorationChange::None))
>      , m_fontSizeDelta(NoFontDelta)

These should all be initialized in the class definition on the lines that define these data members. Then you wouldn’t have to call the EditingStyle() constructor from the other constructors.

> Source/WebCore/editing/EditingStyle.cpp:776
> +    if (!value || !is<CSSValueList>(*value))
> +        return nullptr;

This can be written:

    if (!is<CSSValueList(value.get())
        return nullptr;

I suggest doing it that way.

> Source/WebCore/editing/EditingStyle.cpp:901
>      const Vector<std::unique_ptr<HTMLElementEquivalent>>& HTMLElementEquivalents = htmlElementEquivalents();
>      for (size_t i = 0; i < HTMLElementEquivalents.size(); ++i) {
>          const HTMLElementEquivalent* equivalent = HTMLElementEquivalents[i].get();

Should use modern for loop.

> Source/WebCore/editing/EditingStyle.cpp:942
>      const Vector<std::unique_ptr<HTMLAttributeEquivalent>>& HTMLAttributeEquivalents = htmlAttributeEquivalents();
>      for (size_t i = 0; i < HTMLAttributeEquivalents.size(); ++i) {
> -        if (HTMLAttributeEquivalents[i]->matches(element) && HTMLAttributeEquivalents[i]->propertyExistsInStyle(m_mutableStyle.get())
> -            && !HTMLAttributeEquivalents[i]->valueIsPresentInStyle(element, m_mutableStyle.get()))
> +        if (HTMLAttributeEquivalents[i]->matches(*element)
> +            && HTMLAttributeEquivalents[i]->propertyExistsInStyle(*this)
> +            && !HTMLAttributeEquivalents[i]->valueIsPresentInStyle(*element, *this))
>              return true;
>      }

Can we make this a lot easier to read with a modern for loop?

    for (auto& equivalent : htmlAttributeEquivalents()) {
        if (equivalent->matches(*element) && equivalent->propertyExistsInStyle(*this) && !equivalent->valueIsPresentInStyle(*element, *this))
            return true;
    }

> Source/WebCore/editing/EditingStyle.cpp:1204
>      RefPtr<CSSPrimitiveValue> underline = cssValuePool().createIdentifierValue(CSSValueUnderline);
>      RefPtr<CSSPrimitiveValue> lineThrough = cssValuePool().createIdentifierValue(CSSValueLineThrough);

These can be Ref instead of RefPtr. MY preferred technique is to use auto. Then there would be a WTF::move below instead of a releaseNonNull.

> Source/WebCore/editing/EditingStyle.cpp:1554
> -    Document* document = position.anchorNode() ? &position.anchorNode()->document() : 0;
> -    if (!style || !style->style() || !document || !document->frame())
> +    Document* document = position.deprecatedNode() ? &position.deprecatedNode()->document() : 0;
> +    if (!style || style->isEmpty() || !document || !document->frame())
>          return;

That’s sad. Adding in a new use of deprecatedNode. Is there a way to avoid this?

> Source/WebCore/editing/EditingStyle.cpp:1575
> +        if (!value || !is<CSSValueList>(*value))

if (!is<CSSSValueList>(value.get())

> Source/WebCore/editing/EditingStyle.cpp:1579
> +        if (value && is<CSSValueList>(*value))

if (is<CSSSValueList>(value.get())

> Source/WebCore/editing/EditingStyle.h:127
>      bool conflictsWithInlineStyleOfElement(StyledElement* element) const { return conflictsWithInlineStyleOfElement(element, 0, 0); }

Should take a StyledElement&.

> Source/WebCore/editing/EditingStyle.h:128
> +    bool conflictsWithInlineStyleOfElement(StyledElement* element, RefPtr<MutableStyleProperties>& newInlineStyle,

Should take a StyledElement&.

> Source/WebCore/editing/EditingStyle.h:178
> +    bool conflictsWithInlineStyleOfElement(StyledElement*, RefPtr<MutableStyleProperties>* newInlineStyle, EditingStyle* extractedStyle) const;

Should take a StyledElement&.

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

Should write all the conditions as early returns.

> Source/WebCore/editing/EditorCommand.cpp:121
> +    // Mac: present at the beginning of selection
> +    // other: present throughout the selection

I think you should say "Mac" and "Windows" here rather than "Mac" and "other". Others are in both categories. Open source Unix systems tend to just match Windows but iOS, for example, matches Mac.

[Ryosuke Niwa]

Comment on attachment 252209 [details]
Fixes the bug

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

>> Source/WebCore/editing/ApplyStyleCommand.cpp:1457
>> +    if (const StyleProperties* styleToMerge = styleChange.cssStyle()) {
> 
> I would use auto here.

Done.

>> Source/WebCore/editing/ApplyStyleCommand.cpp:1460
>> +                Ref<EditingStyle> inlineStyle = EditingStyle::create(existingStyle);
> 
> I would use auto here.

Done.

>> Source/WebCore/editing/EditingStyle.cpp:256
>> +    bool matches(const Element& elem) const { return HTMLElementEquivalent::matches(elem) && elem.hasAttribute(m_attrName); }
> 
> Should expand "elem" to "element".

Fixed.

>> Source/WebCore/editing/EditingStyle.cpp:336
>>      , m_fontSizeDelta(NoFontDelta)
> 
> These should all be initialized in the class definition on the lines that define these data members. Then you wouldn’t have to call the EditingStyle() constructor from the other constructors.

Unfortunately we can't because they're bitfields :(
I've moved the initialziation of m_fontSizeDelta to the class declaration since it's a float.

>> Source/WebCore/editing/EditingStyle.cpp:776
>> +        return nullptr;
> 
> This can be written:
> 
>     if (!is<CSSValueList(value.get())
>         return nullptr;
> 
> I suggest doing it that way.

Fixed.

>> Source/WebCore/editing/EditingStyle.cpp:901
>>          const HTMLElementEquivalent* equivalent = HTMLElementEquivalents[i].get();
> 
> Should use modern for loop.

Fixed.

>> Source/WebCore/editing/EditingStyle.cpp:942
>>      }
> 
> Can we make this a lot easier to read with a modern for loop?
> 
>     for (auto& equivalent : htmlAttributeEquivalents()) {
>         if (equivalent->matches(*element) && equivalent->propertyExistsInStyle(*this) && !equivalent->valueIsPresentInStyle(*element, *this))
>             return true;
>     }

Fixed.

>> Source/WebCore/editing/EditingStyle.cpp:1204
>>      RefPtr<CSSPrimitiveValue> lineThrough = cssValuePool().createIdentifierValue(CSSValueLineThrough);
> 
> These can be Ref instead of RefPtr. MY preferred technique is to use auto. Then there would be a WTF::move below instead of a releaseNonNull.

Fixed.

>> Source/WebCore/editing/EditingStyle.cpp:1554
>>          return;
> 
> That’s sad. Adding in a new use of deprecatedNode. Is there a way to avoid this?

I'm simply replacing the incorrect use of anchorNode() here because it's used as a "deprecated" node.
I swear we used to have deprecatedNode here. I don't know who replaced it with anchorNode :(

>> Source/WebCore/editing/EditingStyle.cpp:1575
>> +        if (!value || !is<CSSValueList>(*value))
> 
> if (!is<CSSSValueList>(value.get())

Fixed.

>> Source/WebCore/editing/EditingStyle.cpp:1579
>> +        if (value && is<CSSValueList>(*value))
> 
> if (is<CSSSValueList>(value.get())

Fixed.

>> Source/WebCore/editing/EditingStyle.h:127
>>      bool conflictsWithInlineStyleOfElement(StyledElement* element) const { return conflictsWithInlineStyleOfElement(element, 0, 0); }
> 
> Should take a StyledElement&.

I'll do that in a separate patch to avoid making this patch even larger.

>> Source/WebCore/editing/EditingStyle.h:128
>> +    bool conflictsWithInlineStyleOfElement(StyledElement* element, RefPtr<MutableStyleProperties>& newInlineStyle,
> 
> Should take a StyledElement&.

Ditto.

>> Source/WebCore/editing/EditingStyle.h:178
>> +    bool conflictsWithInlineStyleOfElement(StyledElement*, RefPtr<MutableStyleProperties>* newInlineStyle, EditingStyle* extractedStyle) const;
> 
> Should take a StyledElement&.

Ditto.

>> Source/WebCore/editing/Editor.cpp:950
>> +        applyStyle(WTF::move(style), editingAction);
> 
> Should write all the conditions as early returns.

Done.

>> Source/WebCore/editing/EditorCommand.cpp:121
>> +    // other: present throughout the selection
> 
> I think you should say "Mac" and "Windows" here rather than "Mac" and "other". Others are in both categories. Open source Unix systems tend to just match Windows but iOS, for example, matches Mac.

Done.

[Ryosuke Niwa]

Created attachment 252326 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 252326 [details]
Patch for landing

Clearing flags on attachment: 252326

Committed r183770: <http://trac.webkit.org/changeset/183770>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

Two strikethrough tests are broken on Windows and Gtk. Ryosuke, can you take a look?

https://build.webkit.org/results/Apple%20Win%207%20Debug%20(Tests)/r183833%20(66056)/results.html

[Ryosuke Niwa]

(In reply to comment #17)
> Two strikethrough tests are broken on Windows and Gtk. Ryosuke, can you take
> a look?
> 
> https://build.webkit.org/results/Apple%20Win%207%20Debug%20(Tests)/
> r183833%20(66056)/results.html

Oops, these tests are expecting Mac editing behavior. Fixed the tests in http://trac.webkit.org/changeset/183848.

[Ryosuke Niwa]

*** Bug 120530 has been marked as a duplicate of this bug. ***