Bug 27749 – StyleChange::init needs clean up before fixing the bug 23892

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

StyleChange::init and checkForLegacyHTMLStyleChange needs clean up in order to fix:
Bug 23892 - execCommand("Underline") uses CSS even when styleWithCSS has been turned off (edit)

[Ryosuke Niwa]

Created attachment 33590 [details]
fixes the bug

[Ryosuke Niwa]

Created attachment 33591 [details]
minor correction

[Ryosuke Niwa]

SVN diff did terrible job here:

Original if statments:
118	        if (property->id() == CSSPropertyWhiteSpace && (isTabSpanTextNode(position.node()) || isTabSpanNode((position.node()))))
119	            continue;
120	       
121	        // If needed, figure out if this change is a legacy HTML style change.
122	        if (useHTMLFormattingTags && checkForLegacyHTMLStyleChange(property))
123	            continue;
124	
125	        if (property->id() == CSSPropertyDirection) {
126	            if (addedDirection)
127	                continue;
128	            addedDirection = true;
129	        }
130	
131	        // Add this property
132	        if (property->id() == CSSPropertyTextDecoration || property->id() == CSSPropertyWebkitTextDecorationsInEffect) {
133	            // Always use text-decoration because -webkit-text-decoration-in-effect is internal.
134	            CSSProperty alteredProperty(CSSPropertyTextDecoration, property->value(), property->isImportant());
135	            // We don't add "text-decoration: none" because it doesn't override the existing text decorations; i.e. redundant
136	            if (!equalIgnoringCase(alteredProperty.value()->cssText(), "none"))
137	                styleText += alteredProperty.cssText();
138	        } else
139	            styleText += property->cssText();
140	
141	        if (!addedDirection && property->id() == CSSPropertyUnicodeBidi) {
142	            styleText += "direction: " + style->getPropertyValue(CSSPropertyDirection) + "; ";
143	            addedDirection = true;
144	        }

My switch statement:
        switch (property->id()) {
        case CSSPropertyWhiteSpace:
            // Changing the whitespace style in a tab span would collapse the tab into a space.
            if (isTabSpanTextNode(position.node()) || isTabSpanNode(position.node()))
                continue;
            break;
        case CSSPropertyDirection:
            if (!addedDirection) {
                styleText += property->cssText();
                addedDirection = true;
            }
            continue;
        case CSSPropertyUnicodeBidi:
            if (!addedDirection) {
                styleText += "direction: " + style->getPropertyValue(CSSPropertyDirection) + "; ";
                addedDirection = true;
            }
            break;
        case CSSPropertyTextDecoration:
        case CSSPropertyWebkitTextDecorationsInEffect:
            // Always use text-decoration because -webkit-text-decoration-in-effect is internal.
            CSSProperty alteredProperty(CSSPropertyTextDecoration, property->value(), property->isImportant());
            // We don't add "text-decoration: none" because it doesn't override the existing text decorations; i.e. redundant
            if (!equalIgnoringCase(alteredProperty.value()->cssText(), "none"))
                styleText += alteredProperty.cssText();
            continue;
        }

        // If needed, convert the CSS property to HTML tags by turning on m_apply*
        styleText += useHTMLFormattingTags ? convertCSSPropertyToApplyFlags(property) : property->cssText();

[Eric Seidel (no email)]

Comment on attachment 33591 [details]
minor correction

Maybe convertCSSPropertyToApplyFlags should just be "styleText += cssTextForPropertyRespectingLegacyTags(propertyId, useHTMLFormattingTags)"

or
"checkForFormatingTagsAndReturnCSSText"

or
"cssTextIgnoringFormattingTags"

Any of these would just take the useHTMLFormattingTags parameter, and then you wouldn't have two places where you have to call property->cssText()

[Ryosuke Niwa]

Created attachment 33652 [details]
fixed per comment

[Ryosuke Niwa]

