Bug 61887 - ApplyStyleCommand shouldn't call collapseTextDecorationProperties
Summary: ApplyStyleCommand shouldn't call collapseTextDecorationProperties
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Enhancement
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 54932 34564
  Show dependency treegraph
 
Reported: 2011-06-01 15:45 PDT by Ryosuke Niwa
Modified: 2011-06-02 14:35 PDT (History)
4 users (show)

See Also:


Attachments
refactoring (8.25 KB, patch)
2011-06-01 16:39 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-06-01 15:45:11 PDT
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.
Comment 1 Ryosuke Niwa 2011-06-01 16:39:15 PDT
Created attachment 95684 [details]
refactoring
Comment 2 Ryosuke Niwa 2011-06-01 16:42:04 PDT
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.
Comment 3 Ryosuke Niwa 2011-06-01 23:22:20 PDT
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.
Comment 4 Enrica Casucci 2011-06-02 14:02:40 PDT
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.
Comment 5 Ryosuke Niwa 2011-06-02 14:18:45 PDT
(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.
Comment 6 Enrica Casucci 2011-06-02 14:22:02 PDT
Comment on attachment 95684 [details]
refactoring

Looks good to me
Comment 7 Ryosuke Niwa 2011-06-02 14:35:19 PDT
Comment on attachment 95684 [details]
refactoring

Clearing flags on attachment: 95684

Committed r87952: <http://trac.webkit.org/changeset/87952>
Comment 8 Ryosuke Niwa 2011-06-02 14:35:23 PDT
All reviewed patches have been landed.  Closing bug.