Bug 23892 - execCommand("Underline") uses CSS even when styleWithCSS has been turned off
Summary: execCommand("Underline") uses CSS even when styleWithCSS has been turned off
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac All
: P3 Minor
Assignee: Ryosuke Niwa
Keywords: InRadar
: 20828 (view as bug list)
Depends on: 20215 27749 27858 28471
  Show dependency treegraph
Reported: 2009-02-11 00:49 PST by Justin Garcia
Modified: 2009-08-21 14:54 PDT (History)
5 users (show)

See Also:

fix the bug. (79.81 KB, patch)
2009-08-20 22:36 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
isolated code into helper functions (78.90 KB, patch)
2009-08-21 11:25 PDT, Ryosuke Niwa
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Garcia 2009-02-11 00:49:54 PST
execCommand("Underline") always uses text-decoration.

Comment 1 Eric Seidel (no email) 2009-02-13 11:26:21 PST
This is really the same underlying bug as bug 20215.  I think we could just dupe it.
Comment 2 Ryosuke Niwa 2009-07-27 10:54:14 PDT
*** Bug 20828 has been marked as a duplicate of this bug. ***
Comment 3 Ryosuke Niwa 2009-08-13 18:28:54 PDT
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?
Comment 4 Ryosuke Niwa 2009-08-17 11:14:18 PDT
I already fixed this bug in my local checkout.  But patch can't be submitted until the patch for the bug 27749 is landed.
Comment 5 Ryosuke Niwa 2009-08-20 22:36:19 PDT
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.
Comment 6 Eric Seidel (no email) 2009-08-21 09:34:23 PDT
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]);

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.
Comment 7 Ryosuke Niwa 2009-08-21 11:25:11 PDT
Created attachment 38383 [details]
isolated code into helper functions
Comment 8 Ryosuke Niwa 2009-08-21 11:26:33 PDT
It turned out I did enough cleanup on text decorations that trimTextDecorations is no longer needed.
Comment 9 Eric Seidel (no email) 2009-08-21 11:51:08 PDT
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.
Comment 10 Ryosuke Niwa 2009-08-21 14:54:19 PDT
Landed as http://trac.webkit.org/changeset/47640.