(In reply to comment #4)
> (From update of attachment 33591 [details])
> Maybe convertCSSPropertyToApplyFlags should just be "styleText +=
> cssTextForPropertyRespectingLegacyTags(propertyId, useHTMLFormattingTags)"

I was considering this earlier but I thought passing boolean value and bailing out right away isn't so elegant.  But I guess that's much cleaner.

> "cssTextIgnoringFormattingTags"

I like the name

[Ryosuke Niwa]

It turned out that we need more substantial clean up.

[Ryosuke Niwa]

Created attachment 34712 [details]
fixes the bug, a minor rebaseline is required

[Darin Adler]

Comment on attachment 34712 [details]
fixes the bug, a minor rebaseline is required

> +    void cleanupTextDecorationProperties(CSSMutableStyleDeclaration* style);

The verb "clean up" is two words, so this would be "cleanUpTextDecorationProperties". But it's not clear what "clean up" means and I'd like the function to have a better name. I think a better verb might be "reconcile" or "standardize". This function needs a name that explains what it does as clearly as possible.

The declaration of this function should not have a name for the argument "style" since the type is unambiguous. Same for the other function added in this patch.

> +    RefPtr<CSSValue> textDecorationsInEffect = style->getPropertyCSSValue(CSSPropertyWebkitTextDecorationsInEffect);
> +    RefPtr<CSSValue> textDecoration = style->getPropertyCSSValue(CSSPropertyTextDecoration);
> +    // We shouldn't have both text-decoration and -webkit-text-decorations-in-effect because that wouldn't make sense.
> +    ASSERT(!textDecorationsInEffect || !textDecoration);

The textDecoration value fetched here is never used except in the assertion. It would be slightly better not to even make the getPropertyCSSValue call in debug builds.

> +    textDecoration = style->getPropertyCSSValue(CSSPropertyTextDecoration);
> +    if (textDecoration && !textDecoration->isValueList())
> +        style->removeProperty(CSSPropertyTextDecoration);

I don't understand the purpose of removing the text-decoration property when its value is not a value list. The code needs a comment explaining why this is a good idea.

> +    if (equalIgnoringCase(style->getPropertyValue(CSSPropertyFontWeight), "bold")) {

Even though the old code already did this, it would be more efficient to do this check on CSSValue rather than constructing a string with the text form of the value just so you can compare with a constant string. You can make a helper function to help automate checking that it's a CSSPrimitiveValue, then calling getIdent() and checking for the particular identifier value.

> +    if (equalIgnoringCase(style->getPropertyValue(CSSPropertyFontStyle), "italic")
> +        || equalIgnoringCase(style->getPropertyValue(CSSPropertyFontStyle), "oblique")) {

Ditto.

> +    if (equalIgnoringCase(style->getPropertyValue(CSSPropertyVerticalAlign), "sub")) {
> +        style->removeProperty(CSSPropertyVerticalAlign);
> +        m_applySubscript = true;
> +    } else if (equalIgnoringCase(style->getPropertyValue(CSSPropertyVerticalAlign), "super")) {
> +        style->removeProperty(CSSPropertyVerticalAlign);
> +        m_applySuperscript = true;
> +    }

Ditto. It would also be more efficient to not get the same property value twice.

> +    if (RefPtr<CSSValue> colorValue = style->getPropertyCSSValue(CSSPropertyColor)) {
> +        RGBA32 rgba = 0;
> +        CSSParser::parseColor(rgba, colorValue->cssText());
> +        Color color(rgba);
> +        m_applyFontColor = color.name();
> +        style->removeProperty(CSSPropertyColor);
> +    }

Is there no more efficient way to find out what color is specified by a CSSValue than to convert to a string and then parse the color? For example, would the getRGBA32Value function cover enough cases? Or do we also have to parse color strings?

> +    RefPtr<CSSValue> fontSize = style->getPropertyCSSValue(CSSPropertyFontSize);
> +    if (fontSize) {
> +        if (!fontSize->isPrimitiveValue())
> +            style->removeProperty(CSSPropertyFontSize); // Can't make sense of the number. Put no font size.
> +        else {
> +            CSSPrimitiveValue* value = static_cast<CSSPrimitiveValue*>(fontSize.get());
>  
> +            if (value->primitiveType() < CSSPrimitiveValue::CSS_PX || value->primitiveType() > CSSPrimitiveValue::CSS_PC)
> +                ;// Size keyword or relative unit.

The formatting here is confusing. I suggest using braces instead of just a semicolon. I also suggest a longer comment, and if it's two lines then it would justify having braces! Understood that it's a size keyword or a relative unit, but why are does this code do nothing in that case? Is it something we want to handle later or is it already correct?

> +        RefPtr<CSSValueList> resultValueList = static_cast<CSSValueList*>(resultValue.get())->copy();
>          CSSValueList* computedValueList = static_cast<CSSValueList*>(computedValue.get());
> +        for (size_t i = 0; i < computedValueList->length(); i++)
> +            resultValueList->removeAll(computedValueList->item(i));
> +        result->setProperty(textDecorationProperties[n], resultValueList->cssText());

I don't understand why you're copying the CSSValueList here. If the list is "live" and associated with the original property, then it seems that we could just modify the list and we wouldn't need to call setProperty at all. If the list is not live and is not associated with the property, then it seems we could modify it without making a copy.

I'm going to say review- because there are a few items above that might inspire code changes. Generally the patch seems quite good.

[Ryosuke Niwa]

Thanks you for your feedback!

(In reply to comment #9)
> I'd like the function to have a better name. I think a better verb might be
> "reconcile" or "standardize". This function needs a name that explains what it
> does as clearly as possible.

I'll use reconcileTextDecorationProperties unless we come up with a better name.

> The declaration of this function should not have a name for the argument
> "style" since the type is unambiguous. Same for the other function added in
> this patch.

Ugh, style glitch.  Removed.

> The textDecoration value fetched here is never used except in the assertion. It
> would be slightly better not to even make the getPropertyCSSValue call in debug
> builds.

I changed the logic slightly so that textDecoration is used when textDecorationsInEffect is 0.
 
> I don't understand the purpose of removing the text-decoration property when
> its value is not a value list. The code needs a comment explaining why this is
> a good idea.

We don't want to add "text-decoration: none" because it's redundant

> Even though the old code already did this, it would be more efficient to do
> this check on CSSValue rather than constructing a string with the text form of
> the value just so you can compare with a constant string. You can make a helper
> function to help automate checking that it's a CSSPrimitiveValue, then calling
> getIdent() and checking for the particular identifier value.

I added
int getPrimitiveValue(CSSMutableStyleDeclaration* style, int propertyID)
don't know why I didn't come up with this earlier.

> It would also be more efficient to not get the same property value twice.

Done.  A local copy of the identifier is made.

> > +    if (RefPtr<CSSValue> colorValue = style->getPropertyCSSValue(CSSPropertyColor)) {
> > +        RGBA32 rgba = 0;
> > +        CSSParser::parseColor(rgba, colorValue->cssText());
> > +        Color color(rgba);
> > +        m_applyFontColor = color.name();
> > +        style->removeProperty(CSSPropertyColor);
> > +    }
> 
> Is there no more efficient way to find out what color is specified by a
> CSSValue than to convert to a string and then parse the color? For example,
> would the getRGBA32Value function cover enough cases? Or do we also have to
> parse color strings?

I'm working on this.

> > +            CSSPrimitiveValue* value = static_cast<CSSPrimitiveValue*>(fontSize.get());
> >  
> > +            if (value->primitiveType() < CSSPrimitiveValue::CSS_PX || value->primitiveType() > CSSPrimitiveValue::CSS_PC)
> > +                ;// Size keyword or relative unit.
> 
> The formatting here is confusing. I suggest using braces instead of just a
> semicolon. I also suggest a longer comment, and if it's two lines then it would
> justify having braces! Understood that it's a size keyword or a relative unit,
> but why are does this code do nothing in that case? Is it something we want to
> handle later or is it already correct?

In the original function, there was a break statement so I was intending to negate the condition
and delete the empty if but totally forgot.  Now done.

> 
> > +        RefPtr<CSSValueList> resultValueList = static_cast<CSSValueList*>(resultValue.get())->copy();
> >          CSSValueList* computedValueList = static_cast<CSSValueList*>(computedValue.get());
> > +        for (size_t i = 0; i < computedValueList->length(); i++)
> > +            resultValueList->removeAll(computedValueList->item(i));
> > +        result->setProperty(textDecorationProperties[n], resultValueList->cssText());
> 
> I don't understand why you're copying the CSSValueList here. If the list is
> "live" and associated with the original property, then it seems that we could
> just modify the list and we wouldn't need to call setProperty at all. If the
> list is not live and is not associated with the property, then it seems we
> could modify it without making a copy.

So it turned out that when we copy() computed style, it won't create a copy of CSSValueList.
i.e. we will be changing the value of both mutable object and computed style.

Take a look at the lines 147-161 of CSSStyleDeclaration.  It only copies CSSProperty but CSSProperty merely copies the RefPtr to CSSValue* which means CSSValueList isn't really copied.  I think this is a bad implementation of copy, but if we decided to copy every single CSSValue then performance may regress significantly, so I won't touch it this time.  I should add a comment maybe?


> I'm going to say review- because there are a few items above that might inspire
> code changes. Generally the patch seems quite good.

Thanks for the review.  I'm posting the follow up patch shortly.

[Darin Adler]

(In reply to comment #10)
> > I don't understand the purpose of removing the text-decoration property when
> > its value is not a value list. The code needs a comment explaining why this is
> > a good idea.
> 
> We don't want to add "text-decoration: none" because it's redundant

Needs a comment. !isValueList() is not an obvious way to check for "none".

> > Even though the old code already did this, it would be more efficient to do
> > this check on CSSValue rather than constructing a string with the text form of
> > the value just so you can compare with a constant string. You can make a helper
> > function to help automate checking that it's a CSSPrimitiveValue, then calling
> > getIdent() and checking for the particular identifier value.
> 
> I added
> int getPrimitiveValue(CSSMutableStyleDeclaration* style, int propertyID)

I don't think that's quite the right name. A primitive value is not necessarily an identifier, and I think you need a name that mentions the word identifier.

> > > +        RefPtr<CSSValueList> resultValueList = static_cast<CSSValueList*>(resultValue.get())->copy();
> > >          CSSValueList* computedValueList = static_cast<CSSValueList*>(computedValue.get());
> > > +        for (size_t i = 0; i < computedValueList->length(); i++)
> > > +            resultValueList->removeAll(computedValueList->item(i));
> > > +        result->setProperty(textDecorationProperties[n], resultValueList->cssText());
> > 
> > I don't understand why you're copying the CSSValueList here. If the list is
> > "live" and associated with the original property, then it seems that we could
> > just modify the list and we wouldn't need to call setProperty at all. If the
> > list is not live and is not associated with the property, then it seems we
> > could modify it without making a copy.
> 
> So it turned out that when we copy() computed style, it won't create a copy of
> CSSValueList.
> i.e. we will be changing the value of both mutable object and computed style.
> 
> Take a look at the lines 147-161 of CSSStyleDeclaration.  It only copies
> CSSProperty but CSSProperty merely copies the RefPtr to CSSValue* which means
> CSSValueList isn't really copied.  I think this is a bad implementation of
> copy, but if we decided to copy every single CSSValue then performance may
> regress significantly, so I won't touch it this time.  I should add a comment
> maybe?

I don't understand how a computed style is involved here. The CSSValueList you are copying is from resultValue. Can that be a a computed style?

[Ryosuke Niwa]

(In reply to comment #11)
> Needs a comment. !isValueList() is not an obvious way to check for "none".

I'm adding comment.

> > int getPrimitiveValue(CSSMutableStyleDeclaration* style, int propertyID)
> 
> I don't think that's quite the right name. A primitive value is not necessarily
> an identifier, and I think you need a name that mentions the word identifier.

How about getPrimitiveValueIdentifier then?
 
> > So it turned out that when we copy() computed style, it won't create a copy of
> > CSSValueList.
> > i.e. we will be changing the value of both mutable object and computed style.
> > 
> > Take a look at the lines 147-161 of CSSStyleDeclaration.  It only copies
> > CSSProperty but CSSProperty merely copies the RefPtr to CSSValue* which means
> > CSSValueList isn't really copied.  I think this is a bad implementation of
> > copy, but if we decided to copy every single CSSValue then performance may
> > regress significantly, so I won't touch it this time.  I should add a comment
> > maybe?
> 
> I don't understand how a computed style is involved here. The CSSValueList you
> are copying is from resultValue. Can that be a a computed style?

So we're creating mutable style by calling computedStyle->copy(). But then this copy function does NOT copy CSSValue object.  It merely copies the pointer. So if we alter the content of CSSValue, we'll be modifying the content of all computed / mutable styles that share this CSSValueList.


And it turned out that there is a bug that forces us to re-parse color.  I'll file a bug and possibility fix it.

[Darin Adler]

(In reply to comment #12)
> How about getPrimitiveValueIdentifier then?

I think getIdentifierValue would be even better. Returns the identifier of a value unless it's not an identifier in which case it returns 0.

> So we're creating mutable style by calling computedStyle->copy(). But then this
> copy function does NOT copy CSSValue object.  It merely copies the pointer. So
> if we alter the content of CSSValue, we'll be modifying the content of all
> computed / mutable styles that share this CSSValueList.

I think we should take the risk of slowing down performance and fix this in the style copy function. The code is getting more twisted and we need it to be less twisted. Complicated rules about when we need to copy create trouble.

> And it turned out that there is a bug that forces us to re-parse color.  I'll
> file a bug and possibility fix it.

OK.

[Ryosuke Niwa]

(In reply to comment #13)
> I think getIdentifierValue would be even better. Returns the identifier of a
> value unless it's not an identifier in which case it returns 0.

Ok, I'll use that.

> > So we're creating mutable style by calling computedStyle->copy(). But then this
> > copy function does NOT copy CSSValue object.  It merely copies the pointer. So
> > if we alter the content of CSSValue, we'll be modifying the content of all
> > computed / mutable styles that share this CSSValueList.
> 
> I think we should take the risk of slowing down performance and fix this in the
> style copy function. The code is getting more twisted and we need it to be less
> twisted. Complicated rules about when we need to copy create trouble.

Eric S. mentioned that we're trying to avoid copying computed styles and CSSValue to minimize memory usage and such.  I think changing copy method is quite expensive operation since it's used everywhere.  One idea Eric broguht up is to make the return value of getPropertyCSSValue const so that we don't accidently modify it.  I think it's quite misleading that it returns RefPtr.  I first though it'll return new object because of it.

+hyatt since he knows a lot about CSS.

[Darin Adler]

(In reply to comment #14)
> One idea Eric broguht up
> is to make the return value of getPropertyCSSValue const so that we don't
> accidently modify it.  I think it's quite misleading that it returns RefPtr.  I
> first though it'll return new object because of it.

It returns PassRefPtr because it returns a new object in the case of computed style.

I think we may want a different call for use when you have a mutable style.

Maybe we can move getPropertyCSSValue out of the base class into the mutable style derived class?

[Ryosuke Niwa]

(In reply to comment #13)
> > And it turned out that there is a bug that forces us to re-parse color.  I'll
> > file a bug and possibility fix it.
> 
> OK.

Filed:
CSSMutableStyleDeclaration::setProperty does not parse color property
https://bugs.webkit.org/show_bug.cgi?id=28282

[Ryosuke Niwa]

Created attachment 34791 [details]
fixed per comment

[Ryosuke Niwa]

(In reply to comment #15)
> It returns PassRefPtr because it returns a new object in the case of computed
> style.
> 
> I think we may want a different call for use when you have a mutable style.
> 
> Maybe we can move getPropertyCSSValue out of the base class into the mutable
> style derived class?

That might be a good call.  I'll look into it.  But that should be a separate patch though.  This cleanup is already getting pretty big.

[Ryosuke Niwa]

Talked with Eric S. in person.  It seems that we need duplicateCSSValues that deep-copies the content of a CSSStyleDeclaration.  For time being, we can implement this with just iterating through properties and call setProperty for each property.  But ultimately, we want copy method in each derived class of CSSValue to speed up this.

[Ryosuke Niwa]

(In reply to comment #19)
> Talked with Eric S. in person.  It seems that we need duplicateCSSValues that
> deep-copies the content of a CSSStyleDeclaration.  For time being, we can
> implement this with just iterating through properties and call setProperty for
> each property.  But ultimately, we want copy method in each derived class of
> CSSValue to speed up this.

Mn... this might not be as efficient as we want it to be.

> Maybe we can move getPropertyCSSValue out of the base class into the mutable
> style derived class?

Maybe that we should copy when getPropertyCSSValue is first called for the mutable style.  Either way, it's not clear to me whether we should copy every property at once or should copy on demand.

[Justin Garcia]

+        and adding functions cleanupTextDecorationProperties and extractTextStyle to handle the style.

Typo new name is reconcileTextDecorationProperties.

[Ryosuke Niwa]

Created attachment 34995 [details]
removed the fix on how text decorations are trimmed, no behavioral change

[Ryosuke Niwa]

Created attachment 34998 [details]
made formatted diff more readable

[Darin Adler]

Comment on attachment 34998 [details]
made formatted diff more readable

Changes look good. I'm saying r=me.

But doesn't this patch change the results of some layout tests? Where are the changes to the expected results?

[Ryosuke Niwa]

(In reply to comment #24)
> (From update of attachment 34998 [details])
> Changes look good. I'm saying r=me.
> 
> But doesn't this patch change the results of some layout tests? Where are the
> changes to the expected results?

No, the code I had in getPropertiesNotInComputedStyle was preparation to use it in other places.  But since that patch hasn't been submitted yet, there is no harm in replacing it with a simpler code.  In fact, that was why the bug survived there without being noticed by me.

[Eric Seidel (no email)]

Comment on attachment 34998 [details]
made formatted diff more readable

Clearing flags on attachment: 34998

Committed r47466: <http://trac.webkit.org/changeset/47466>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.