Bug 46335 – Add EditingStyle

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Created attachment 68496 [details]
work in progress

Currently, ApplyStyleCommand.cpp contains a few functions to extract CSSMutableStyleDeclaration and prepares it to apply for style preservation purposes: removeNonEditingProperties, editingStyleAtPosition, prepareEditingStyleToApplyAt, and removeStylesAddedByNode.  But the fact they are global functions and reside in "Apply"StyleCommand.cpp is rather misleading, and so I propose to isolate all of these functions into a new EditingStyle class.

A big motivation behind this is the bug 27818.  Resolving the bug 27818 requires adding new argument to one of the constructors of ApplyStyleCommand and to many functions between executeUnderline and ApplyStyleCommand.  This is not an ideal solution since it introduces new dependency between many parts of editing.  Adding EditingStyle solves this problem because EditingStyle can encapsulate this new argument, decoupling the implementation details of adding and removing text decorations from the rest of editing code.

In the long run, I also want to merge StyleChange and much of logic in removeImplicitlyStyledElement to this new class so that ApplyStyleCommand can ask EditingStyle which element needs to be removed or added instead of ApplyStyleCommand figuring out what element needs to be added or removed.  This nicely separates the logic to manipulate the DOM and the logic to query against styles.

[Ryosuke Niwa]

The attachment is my work in progress.  I still need to do more cleanup in EditingStyle.  Namely I'd like to make constructors of EditingStyle explicit about whether or not it includes non-inheritable styles.

The current implementation of EditingStyle has style() member function that lets caller manipulate the style directly but this is intended to be removed in the future once we deployed this class in all parts of editing and removed the direct dependency on CSSMutableStyleDeclaration and CSSComputedStyleDeclaration.

[Eric Seidel (no email)]

Comment on attachment 68496 [details]
work in progress

The idea seems sound to me.

[Eric Seidel (no email)]

Comment on attachment 68496 [details]
work in progress

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

> WebCore/editing/CompositeEditCommand.cpp:935
> +        styleInEmptyParagraph = editingStyleWithTypingStyle(startOfParagraphToMove.deepEquivalent());

Maybe editingStyleIncludingTypingStyle?  I'm not sure what exactly this method means.

> WebCore/editing/ReplaceSelectionCommand.cpp:641
> +    sourceDocumentStyle->prepareAppliedAt(Position(context, 0));

prepareToApplyAt?

> WebCore/editing/ReplaceSelectionCommand.cpp:689
> +    setNodeAttribute(static_cast<Element*>(copiedRangeStyleSpan), styleAttr, copiedRangeStyle->style()->cssText());

Are we eventually going to hide the existance of the style() member?  And expose only the functions callers need?  Or will EditingStyle always expose to the world that it "has" a CSSMutableStyleDeclaration?

> WebCore/editing/Editor.cpp:3262
> +    mutableStyle->mergeAndOverrideWith(typingStyle.get());

This might be cleaner as a merge( fucntion with an OverrideWithNew enum (or some similarly named enum to explain the differnet modes).  Do we have a non-overriding merge?

> WebCore/editing/SelectionController.h:249
>  inline void SelectionController::clearTypingStyle()
>  {
> -    m_typingStyle.clear();
> +    m_typingStyle->clear();
>  }
>  
>  inline void SelectionController::setTypingStyle(PassRefPtr<CSSMutableStyleDeclaration> style)
>  {
> -    m_typingStyle = style;
> +    m_typingStyle->setStyle(style);
>  }

I guess not in this patch, but do we need to be clearing the whole typing style?  It seems strange to only clear its style member.  I understand you're not trying to change behavior here, but it's unclear to me why this part of code would only want to clear "part" of an EditingStyle

> WebCore/editing/EditingStyle.cpp:80
> +    : m_mutableStyle(0)

No need to explictly 0 RefPtr.

> WebCore/editing/EditingStyle.cpp:112
> +    RefPtr<CSSComputedStyleDeclaration> computedStyleAtPosition = computedStyle(node);
> +    RefPtr<CSSMutableStyleDeclaration> style;
> +    if (!computedStyleAtPosition)
> +        style = CSSMutableStyleDeclaration::create();
> +    else
> +        style = removeNonEditingProperties(computedStyleAtPosition.get());

This feels like a helper function.

> WebCore/editing/EditingStyle.cpp:127
> +    if (node && node->computedStyle()) {
> +        RenderStyle* renderStyle = node->computedStyle();
> +        // If a node's text fill color is invalid, then its children use 
> +        // their font-color as their text fill color (they don't
> +        // inherit it).  Likewise for stroke color.
> +        ExceptionCode ec = 0;
> +        if (!renderStyle->textFillColor().isValid())
> +            style->removeProperty(CSSPropertyWebkitTextFillColor, ec);
> +        if (!renderStyle->textStrokeColor().isValid())
> +            style->removeProperty(CSSPropertyWebkitTextStrokeColor, ec);
> +        ASSERT(ec == 0);
> +        if (renderStyle->fontDescription().keywordSize())
> +            style->setProperty(CSSPropertyFontSize, computedStyleAtPosition->getFontSizeCSSValuePreferringKeyword()->cssText());
> +    }

This also feels like a separate function.  Once you break it out into a seprate function, maybe we can move it to somewhere else more fitting?

> WebCore/editing/EditingStyle.cpp:137
> +    /*    if (shouldIncludeTypingStyle == IncludeTypingStyle) {
> +     CSSMutableStyleDeclaration* typingStyle = pos.node()->document()->frame()->selection()->typingStyle();
> +     if (typingStyle)
> +     style->merge(typingStyle);
> +     }*/
> +}

