Bug 30784 – WebKit cannot remove nested bold tags

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

[Ryosuke Niwa]

WebKit fails to remove bold tags from "<b><b>hello</b></b>".  Unbold it, and it produces "<b>hello</b>".  Similarly, unbolding "<b><b>hello</b> world</b>" results in "<b>hello world</b>".  We expect to get "hello" and "hello world" in respective cases.  This bug is due to the fact executeToggleStyle compares the font weight with "bold" but triStateOfStyleInComputedStyle returns the value 700.  Since "bold" != "700", triState is false and causes executeToogleStyle to bold the text instead of removing the bold.

[Ryosuke Niwa]

Created attachment 41926 [details]
fixes the bug by making getPropertiesNotInComputedStyle more flexible

[Ryosuke Niwa]

Created attachment 41927 [details]
fixes the bug by making getPropertiesNotInComputedStyle more flexible (fixed expected result)

[Ryosuke Niwa]

Since triStateOfStyleInComputedStyle calls getPropertiesNotInComputedStyle, I added new function getFontWeightValue in ApplyStyleCommand.  getPropertiesNotInComputedStyle removes font-weight property from the style whenever the return value of getFontWeightValue match with that of the computed style.  Because getFontWeightValue always converts the value of font-weight property to either 500 or 700, CSSValueBold and CSSValue700...900 will always match to each other.

[Darin Adler]

Comment on attachment 41927 [details]
fixes the bug by making getPropertiesNotInComputedStyle more flexible (fixed expected result)

> +int getFontWeightValue(CSSStyleDeclaration* style) {

Naming here does not match the WebKit coding style. We use nouns to name functions with return values, not "get". I also think the word "value" is not so good here in the name. There is already a font weight value, and it's a CSSValue object. The name of the function needs to describe what it does. It seems to me that this function actually just returns a boolean stating whether or not the font weight is a "bold" weight. So it would probably be better to make a function named fontWeightIsBold that returns a boolean. Or perhaps you have plans for the future that involve multiple font weights and therefore want to use the font weight numbers.

Formatting here does not match the WebKit coding style. The brace goes on the beginning of the next line.

I think the code needs a comment explaining exactly why "there are only two font-weights for rich text editing purposes". A concrete explanation of this.

> +        case CSSValue100    :
> +        case CSSValue200    :
> +        case CSSValue300    :
> +        case CSSValue400    :
> +        case CSSValue500    :
> +        case CSSValueNormal :
> +            return 500;
> +        case CSSValueBold   :
> +        case CSSValue600    :
> +        case CSSValue700    :
> +        case CSSValue800    :
> +        case CSSValue900    :
> +            return 700;

Formatting here does not match the WebKit coding style. We don't line up the colons like this.

Patch otherwise looks pretty good to me.

review- because of the issues mentioned above

[Ryosuke Niwa]

Created attachment 41975 [details]
fixed per Darin's comment

[Eric Seidel (no email)]

Comment on attachment 41975 [details]
fixed per Darin's comment

LGTM.

[Eric Seidel (no email)]

Yeah, the queue is kinda backed up at the moment.  It may be faster for you to land by hand.  Mostly it's backed up because the buildbots are so red. :(

[Ryosuke Niwa]

Landed as http://trac.webkit.org/changeset/50172.