Bug 61887 – ApplyStyleCommand shouldn't call collapseTextDecorationProperties

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

ApplyStyleCommand::applyInlineStyle modifies the EditingStyle by calling collapseTextDecorationProperties but it shouldn't ApplyStyleCommand should be unaware of crazy text decoration manipulations we do in EditingStyle.

[Ryosuke Niwa]

Created attachment 95684 [details]
refactoring

[Ryosuke Niwa]

This will remove the last trace of text-decoration / -webkit-text-decorations-in-effect from ApplyStyleCommand.  And in fact, there will be no mentioning of these properties in ApplyStyleCommand after this patch!

ApplyStyleCommand still knows about font-size, unicode-bidi, and direction properties but hopefully I can get rid of them soon as well.

[Ryosuke Niwa]

Comment on attachment 95684 [details]
refactoring

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

> Source/WebCore/editing/EditingStyle.cpp:609
> +        if (propertyID == CSSPropertyWebkitTextDecorationsInEffect && inlineStyle->getPropertyCSSValue(CSSPropertyTextDecoration)) {
> +            if (!conflictingProperties)
>                  return true;
> +            conflictingProperties->append(CSSPropertyTextDecoration);
> +            if (extractedStyle)
> +                extractedStyle->setProperty(CSSPropertyTextDecoration, inlineStyle->getPropertyValue(CSSPropertyTextDecoration), inlineStyle->getPropertyPriority(CSSPropertyTextDecoration));
> +            continue;

Just to clarify, the only code change I'm making here is these lines where I explicitly add text-decoration property to the list of conflicting properties when I see -webkit-text-decorations-in-effect in m_mutableStyle and inlineStyle has text-decoration property.  Because inlineStyle is element's inline style declaration, we don't have to consider the case where inlineStyle has -webkit-text-decorations-in-effect property.

The rest of changes are due to my merging two loops together.  Whereas we used to have a faster loop when conflictingProperties == 0, I merged them and added return true when !conflictingProperties in several places.

[Enrica Casucci]

Comment on attachment 95684 [details]
refactoring

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

> Source/WebCore/editing/EditingStyle.cpp:171
> +    RefPtr<CSSValue> styleValue = style->getPropertyCSSValue(CSSPropertyWebkitTextDecorationsInEffect);

Could you explain why you don't use m_propertyID anymore? It's not clear to me.

[Ryosuke Niwa]

(In reply to comment #4)
> (From update of attachment 95684 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95684&action=review
> 
> > Source/WebCore/editing/EditingStyle.cpp:171
> > +    RefPtr<CSSValue> styleValue = style->getPropertyCSSValue(CSSPropertyWebkitTextDecorationsInEffect);
> 
> Could you explain why you don't use m_propertyID anymore? It's not clear to me.

I have to test for both text-decoration and -webkit-text-decorations-in-effect because both of these property should match u, s, & strike.

[Enrica Casucci]

Comment on attachment 95684 [details]
refactoring

Looks good to me

[Ryosuke Niwa]

Comment on attachment 95684 [details]
refactoring

Clearing flags on attachment: 95684

Committed r87952: <http://trac.webkit.org/changeset/87952>

[Ryosuke Niwa]

All reviewed patches have been landed.  Closing bug.