Commented out code?

> WebCore/editing/EditingStyle.cpp:141
> +    return !m_mutableStyle || m_mutableStyle->length() == 0;

no isEmpty() here too?

> WebCore/editing/EditingStyle.cpp:147
> +    m_shouldUseFixedDefaultFontSize = false; // FIXME: we should take care of this

What does your FIXME mean?

> WebCore/editing/EditingStyle.cpp:175
> +    RefPtr<CSSComputedStyleDeclaration> parentStyle = computedStyle(node->parentNode());
> +    RefPtr<CSSMutableStyleDeclaration> styleAddedByNode = computedStyle(node)->copy();
> +
> +    parentStyle->diff(styleAddedByNode.get());
> +    removeNonEditingProperties(styleAddedByNode.get());
> +
> +    styleAddedByNode->diff(m_mutableStyle.get());

This feels wrong.  Why are we copying here?  If this is wrong, please add a FIXME, otherwise maybe a comment to explain why this is here?

> WebCore/editing/EditingStyle.cpp:186
> +void EditingStyle::removePropertiesPresentIn(CSSMutableStyleDeclaration* style)

Python calls this operation:
difference_update(other, ...)
set -= other | ...

But I'm not sure differenceUpdate makes any more sense.  But this does feel like a set operation.  removePropertiesSharedWith?  removeProperitesAlsoIn?  removePropertiesPresentIn is actually a fine name.

> WebCore/editing/EditingStyle.cpp:195
> +    for (CSSMutableStyleDeclaration::const_iterator it = style->begin(); it != end; ++it) {
> +        const CSSProperty& property = *it;
> +        m_mutableStyle->removeProperty(property.id());
> +    }

Can remove local and {}

> WebCore/editing/EditingStyle.cpp:212
> +        if (color.alpha() == 0)

Isn't there a !transparent()?

[Ryosuke Niwa]

Comment on attachment 68496 [details]
work in progress

Thanks for the feedback!  I'll update a new smaller patch soon.

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

>> WebCore/editing/CompositeEditCommand.cpp:935
>> +        styleInEmptyParagraph = editingStyleWithTypingStyle(startOfParagraphToMove.deepEquivalent());
> 
> Maybe editingStyleIncludingTypingStyle?  I'm not sure what exactly this method means.

Fixed.

>> WebCore/editing/ReplaceSelectionCommand.cpp:641
>> +    sourceDocumentStyle->prepareAppliedAt(Position(context, 0));
> 
> prepareToApplyAt?

Fixed.

>> WebCore/editing/ReplaceSelectionCommand.cpp:689
>> +    setNodeAttribute(static_cast<Element*>(copiedRangeStyleSpan), styleAttr, copiedRangeStyle->style()->cssText());
> 
> Are we eventually going to hide the existance of the style() member?  And expose only the functions callers need?  Or will EditingStyle always expose to the world that it "has" a CSSMutableStyleDeclaration?

