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.
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.
> 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.
This bug no longer blocks 23892.
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.
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.
Created attachment 68345 [details] work in progress 2 (works but needs a test)
(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.
Created attachment 252032 [details] WIP3 With this patch, editing/execCommand/merge-text-decoration-with-typing-style.html is the only failing test.
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.
Created attachment 252209 [details] Fixes the bug
<rdar://problem/3790443>
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 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.
Created attachment 252326 [details] Patch for landing
Comment on attachment 252326 [details] Patch for landing Clearing flags on attachment: 252326 Committed r183770: <http://trac.webkit.org/changeset/183770>
All reviewed patches have been landed. Closing bug.
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
(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.
*** Bug 120530 has been marked as a duplicate of this bug. ***