REGRESSION(r46370-46426): /editing/style/remove-underline-from-stylesheet.html fails

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

http://trac.webkit.org/changeset/46373 caused /editing/style/remove-underline-from-stylesheet.html to fail (pixel test still passes because 1% tolerance allows underline to disappear).

[Ryosuke Niwa]

Created attachment 33732 [details]
demonstrates the bug

It seems like the bug only appears when text-decoration is set in style tag. with inline style, it removes underline successfully.  was there any change in how we treat inline-style & non-inline style?

[Eric Seidel (no email)]

So this was not caused by r46373?  Have we tried running bisect-builds?

[Ryosuke Niwa]

(In reply to comment #2)
> So this was not caused by r46373?  Have we tried running bisect-builds?

No.  I commented out all the code I changed, and the bug still reproduce.  The problem is that hasTextDecorationProperty doesn't return "underline" when the style is set in style tag.

@1085
static bool hasTextDecorationProperty(Node *node)
{
    if (!node->isElementNode())
        return false;

    RefPtr<CSSValue> value = computedStyle(node)->getPropertyCSSValue(CSSPropertyTextDecoration, DoNotUpdateLayout);
    return value && !equalIgnoringCase(value->cssText(), "none");
}

[Ryosuke Niwa]

wait a minute, I feel like our new behavior is correct.  Because if the style was set in style tag, then why should we changing that style?  In fact firefox does not remove underline either.

[Justin Garcia]

Yes, I agree with Ryosuke.  The text-decoration can not be negated because it cannot be pushed down, since it is on the editable root.

[Justin Garcia]

To make this test useful, we could put the text-decoration property on another element (with a style rule), one inside an editable region.

[Justin Garcia]

Actually I think it's that text-decoration cannot be safely pushed down if it comes from a style rule, regardless of whether or not it's on an editable root.

We might be able to negate text-decoration from a style rule if it's on an element that is fully selected, like if you had foo<span>bar</bar>baz with span { text-decoration: underline; } and everything was selected and you did execCommand("Underline"), which would be worth testing.

[Justin Garcia]

Actually I think it's that text-decoration cannot be pushed down if it comes from a style rule, regardless of whether or not it's on an editable root.

We might be able to negate text-decoration from a style rule if it's on an element that is fully selected, like if you had foo<span>bar</bar>baz with span { text-decoration: underline; } and everything was selected and you did execCommand("Underline"), which would be worth testing.

[Ryosuke Niwa]

Created attachment 33747 [details]
fixes the bug

[Ryosuke Niwa]

The patch is large only because I converted two pixel tests to dumpAsText tests in order to detect disappearance of underline in future.

[Ryosuke Niwa]

The corresponding Chromium bug.
http://code.google.com/p/chromium/issues/detail?id=18014

Thanks to [e-mail] who told me about this regression.

[Justin Garcia]

Comment on attachment 33747 [details]
fixes the bug

"set by u, s, & stirke"

Small typo.  And I'd use "and" instead of &, but it's up to you.

r=me

[Ryosuke Niwa]

Landed in http://trac.webkit.org/changeset/46567