Yes, that's the plan.  I want to centralize all style manipulations in editing at one place so that we have a better control of it.

>> WebCore/editing/Editor.cpp:3262
>> +    mutableStyle->mergeAndOverrideWith(typingStyle.get());
> 
> This might be cleaner as a merge( fucntion with an OverrideWithNew enum (or some similarly named enum to explain the differnet modes).  Do we have a non-overriding merge?

I'm removing mergeAndOverrideWith from my initial patch.

>> WebCore/editing/SelectionController.h:249
>>  }
> 
> I guess not in this patch, but do we need to be clearing the whole typing style?  It seems strange to only clear its style member.  I understand you're not trying to change behavior here, but it's unclear to me why this part of code would only want to clear "part" of an EditingStyle

I set m_shouldUseFixedDefaultFontSize = false in setStyle / clear so I think we're clearing the whole editing style.  I'm not sure what you're referring to by "part".

>> WebCore/editing/EditingStyle.cpp:80
>> +    : m_mutableStyle(0)
> 
> No need to explictly 0 RefPtr.

Good point.  Removed.

>> WebCore/editing/EditingStyle.cpp:112
>> +        style = removeNonEditingProperties(computedStyleAtPosition.get());
> 
> This feels like a helper function.

Added editingStyleFromCompuedStyle.

>> WebCore/editing/EditingStyle.cpp:127
>> +    }
> 
> This also feels like a separate function.  Once you break it out into a seprate function, maybe we can move it to somewhere else more fitting?

Added removeTextFillAndStroleColorsIfNeeded and replaceFontSizeByKeywordIfPossible.

>> WebCore/editing/EditingStyle.cpp:137
>> +}
> 
> Commented out code?

Oops, removed.

>> WebCore/editing/EditingStyle.cpp:147
>> +    m_shouldUseFixedDefaultFontSize = false; // FIXME: we should take care of this
> 
> What does your FIXME mean?

Fixed.

>> WebCore/editing/EditingStyle.cpp:175
>> +    styleAddedByNode->diff(m_mutableStyle.get());
> 
> This feels wrong.  Why are we copying here?  If this is wrong, please add a FIXME, otherwise maybe a comment to explain why this is here?

I'm removing this function from my initial patch.

>> WebCore/editing/EditingStyle.cpp:186
>> +void EditingStyle::removePropertiesPresentIn(CSSMutableStyleDeclaration* style)
> 
> Python calls this operation:
> difference_update(other, ...)
> set -= other | ...
> 
> But I'm not sure differenceUpdate makes any more sense.  But this does feel like a set operation.  removePropertiesSharedWith?  removeProperitesAlsoIn?  removePropertiesPresentIn is actually a fine name.

I'm removing this one as well.

>> WebCore/editing/EditingStyle.cpp:212
>> +        if (color.alpha() == 0)
> 
> Isn't there a !transparent()?

No.

[Ryosuke Niwa]

Created attachment 69363 [details]
Fixed per Eric's comment

[Eric Seidel (no email)]

Comment on attachment 69363 [details]
Fixed per Eric's comment

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

> WebCore/editing/CompositeEditCommand.cpp:935
> +        styleInEmptyParagraph = editingStyleIncludingTypingStyle(startOfParagraphToMove.deepEquivalent());

Is there an editing style which does not include typing style?

> WebCore/editing/CompositeEditCommand.cpp:1052
> +        applyStyle(style->style());

Does applyStyle ASSERT ! empty?  Or should this check just go into applyStyle?

> WebCore/editing/DeleteSelectionCommand.cpp:297
> +    m_typingStyle = EditingStyle::create(positionBeforeTabSpan(m_selectionToDelete.start()));

I take it this is an editing style which does not include the typing style?

> WebCore/editing/EditingStyle.cpp:51
> +: m_shouldUseFixedDefaultFontSize(false)

indent.

> WebCore/editing/EditingStyle.cpp:56
> +: m_shouldUseFixedDefaultFontSize(false)

again.

> WebCore/editing/EditingStyle.cpp:131
> +    if (!m_mutableStyle)
> +        return;
> +
> +    m_mutableStyle->removeBlockProperties();

I would have probably just reversed this if since it's one line.

> WebCore/editing/EditingStyle.cpp:146
> +        CSSPrimitiveValue* primitiveValue = static_cast<CSSPrimitiveValue*>(backgroundColor.get());
> +        Color color = Color(primitiveValue->getRGBA32Value());

