Bug 27818 - ApplyStyleCommand needs parameter to toggle underline and strike through over multiple nodes with multiple text decorations
: ApplyStyleCommand needs parameter to toggle underline and strike through over...
Status: NEW
: WebKit
HTML Editing
: 528+ (Nightly build)
: PC All
: P2 Normal
Assigned To:
:
: HasReduction
: 28968 46205 49813 49938
:
  Show dependency treegraph
 
Reported: 2009-07-29 15:40 PST by
Modified: 2011-11-07 18:54 PST (History)


Attachments
demonstrates the bug (759 bytes, text/html)
2009-07-29 15:40 PST, Ryosuke Niwa
no flags Details
my attempt to add parameter (33.22 KB, patch)
2009-08-14 11:16 PST, Ryosuke Niwa
no flags Review Patch | Details | Formatted Diff | Diff
work in progress (37.09 KB, patch)
2010-09-20 21:17 PST, Ryosuke Niwa
no flags Review Patch | Details | Formatted Diff | Diff
work in progress 2 (works but needs a test) (28.29 KB, patch)
2010-09-22 01:01 PST, Ryosuke Niwa
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-07-29 15:40:11 PST
Created an attachment (id=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 From 2009-07-29 15:44:16 PST -------
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 From 2009-07-29 15:55:28 PST -------
> 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 From 2009-08-13 18:13:55 PST -------
This bug no longer blocks 23892.
------- Comment #4 From 2009-08-14 11:16:08 PST -------
Created an attachment (id=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 From 2010-09-20 21:17:44 PST -------
Created an attachment (id=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 From 2010-09-22 01:01:19 PST -------
Created an attachment (id=68345) [details]
work in progress 2 (works but needs a test)
------- Comment #7 From 2010-09-22 01:05:02 PST -------
(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.