execCommand("Underline") always uses text-decoration. <rdar://problem/6575858>
This is really the same underlying bug as bug 20215. I think we could just dupe it.
*** Bug 20828 has been marked as a duplicate of this bug. ***
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?
I already fixed this bug in my local checkout. But patch can't be submitted until the patch for the bug 27749 is landed.
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 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.
Created attachment 38383 [details] isolated code into helper functions
It turned out I did enough cleanup on text decorations that trimTextDecorations is no longer needed.
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.
Landed as http://trac.webkit.org/changeset/47640.