Bug 23892 – execCommand("Underline") uses CSS even when styleWithCSS has been turned off

• Component: HTML Editing
• Status: RESOLVED FIXED

[Justin Garcia]

execCommand("Underline") always uses text-decoration.

<rdar://problem/6575858>

[Eric Seidel (no email)]

This is really the same underlying bug as bug 20215.  I think we could just dupe it.

[Ryosuke Niwa]

*** Bug 20828 has been marked as a duplicate of this bug. ***

[Ryosuke Niwa]

An edge case, what if we did execCommand('underline') on "world" as in:
<div style="text-decoration: underline;">hello <blockquote>world</blockquote> webkit</div>

We currently wrap hello with Style-span.  Should that also be styled by u?

[Ryosuke Niwa]

I already fixed this bug in my local checkout.  But patch can't be submitted until the patch for the bug 27749 is landed.

[Ryosuke Niwa]

Created attachment 38355 [details]
fix the bug.

This patch is large because it involves a lot of rebaselines but the change itself is fairly small.

[Eric Seidel (no email)]

Comment on attachment 38355 [details]
fix the bug.

I would have made the bool trimTextDecorations flag into an enum with two values instead.  That tends to make the callsites more readable.

Also, if "trimTextDecorations" only trims 'none' it should say that. 'TrimTextDecorationNone'

It seems this code could possibly be shared between the two callers:
         if (valueListInStyle->length())
 339             result->setProperty(textDecorationProperties[n], valueListInStyle->cssText());
 340         else
 341             result->removeProperty(textDecorationProperties[n]);

Spacing:
329     static const int textDecorationProperties[]={CSSPropertyTextDecoration, CSSPropertyWebkitTextDecorationsInEffect};

In general looks great.  I'd like to ask you about this in person when I'm in in a few minutes.  I'm not sure I fully understand the need to trim.

[Ryosuke Niwa]

Created attachment 38383 [details]
isolated code into helper functions

[Ryosuke Niwa]

It turned out I did enough cleanup on text decorations that trimTextDecorations is no longer needed.

[Eric Seidel (no email)]

Comment on attachment 38383 [details]
isolated code into helper functions

Looks good.

Two comments needed here:
+    else
+        style->removeProperty(propertyID);
+}

1.  That "text-decoration: none" is a noop.
2.  an ASSERT is needed to make sure the property is never !important.

[Ryosuke Niwa]

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