Bug 27809 - REGRESSION(r46370-46426): /editing/style/remove-underline-from-stylesheet.html fails
Summary: REGRESSION(r46370-46426): /editing/style/remove-underline-from-stylesheet.htm...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P1 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-29 12:24 PDT by Ryosuke Niwa
Modified: 2009-07-29 17:15 PDT (History)
4 users (show)

See Also:


Attachments
demonstrates the bug (821 bytes, text/html)
2009-07-29 12:42 PDT, Ryosuke Niwa
no flags Details
fixes the bug (76.47 KB, patch)
2009-07-29 16:06 PDT, Ryosuke Niwa
justin.garcia: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2009-07-29 12:24:52 PDT
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).
Comment 1 Ryosuke Niwa 2009-07-29 12:42:23 PDT
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?
Comment 2 Eric Seidel (no email) 2009-07-29 13:09:03 PDT
So this was not caused by r46373?  Have we tried running bisect-builds?
Comment 3 Ryosuke Niwa 2009-07-29 13:32:13 PDT
(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");
}
Comment 4 Ryosuke Niwa 2009-07-29 13:37:24 PDT
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.
Comment 5 Justin Garcia 2009-07-29 14:08:57 PDT
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.
Comment 6 Justin Garcia 2009-07-29 14:09:41 PDT
To make this test useful, we could put the text-decoration property on another element (with a style rule), one inside an editable region.
Comment 7 Justin Garcia 2009-07-29 15:17:52 PDT
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.
Comment 8 Justin Garcia 2009-07-29 15:17:52 PDT
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.
Comment 9 Ryosuke Niwa 2009-07-29 16:06:16 PDT
Created attachment 33747 [details]
fixes the bug
Comment 10 Ryosuke Niwa 2009-07-29 16:09:00 PDT
The patch is large only because I converted two pixel tests to dumpAsText tests in order to detect disappearance of underline in future.
Comment 11 Ryosuke Niwa 2009-07-29 16:12:19 PDT
The corresponding Chromium bug.
http://code.google.com/p/chromium/issues/detail?id=18014

Thanks to pkasting@chromium.org who told me about this regression.
Comment 12 Justin Garcia 2009-07-29 16:33:36 PDT
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
Comment 13 Ryosuke Niwa 2009-07-29 17:15:47 PDT
Landed in http://trac.webkit.org/changeset/46567