This is the way we build colors from CSSPrimativeValues?  I would have expected there to be a function or constructor which did this.

> WebCore/editing/EditingStyle.h:78
> +    void init(Node*);

What is the purpose of a separate init() call?  Generally we don't have separate init() in webkit.

[Ryosuke Niwa]

Comment on attachment 69363 [details]
Fixed per Eric's comment

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

>> WebCore/editing/CompositeEditCommand.cpp:935
>> +        styleInEmptyParagraph = editingStyleIncludingTypingStyle(startOfParagraphToMove.deepEquivalent());
> 
> Is there an editing style which does not include typing style?

Yes.  See where we call EditingStyle::create.  As I mentioned yesterday, it might make sense to make editingStyle() for those places as supposed to be calling EditingStyle::create.

>> WebCore/editing/CompositeEditCommand.cpp:1052
>> +        applyStyle(style->style());
> 
> Does applyStyle ASSERT ! empty?  Or should this check just go into applyStyle?

I think this check is an optimization because we do check this condition in ApplyStyleCommand::doApply.  See http://trac.webkit.org/browser/trunk/WebCore/editing/ApplyStyleCommand.cpp#L599.  i.e. applyStyle shouldn't be doing anything when style->isEmpty() is true.

>> WebCore/editing/DeleteSelectionCommand.cpp:297
>> +    m_typingStyle = EditingStyle::create(positionBeforeTabSpan(m_selectionToDelete.start()));
> 
> I take it this is an editing style which does not include the typing style?

Yes.

>> WebCore/editing/EditingStyle.cpp:51
>> +: m_shouldUseFixedDefaultFontSize(false)
> 
> indent.

Will fix.

>> WebCore/editing/EditingStyle.cpp:56
>> +: m_shouldUseFixedDefaultFontSize(false)
> 
> again.

Will fix.

>> WebCore/editing/EditingStyle.cpp:131
>> +    m_mutableStyle->removeBlockProperties();
> 
> I would have probably just reversed this if since it's one line.

Will fix.

>> WebCore/editing/EditingStyle.cpp:146
>> +        Color color = Color(primitiveValue->getRGBA32Value());
> 
> This is the way we build colors from CSSPrimativeValues?  I would have expected there to be a function or constructor which did this.

I should have done:
if (!alphaChannel(primitiveValue->getRGBA32Value()))
  m_mutableStyle->removeProperty(CSSPropertyBackgroundColor, ec);

Will fix.

>> WebCore/editing/EditingStyle.h:78
>> +    void init(Node*);
> 
> What is the purpose of a separate init() call?  Generally we don't have separate init() in webkit.

Constructors that take Node and Position need to do the same initialization.

[Ryosuke Niwa]

Created attachment 69541 [details]
fixed more per Eric's comment

[Ryosuke Niwa]

Created attachment 69542 [details]
fixed changelog

[Eric Seidel (no email)]

Comment on attachment 69542 [details]
fixed changelog

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

Generally looks OK, but curiosu to teh answers to above.

> WebCore/editing/DeleteSelectionCommand.cpp:720
> +    document()->frame()->selection()->setTypingStyle(m_typingStyle ? m_typingStyle->style() : 0);

Any time we're reaching into the style like this I"m suspicious.
Since we're changing selection too, shouldn't we add a setTypingStyle method which takes an editing styel and deprecate the other one?

> WebCore/editing/SelectionController.h:243
> -    m_typingStyle.clear();
> +    m_typingStyle->clear();

Why not m_typingstyle = 0?

> WebCore/editing/SelectionController.h:248
> -    m_typingStyle = style;
> +    m_typingStyle->setStyle(style);

Seems odd to be reaching into the typeingstyle like this.  Will this go away?  Should this hav ea FIXME?

[Ryosuke Niwa]

Comment on attachment 69542 [details]
fixed changelog

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

> WebCore/editing/DeleteSelectionCommand.cpp:697
> +    prepareEditingStyleToApplyAt(m_typingStyle->style(), m_endingPosition);
> +    if (m_typingStyle->isEmpty())

oops, I should be calling prepareToApplyAt here.

