Steps to reproduce: 1. Go to Midas Demo: http://www.mozilla.org/editor/midasdemo/ 2. Type a word, say, "test". 3. Press the Insert Link button and create a link. 4. Press Enter. Html now looks like this: <a href="http://www.test.com">test</a><div><br></div> 5. With the cursor on the new line, press backspace. Actual result: Newline removed, but underline is removed from link. HTML looks like this: <a href="http://www.test.com" style="text-decoration: none;">test</a> Expected result: Newline removed, text-decoration not changed.
FYI, I can repro this in Safari 4 beta and Chrome 2, but not on Safari 3.2
I've been looking for an excuse to fix a webkit bug, and this one looks like it could be a good first bug for me. Unless Eric or Justin know off the top of their heads why this would have broken, I'm happy to look into it.
text-decoration is handled special by ApplyStyleCommand (-webkit-text-decorations-in-effect or something). But I've not re-written it yet... ApplyStyleCommand might not even be involved here. I'm not sure why a delete would be applying style to the previous line. So it's all yours, unless Justin has some insight. Since this is a regression, it's a P1 according to the guide. It would help a lot to know which revision regressed. If you can make a layout test to test this you can use WebKitTools/Scripts/bisect-builds to quickly run through the nightly builds and find out which revision range caused the regression. ;)
Or if you're a git user, once you figure out which nightly range caused the regression, then you can use git's more brute-force "git bisect" command to find the exact change which caused the failure. I find thumbing through the revision range in question is generally faster than using git bisect though. :)
Note: it doesn't need to be an anchor for this to occur. For example, replacing the a with a div with any text-decoration has the same broken behavior.
I've identified where this regression came from. It was introduced with the changes in trunk/WebCore/editing/CompositeEditCommand.cpp to preserve empty paragraph style when moving paragraphs in http://trac.webkit.org/changeset/34681/trunk/WebCore/editing. Given my original example HTML (<a href="http://www.test.com">test</a><div><br></div>), with the cursor in the last div, in move paragraphs, the style is calculated for that empty paragraph (it has no special styles, so text-decoration:none), the empty paragraph is removed, and then the empty paragraph style is applied to the move destination (the text node inside the anchor) by applyInlineStyle, which calls removeInlineStyle, which sees text-decoration:none, which calls pushDownTextDecorationStyleAtBoundaries, which calls pushDownTextDecorationStyleAroundNode(start.node(), true) (start is the text node inside the anchor). This code finds the anchor ancestor which does have text decoration, and calls extractAndNegateTextDecorationStyle, which negates it, setting text-decoration:none on the anchor. Thus, we wind up with text-decoration:none on the anchor. Anyway, that is what is happening. I don't really know which part is the actual *bug* though. I'm happy to keep digging, but since I don't know this code at all, I'd love advice from Justin or Eric on how to proceed here ...
> which calls removeInlineStyle, which sees text-decoration:none, which calls > pushDownTextDecorationStyleAtBoundaries, which calls > pushDownTextDecorationStyleAroundNode(start.node(), true) (start is the text > node inside the anchor). This code finds the anchor ancestor which does have > text decoration, and calls extractAndNegateTextDecorationStyle, which negates > it, setting text-decoration:none on the anchor. Thus, we wind up with > text-decoration:none on the anchor. The bug is that when/after we push down text decoration, we should only be removing it from the input range. Seems like pushDownTextDecorationStyleAtBoundaries and friends make the assumption that the start position will be in a node that is necessarily part of that range, because of work that applyInlineStyle did to split the text nodes that contained the start and end. Noticed that this happens with plain old underlined text too, which makes sense.
Actually, I don't think moveParagraphs should be preserving the style of an empty paragraph in this case: // Restore styles from an empty paragraph to the new empty paragraph. if (styleInEmptyParagraph) applyStyle(styleInEmptyParagraph.get()); notice "to the new empty paragraph". I also think that in general, the moveParagraphs that deletion does shouldn't preserve the style of empty paragraphs. In TextEdit, do: <return> <bold> <delete> "hello" "hello" isn't bolded. I think that moving the preserving the style of an empty paragraph should be conditionalized behavior of moveParagraphs, and only happen for indenting, outdenting and perhaps a few other operations, so that we don't break the change made in: http://trac.webkit.org/changeset/34681
> Actually, I don't think moveParagraphs should be preserving the style of an > empty paragraph in this case: This should have read "in this case. See:".
I checked in revision 42869 to address a duplicate of this bug - sorry, I didn't see this one. Justin mentioned that there may be some cases that this change doesn't address. Could you guys try it out, and let me know what you find?
I think my patches have fixed this bug. It does not reproduce on Chrome 3.0.195. Could someone with Mac try it on the nightly build of WebKit? And if it passes, please add a test case that fails on old WebKit.
The reported bug no longer reproduces on TOT.