Reproduction steps: 1. Go to http://www.mozilla.org/editor/midasdemo/ 2. Type "hello" 3. Change the font size to 4 4. Change the font family to Courier Expected result: <font size="4" face="Courier">hello</font> Actual result: <font class="Apple-style-span" size="4"><font class="Apple-style-span" face="Courier">hello</font></font>
Created attachment 67265 [details] work in progress I need to rebaseline editing/inserting/insert-3659587-fix.html and editing/style/style-3690704-fix.html as well but I'll convert them to use runDumpAsTextEditingTest() first.
Created attachment 67331 [details] fixes the bug
Comment on attachment 67331 [details] fixes the bug I found a way to break this patch up. Will file a blocker bug with a patch tomorrow morning.
Created attachment 67706 [details] new patch
Comment on attachment 67706 [details] new patch View in context: https://bugs.webkit.org/attachment.cgi?id=67706&action=review In general, looks fine, just need some clarification. > WebCore/editing/ApplyStyleCommand.cpp:1162 > + if (!needToApplyStyle) > + return false; What happens if we don't return early? Do we end up with a redundant tag? > WebCore/editing/ApplyStyleCommand.cpp:1192 > + if (!element->parentNode() || !element->parentNode()->isContentEditable()) > + mode = RemoveNone; This seems strange to me. Why would we change the passed in mode? Should the method just return false instead or should this check be done by the caller? > WebCore/editing/ApplyStyleCommand.cpp:1879 > + Node* container = fontContainer ? fontContainer : startNode; Isn't fontContainer always null here?
Comment on attachment 67706 [details] new patch View in context: https://bugs.webkit.org/attachment.cgi?id=67706&action=review >> WebCore/editing/ApplyStyleCommand.cpp:1162 >> + return false; > > What happens if we don't return early? Do we end up with a redundant tag? No. It's quite opposite. When needToApplyStyle is false, the current run already has the desired style, and there's nothing to do. So we skip the run in applyInlineStyleToNodeRange (see http://trac.webkit.org/browser/trunk/WebCore/editing/ApplyStyleCommand.cpp#L1158). This is to avoid removing and adding the exact same markup. Of course, it implies that we leave the existing redundant styles in certain cases but I don't think that's a problem. When needToApplyStyle is true on the other hand, we go ahead and remove all styles that are either in conflict or same as the desired style from the current run so that there is no redundant markups present when we apply new style in applyInlineStyleToNodeRange. >> WebCore/editing/ApplyStyleCommand.cpp:1192 >> + mode = RemoveNone; > > This seems strange to me. Why would we change the passed in mode? Should the method just return false instead or should this check be done by the caller? We could do that. I need to look into the reason I added that check since I don't remember it anymore. >> WebCore/editing/ApplyStyleCommand.cpp:1879 >> + Node* container = fontContainer ? fontContainer : startNode; > > Isn't fontContainer always null here? Right. Will fix.
Created attachment 69286 [details] fixed per Tony's comment
Comment on attachment 69286 [details] fixed per Tony's comment View in context: https://bugs.webkit.org/attachment.cgi?id=69286&action=review r=me without the project.pbxproj change > WebCore/WebCore.xcodeproj/project.pbxproj:-20860 > - developmentRegion = English; I don't think you meant to include this. It caused the patch to now apply on the ews bots.
(In reply to comment #8) > (From update of attachment 69286 [details]) > > WebCore/WebCore.xcodeproj/project.pbxproj:-20860 > > - developmentRegion = English; > > I don't think you meant to include this. It caused the patch to now apply on the ews bots. Yeah, it was too late when I realized it. Will fix. Thanks for the timely review!
Committed r68830: <http://trac.webkit.org/changeset/68830>