>> WebCore/editing/DeleteSelectionCommand.cpp:720
>> +    document()->frame()->selection()->setTypingStyle(m_typingStyle ? m_typingStyle->style() : 0);
> 
> Any time we're reaching into the style like this I"m suspicious.
> Since we're changing selection too, shouldn't we add a setTypingStyle method which takes an editing styel and deprecate the other one?

Yeah, I'll add a version that takes EditingStyle.  I probably can't remove the old version because they're called in a bunch of other places.

>> WebCore/editing/SelectionController.h:243
>> +    m_typingStyle->clear();
> 
> Why not m_typingstyle = 0?

Because there are places we access m_typingStyle without a null check.

>> WebCore/editing/SelectionController.h:248
>> +    m_typingStyle->setStyle(style);
> 
> Seems odd to be reaching into the typeingstyle like this.  Will this go away?  Should this hav ea FIXME?

Yes, we should get rid of this version of setTypingStyle.  We should only have a version that takes EditingStyle.

[Ryosuke Niwa]

Created attachment 71607 [details]
fixed & made it compile on TOT

[WebKit Review Bot]

Attachment 71607 [details] did not build on win:
Build output: http://queues.webkit.org/results/4847102

[Eric Seidel (no email)]

Comment on attachment 71607 [details]
fixed & made it compile on TOT

This looks to move us in a positive direction.

[Ryosuke Niwa]

(In reply to comment #14)
> (From update of attachment 71607 [details])
> This looks to move us in a positive direction.

Thanks for the review!

[Ryosuke Niwa]

Created attachment 73014 [details]
fixed the crash on fast/events/keydown-function-keys.html

[Ryosuke Niwa]

(In reply to comment #16)
> Created an attachment (id=73014) [details]
> fixed the crash on fast/events/keydown-function-keys.html

I'm pretty sure this test didn't crash when I uploaded the original patch but the problem was with instantiating EditingStyle in the constructor of EditingStyle.  m_typingStyle somehow reset the ref counter of the mutable style to 0 after returning from EditingStyle::create.  Avoiding to instantiate EditingStyle inside SelectionController::SelectionController fixed this problem.

[Ryosuke Niwa]

Eric, could you review new patch?  The change is very small I stopped instantiating EditingStyle in SelectionController.

[Darin Adler]

Comment on attachment 73014 [details]
fixed the crash on fast/events/keydown-function-keys.html

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

> WebCore/editing/EditingStyle.cpp:118
> +    m_mutableStyle = 0;

Should be nullptr rather than 0.

> WebCore/editing/EditingStyle.cpp:149
> +    RefPtr<EditingStyle> edtingStyle = EditingStyle::create(position);

Typo: "edting style".

> WebCore/editing/EditingStyle.h:43
> +class EditingStyle : public RefCounted<EditingStyle> {

To me it seems unhelpful to have EditingStyle be a reference counted object. Since an editing style contains just one pointer to a reference counted object and one boolean, it seems it could instead be a normal copyable object with constructors instead of create functions. This would make it easier to program with.

> WebCore/editing/EditingStyle.h:79
> +    void removeTextFillAndStroleColorsIfNeeded(RenderStyle*);

Typo: "strole colors"

[Ryosuke Niwa]

Thanks for the review as always!

(In reply to comment #19)
> > WebCore/editing/EditingStyle.cpp:118
> > +    m_mutableStyle = 0;
> 
> Should be nullptr rather than 0.

Used clear() instead.

> Typo: "edting style".

Fixed.

> To me it seems unhelpful to have EditingStyle be a reference counted object. Since an editing style contains just one pointer to a reference counted object and one boolean, it seems it could instead be a normal copyable object with constructors instead of create functions. This would make it easier to program with.

I wrote in comment #1 as well but I have a plan to merge StyleChange and conversion table between CSS & html elements in ApplyStyleCommand.cpp to this class.  So I suspect that it'll have more member variables and definitely more member functions.  I also wanted to make the transition from CSSMutableStyleDeclaration to EditingStyle easy, and since CSSMutableStyleDeclaration is a reference counted object, I made EditingStyle a reference counted object as well to make reviewing process easier.  However, I'm happy to change it to a normal object once I migrated the other parts of editing code to use EditingStyle, and we concluded that reference counting is unnecessary.

> Typo: "strole colors"

Fixed.

[Ryosuke Niwa]

Committed r71469: <http://trac.webkit.org/changeset/71469>