Bug 27818 - Toggling underline or strike through affects each other
: Toggling underline or strike through affects each other
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: HTML Editing
: 528+ (Nightly build)
: PC All
: P2 Normal
Assigned To: Ryosuke Niwa
: HasReduction
Depends on: 28968 46205 49813 49938
Blocks: 144670
  Show dependency treegraph
 
Reported: 2009-07-29 15:40 PDT by Ryosuke Niwa
Modified: 2015-05-05 23:23 PDT (History)
14 users (show)

See Also:


Attachments
demonstrates the bug (759 bytes, text/html)
2009-07-29 15:40 PDT, Ryosuke Niwa
no flags Details
my attempt to add parameter (33.22 KB, patch)
2009-08-14 11:16 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
work in progress (37.09 KB, patch)
2010-09-20 21:17 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
work in progress 2 (works but needs a test) (28.29 KB, patch)
2010-09-22 01:01 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
WIP3 (56.54 KB, patch)
2015-04-29 20:25 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
WIP4 (67.48 KB, patch)
2015-05-01 01:51 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixes the bug (87.71 KB, patch)
2015-05-01 18:04 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (88.89 KB, patch)
2015-05-04 12:46 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2009-07-29 15:40:11 PDT
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.
Comment 1 Ryosuke Niwa 2009-07-29 15:44:16 PDT
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.
Comment 2 Justin Garcia 2009-07-29 15:55:28 PDT
> 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.
Comment 3 Ryosuke Niwa 2009-08-13 18:13:55 PDT
This bug no longer blocks 23892.
Comment 4 Ryosuke Niwa 2009-08-14 11:16:08 PDT
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.
Comment 5 Ryosuke Niwa 2010-09-20 21:17:44 PDT
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.
Comment 6 Ryosuke Niwa 2010-09-22 01:01:19 PDT
Created attachment 68345 [details]
work in progress 2 (works but needs a test)
Comment 7 Ryosuke Niwa 2010-09-22 01:05:02 PDT
(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.
Comment 8 Ryosuke Niwa 2015-04-29 20:25:31 PDT
Created attachment 252032 [details]
WIP3

With this patch, editing/execCommand/merge-text-decoration-with-typing-style.html is the only failing test.
Comment 9 Ryosuke Niwa 2015-05-01 01:51:48 PDT
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.
Comment 10 Ryosuke Niwa 2015-05-01 18:04:08 PDT
Created attachment 252209 [details]
Fixes the bug
Comment 11 Ryosuke Niwa 2015-05-01 18:07:34 PDT
<rdar://problem/3790443>
Comment 12 Darin Adler 2015-05-02 07:24:58 PDT
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.
Comment 13 Ryosuke Niwa 2015-05-04 12:44:48 PDT
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.
Comment 14 Ryosuke Niwa 2015-05-04 12:46:49 PDT
Created attachment 252326 [details]
Patch for landing
Comment 15 WebKit Commit Bot 2015-05-04 13:44:14 PDT
Comment on attachment 252326 [details]
Patch for landing

Clearing flags on attachment: 252326

Committed r183770: <http://trac.webkit.org/changeset/183770>
Comment 16 WebKit Commit Bot 2015-05-04 13:44:20 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Alexey Proskuryakov 2015-05-05 18:50:57 PDT
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
Comment 18 Ryosuke Niwa 2015-05-05 21:41:22 PDT
(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.