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.