With the following markup: <u><b>one two three</b></u>, select the word 'two' and execCommand('underline'). EXPECTED: the word 'two' is bold and NOT underlined ACTUAL: the word 'two' is bold AND underlined
Created attachment 65338 [details] Test case When running this test, the word 'and' should be bold but not underlined.
(In reply to comment #1) > Created an attachment (id=65338) [details] > Test case > > When running this test, the word 'and' should be bold but not underlined. I just found this bug in pushDownInlineStyleAroundNode while working on the bug 30836. Will work on it ASAP. Thanks for filing a bug.
Wait, this is a different bug. We have never been able to push text-decoration down when text-decoration is added by u or s. Notice that WebKit removes underline if your markup was: <span style="text-decoration: underline;"><b>one two three</b></span>
This regression was introduced in http://trac.webkit.org/changeset/46373 where if (alteredProperty.cssText() != "none") was correctly changed to if (!equalIgnoringCase(alteredProperty.value()->cssText(), "none")) . The original if statement was comparing the full css text (text-decoration: none) with none, whereas the second one compares only the value of the property. The change that introduced this was http://trac.webkit.org/changeset/46286 with the intent of removing redundant text-decoration: none. Unfortunately the attached example shows a case where it is necessary to add text-decoration:none to the <u> element, to allow all the children to have text-decoration:underline inline when needed. The current version of AppleStyleCommand.cpp is very different from this one, so I need to figure out how to change it. The code before ttp://trac.webkit.org/changeset/46286 was pushing the text decoration down successfully (see the attached test case for a meaningful example).
(In reply to comment #4) > Unfortunately the attached example shows a case where it is necessary to add text-decoration:none to the <u> element, to allow all the children to have text-decoration:underline inline when needed. The correct fix is to remove u element entirely. I'm working on a patch I speak. Give me 1-2 days and will post a patch.
(In reply to comment #5) > (In reply to comment #4) > > Unfortunately the attached example shows a case where it is necessary to add text-decoration:none to the <u> element, to allow all the children to have text-decoration:underline inline when needed. > > The correct fix is to remove u element entirely. I'm working on a patch I speak. Give me 1-2 days and will post a patch. Oh, I didn't you assigned yourself. I can post work-in-progress patch if you'd like to take over.
Created attachment 65383 [details] work in progress
Most of it is cleanup. I'm combining implicitlyStyledElementShouldBeRemovedWhenApplyingStyle, removeHTMLFontStyle, and removeHTMLBidiEmbeddingStyle into removeImplicitlyStyledElement so that I can add a feature to extract styles later. This cleanup is ~60% of patch and should be isolated into a separate patch. The actual fix is in extractInlineStyleToPushDown where I call removeImplicitlyStyledElement in two places. We have to extract inline declaration first because we won't be able to remove the node otherwise when implicitly styled element has style attribute. Assuming I separate the cleanup, this fix should be very simple and small. The only test that fails with this patch is editing/style/typing-style-003.html and I suspect that we're getting a simpler markup because the result is visually identical. I'll file a bug to convert it to a dump-as-markup / dump-as-text test first.
(In reply to comment #8) > Most of it is cleanup. I'm combining implicitlyStyledElementShouldBeRemovedWhenApplyingStyle, removeHTMLFontStyle, and removeHTMLBidiEmbeddingStyle into removeImplicitlyStyledElement so that I can add a feature to extract styles later. This cleanup is ~60% of patch and should be isolated into a separate patch. > > The actual fix is in extractInlineStyleToPushDown where I call removeImplicitlyStyledElement in two places. We have to extract inline declaration first because we won't be able to remove the node otherwise when implicitly styled element has style attribute. Assuming I separate the cleanup, this fix should be very simple and small. > > The only test that fails with this patch is editing/style/typing-style-003.html and I suspect that we're getting a simpler markup because the result is visually identical. I'll file a bug to convert it to a dump-as-markup / dump-as-text test first. The patch looks good to me (other than the commented code left :-)). The markup generated is even better in my opinion. Do you think you can finish the cleanup and land it soon? You could use that repro case I attached as starting point for the test to be added with the patch. Also, I don't think you need to file a separate bug for the test that fails, it simply needs to be re-baselined since the generated markup is different. Thanks for taking this on!!
(In reply to comment #9) > The patch looks good to me (other than the commented code left :-)). The markup generated is even better in my opinion. Do you think you can finish the cleanup and land it soon? Sure. I'll file cleanup & conversion bugs today so that someone can review those. If those two bugs were to be resolved by Thursday, I should be able to fix the bug this week. > You could use that repro case I attached as starting point for the test to be added with the patch. Yeah, I need a whole bunch of tests here. > Also, I don't think you need to file a separate bug for the test that fails, it simply needs to be re-baselined since the generated markup is different. Right, but I'd like to convert editing tests to dump-as-text / dump-as-markup slowly because they're much more readable and have single expected results for all platforms. > Thanks for taking this on!! No problem. After all, it seems like my regression so happy to fix it.
I have filed bugs for test conversion and cleanup each with a corresponding patch. As soon as those two patches are landed, I'll post a patch for this bug.
Created attachment 65745 [details] work in progress 2
Created attachment 65758 [details] fixes the bug by pushing down all implicitly styled elements
Attachment 65758 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/ChangeLog:11: Line contains tab character. [whitespace/tab] [5] WebCore/ChangeLog:12: Line contains tab character. [whitespace/tab] [5] WebCore/ChangeLog:14: Line contains tab character. [whitespace/tab] [5] WebCore/ChangeLog:15: Line contains tab character. [whitespace/tab] [5] WebCore/ChangeLog:16: Line contains tab character. [whitespace/tab] [5] WebCore/ChangeLog:17: Line contains tab character. [whitespace/tab] [5] WebCore/ChangeLog:19: Line contains tab character. [whitespace/tab] [5] WebCore/ChangeLog:20: Line contains tab character. [whitespace/tab] [5] WebCore/ChangeLog:21: Line contains tab character. [whitespace/tab] [5] Total errors found: 9 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 65759 [details] fixed change logs and the expected result for push-down-implicit-styles
Comment on attachment 65759 [details] fixed change logs and the expected result for push-down-implicit-styles > Index: WebCore/ChangeLog > =================================================================== > --- WebCore/ChangeLog (revision 66198) > +++ WebCore/ChangeLog (working copy) > @@ -1,3 +1,37 @@ > +2010-08-27 Ryosuke Niwa <rniwa@webkit.org> > + > + Reviewed by NOBODY (OOPS!). > + > + cannot remove text-decoration when style is added by u or s > + https://bugs.webkit.org/show_bug.cgi?id=44560 > + > + The bug was caused by WebKit not pushing down implicitly added styles to descendent nodes. > + Fixed the bug by pushing down all elements that implicitly add style. > + > + extractInlineStyleToPushDown now calls removeImplicitlyStyledElement, which was modified to extract > + the style implicitly added by the element to extractStyle. > + > + This revealed a bug in applyInlineStyleToPushDown where applyInlineStyleIfNeeded could add > + an implicitly styled element inside an element with a conflicting style, thereby overriding the style of the element. > + Fixed this by extending the logic to honor the existing inline style declaration used in > + the case of rewriting inline style declaration to all cases including the one calling addInlineStyleIfNeeded. > + > + Also fixed a bug in removeInlineStyle where pushDownInlineStyleAroundNode was called on a text node > + outside of selection if start was at the end of the text node. > + > + Test: editing/style/push-down-implicit-styles.html > + > + * editing/ApplyStyleCommand.cpp: > + (WebCore::ApplyStyleCommand::removeImplicitlyStyledElement): Added extractedStyle, which receives > + the style implicitly added by the element being removed. > + (WebCore::ApplyStyleCommand::extractInlineStyleToPushDown): Calls removeImplicitlyStyledElement > + to remove the implicitly styled element and extract the inline style added by the element. > + (WebCore::ApplyStyleCommand::applyInlineStyleToPushDown): Fixed a bug that addInlineStyleIfNeeded > + could override the inline style declaration style of the node. > + (WebCore::ApplyStyleCommand::pushDownInlineStyleAroundNode): Uses extractInlineStyleToPushDown > + (WebCore::ApplyStyleCommand::removeInlineStyle): See above. > + * editing/ApplyStyleCommand.h: Prototype changes. > + > 2010-08-27 Yury Semikhatsky <yurys@chromium.org> > > Reviewed by Pavel Feldman. > Index: WebCore/editing/ApplyStyleCommand.cpp > =================================================================== > --- WebCore/editing/ApplyStyleCommand.cpp (revision 66189) > +++ WebCore/editing/ApplyStyleCommand.cpp (working copy) > @@ -1197,15 +1197,17 @@ static const HTMLEquivalent HTMLEquivale > { CSSPropertyDirection, false, CSSValueInvalid, 0, &dirAttr, ShouldNotBePushedDown }, > }; > > -bool ApplyStyleCommand::removeImplicitlyStyledElement(CSSMutableStyleDeclaration* style, HTMLElement* element, InlineStyleRemovalMode mode) > +bool ApplyStyleCommand::removeImplicitlyStyledElement(CSSMutableStyleDeclaration* style, HTMLElement* element, InlineStyleRemovalMode mode, CSSMutableStyleDeclaration* extractedStyle) > { > + // Current implementation does not support stylePushedDown when mode == RemoveNone because of early exit. > + ASSERT(!extractedStyle || mode != RemoveNone); > bool removed = false; > for (size_t i = 0; i < sizeof(HTMLEquivalents) / sizeof(HTMLEquivalent); i++) { > const HTMLEquivalent& equivalent = HTMLEquivalents[i]; > ASSERT(equivalent.element || equivalent.attribute); > - if (equivalent.element && !element->hasTagName(*equivalent.element)) > - continue; > - if (equivalent.attribute && !element->hasAttribute(*equivalent.attribute)) > + if (extractedStyle && equivalent.pushDownType == ShouldNotBePushedDown > + || equivalent.element && !element->hasTagName(*equivalent.element) > + || equivalent.attribute && !element->hasAttribute(*equivalent.attribute)) > continue; > > RefPtr<CSSValue> styleValue = style->getPropertyCSSValue(equivalent.propertyID); > @@ -1218,6 +1220,14 @@ bool ApplyStyleCommand::removeImplicitly > else if (styleValue->cssText() == mapValue->cssText()) > continue; // If CSS value is primitive, then skip if they are equal. > > + if (extractedStyle) { > + ASSERT(equivalent.pushDownType != ShouldNotBePushedDown); I think the assert above is redundant. > + if (equivalent.primitiveId == CSSValueInvalid) > + extractedStyle->setProperty(equivalent.propertyID, element->getAttribute(*equivalent.attribute)); > + else > + extractedStyle->setProperty(equivalent.propertyID, mapValue->cssText()); > + } > + > if (mode == RemoveNone) > return true; > > @@ -1314,7 +1324,7 @@ HTMLElement* ApplyStyleCommand::highestA > return result; > } > > -PassRefPtr<CSSMutableStyleDeclaration> ApplyStyleCommand::extractInlineStyleToPushDown(Node* node, bool isStyledElement, const Vector<int>& properties) > +PassRefPtr<CSSMutableStyleDeclaration> ApplyStyleCommand::extractInlineStyleToPushDown(CSSMutableStyleDeclaration* styleToApply, Node* node, bool isStyledElement) > { > ASSERT(node); > ASSERT(node->isElementNode()); > @@ -1330,8 +1340,16 @@ PassRefPtr<CSSMutableStyleDeclaration> A > return style.release(); > } > > - if (!style) > - return 0; > + if (!style) { > + style = CSSMutableStyleDeclaration::create(); > + removeImplicitlyStyledElement(styleToApply, element, RemoveAttributesAndElements, style.get()); > + return style.release(); > + } > + > + Vector<int> properties; > + CSSMutableStyleDeclaration::const_iterator end = styleToApply->end(); > + for (CSSMutableStyleDeclaration::const_iterator it = styleToApply->begin(); it != end; ++it) > + properties.append(it->id()); > > style = style->copyPropertiesInSet(properties.data(), properties.size()); > for (size_t i = 0; i < properties.size(); i++) { > @@ -1346,6 +1364,8 @@ PassRefPtr<CSSMutableStyleDeclaration> A > if (isSpanWithoutAttributesOrUnstyleStyleSpan(element)) > removeNodePreservingChildren(element); > > + removeImplicitlyStyledElement(styleToApply, element, RemoveAttributesAndElements, style.get()); > + > return style.release(); > } > > @@ -1356,15 +1376,14 @@ void ApplyStyleCommand::applyInlineStyle > if (!style || !style->length() || !node->renderer()) > return; > > - // Since addInlineStyleIfNeeded can't add styles to block-flow render objects, add style attribute instead. > - // FIXME: applyInlineStyleToRange should be used here instead. > - if ((node->renderer()->isBlockFlow() || node->childNodeCount()) && node->isHTMLElement()) { > + RefPtr<CSSMutableStyleDeclaration> newInlineStyle = style; > + if (node->isHTMLElement()) { > HTMLElement* element = static_cast<HTMLElement*>(node); > CSSMutableStyleDeclaration* existingInlineStyle = element->inlineStyleDecl(); > > // Avoid overriding existing styles of node > if (existingInlineStyle) { > - RefPtr<CSSMutableStyleDeclaration> newInlineStyle = existingInlineStyle->copy(); > + newInlineStyle = existingInlineStyle->copy(); > CSSMutableStyleDeclaration::const_iterator end = style->end(); > for (CSSMutableStyleDeclaration::const_iterator it = style->begin(); it != end; ++it) { > ExceptionCode ec; > @@ -1391,22 +1410,23 @@ void ApplyStyleCommand::applyInlineStyle > } > } > } > + } > + } > > - setNodeAttribute(element, styleAttr, newInlineStyle->cssText()); > - } else > - setNodeAttribute(element, styleAttr, style->cssText()); > - > + // Since addInlineStyleIfNeeded can't add styles to block-flow render objects, add style attribute instead. > + // FIXME: applyInlineStyleToRange should be used here instead. > + if ((node->renderer()->isBlockFlow() || node->childNodeCount()) && node->isHTMLElement()) { > + setNodeAttribute(static_cast<HTMLElement*>(node), styleAttr, newInlineStyle->cssText()); > return; > } > > if (node->renderer()->isText() && static_cast<RenderText*>(node->renderer())->isAllCollapsibleWhitespace()) > return; > > - // FIXME: addInlineStyleIfNeeded may override the style of node > // We can't wrap node with the styled element here because new styled element will never be removed if we did. > // If we modified the child pointer in pushDownInlineStyleAroundNode to point to new style element > // then we fall into an infinite loop where we keep removing and adding styled element wrapping node. > - addInlineStyleIfNeeded(style, node, node, DoNotAddStyledElement); > + addInlineStyleIfNeeded(newInlineStyle.get(), node, node, DoNotAddStyledElement); > } > > void ApplyStyleCommand::pushDownInlineStyleAroundNode(CSSMutableStyleDeclaration* style, Node* targetNode) > @@ -1415,11 +1435,6 @@ void ApplyStyleCommand::pushDownInlineSt > if (!highestAncestor) > return; > > - Vector<int> properties; > - CSSMutableStyleDeclaration::const_iterator end = style->end(); > - for (CSSMutableStyleDeclaration::const_iterator it = style->begin(); it != end; ++it) > - properties.append(it->id()); > - > // The outer loop is traversing the tree vertically from highestAncestor to targetNode > Node* current = highestAncestor; > while (current != targetNode) { > @@ -1431,7 +1446,7 @@ void ApplyStyleCommand::pushDownInlineSt > RefPtr<StyledElement> styledElement; > if (current->isStyledElement() && m_styledInlineElement && current->hasTagName(m_styledInlineElement->tagQName())) > styledElement = static_cast<StyledElement*>(current); > - RefPtr<CSSMutableStyleDeclaration> styleToPushDown = extractInlineStyleToPushDown(current, styledElement, properties); > + RefPtr<CSSMutableStyleDeclaration> styleToPushDown = extractInlineStyleToPushDown(style, current, styledElement); > > // The inner loop will go through children on each level > // FIXME: we should aggregate inline child elements together so that we don't wrap each child separately. > @@ -1483,6 +1498,12 @@ void ApplyStyleCommand::removeInlineStyl > } > > Position pushDownStart = start.downstream(); > + // If the pushDownStart is at the end of a text node, then this node is not fully selected. > + // Move it to the next deep quivalent position to avoid removing the style from this node. > + // e.g. if pushDownStart was at Position("hello", 5) in <b>hello<div>world</div></b>, we want Position("world", 0) instead. > + Node* pushDownStartContainer = pushDownStart.containerNode(); > + if (pushDownStartContainer && pushDownStartContainer->isTextNode() && pushDownStart.computeOffsetInContainerNode() == pushDownStartContainer->maxCharacterOffset()) I would split this over two lines to ease of reading. > + pushDownStart = nextVisuallyDistinctCandidate(pushDownStart); > Position pushDownEnd = end.upstream(); > pushDownInlineStyleAroundNode(style.get(), pushDownStart.node()); > pushDownInlineStyleAroundNode(style.get(), pushDownEnd.node()); > Index: WebCore/editing/ApplyStyleCommand.h > =================================================================== > --- WebCore/editing/ApplyStyleCommand.h (revision 66189) > +++ WebCore/editing/ApplyStyleCommand.h (working copy) > @@ -73,11 +73,11 @@ private: > // style-removal helpers > bool removeInlineStyleFromElement(CSSMutableStyleDeclaration*, HTMLElement*, InlineStyleRemovalMode = RemoveAttributesAndElements); > inline bool shouldRemoveInlineStyleFromElement(CSSMutableStyleDeclaration* style, HTMLElement* element) {return removeInlineStyleFromElement(style, element, RemoveNone);} > - bool removeImplicitlyStyledElement(CSSMutableStyleDeclaration*, HTMLElement*, InlineStyleRemovalMode); > + bool removeImplicitlyStyledElement(CSSMutableStyleDeclaration*, HTMLElement*, InlineStyleRemovalMode, CSSMutableStyleDeclaration* extractedStyle = 0); > void replaceWithSpanOrRemoveIfWithoutAttributes(HTMLElement*&); > bool removeCSSStyle(CSSMutableStyleDeclaration*, HTMLElement*, InlineStyleRemovalMode = RemoveAttributesAndElements); > HTMLElement* highestAncestorWithConflictingInlineStyle(CSSMutableStyleDeclaration*, Node*); > - PassRefPtr<CSSMutableStyleDeclaration> extractInlineStyleToPushDown(Node*, bool isStyledElement, const Vector<int>&); > + PassRefPtr<CSSMutableStyleDeclaration> extractInlineStyleToPushDown(CSSMutableStyleDeclaration*, Node*, bool isStyledElement); > void applyInlineStyleToPushDown(Node*, CSSMutableStyleDeclaration *style); > void pushDownInlineStyleAroundNode(CSSMutableStyleDeclaration*, Node*); > void removeInlineStyle(PassRefPtr<CSSMutableStyleDeclaration>, const Position& start, const Position& end); > Index: LayoutTests/ChangeLog > =================================================================== > --- LayoutTests/ChangeLog (revision 66198) > +++ LayoutTests/ChangeLog (working copy) > @@ -1,3 +1,25 @@ > +2010-08-27 Ryosuke Niwa <rniwa@webkit.org> > + > + Reviewed by NOBODY (OOPS!). > + > + cannot remove text-decoration when style is added by u or s > + https://bugs.webkit.org/show_bug.cgi?id=44560 > + > + Added a test to ensure WebKit removes text-decorations added by u or s > + and pushes down implicit styles properly into descendent nodes. > + > + * editing/style/push-down-implicit-styles-expected.txt: Added. > + * editing/style/push-down-implicit-styles.html: Added. > + * editing/style/script-tests/push-down-implicit-styles.js: Added. > + (testSingleToggle): > + (selectAll): > + (selectTest): > + (selectSecondWord): > + (selectLastTwoWords): > + * editing/style/typing-style-003-expected.txt: Negation of inline > + styles are now done by pushing down implicitly styled elements rather than > + canceling them by inline style declarations. > + > 2010-08-27 Sheriff Bot <webkit.review.bot@gmail.com> > > Unreviewed, rolling out r66188. > Index: LayoutTests/editing/style/push-down-implicit-styles-expected.txt > =================================================================== > --- LayoutTests/editing/style/push-down-implicit-styles-expected.txt (revision 0) > +++ LayoutTests/editing/style/push-down-implicit-styles-expected.txt (revision 0) > @@ -0,0 +1,30 @@ > +Test to make sure we push down inline styles properly. > + > +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". > + > + > +PASS bold on first word of <b><div>hello</div> world</b> yields <div>hello</div><b> world</b> > +PASS bold on first word of <b><div>hello</div>world</b> yields <div>hello</div><b>world</b> > +PASS bold on first word of <b><div>hello</div><em>world</em></b> yields <div>hello</div><em style="font-weight: bold; ">world</em> > +PASS bold on second word of <b>hello <div>world</div></b> yields <b>hello </b><div>world</div> > +PASS bold on second word of <b><em>hello</em> <div>world</div></b> yields <em style="font-weight: bold; ">hello</em> <div>world</div> > +PASS bold on all of <b> <div>text</div> </b> yields <div>text</div> > +PASS bold on all of <b><s><div>text</div></s></b> yields <s><div>text</div></s> > +PASS bold on first word of <b><div>hello</div><div>world</div></b> yields <div>hello</div><div style="font-weight: bold; ">world</div> > +PASS bold on first word of <b><div>hello</div><div style="font-weight: normal;">world</div>webkit</b> yields <div>hello</div><div style="font-weight: normal; ">world</div><b>webkit</b> > +PASS bold on second word of <b style="font-style: italic;">hello world</b> yields <b style="font-style: italic;">hello</b><span style="font-style: italic;"> world</span> > +PASS underline on second word of <u>hello <b>world</b> webkit</u> yields <u>hello</u> <b>world</b><u> webkit</u> > +PASS underline on last two words of <u>hello <b>world</b> webkit</u> yields <u>hello </u><b>world</b> webkit > +PASS underline on last two words of <u>hello <b>world webkit</b></u> yields <u>hello </u><b>world webkit</b> > +PASS underline on second word of <u>hello <b>world webkit</b></u> yields <u>hello</u> <b>world<u> webkit</u></b> > +PASS underline on second word of <u><b>hello world</b> webkit</u> yields <b><u>hello</u> world</b><u> webkit</u> > +PASS underline on second word of <u><s>hello world</s></u> yields <s><u>hello</u> world</s> > +PASS underline on second word of <u><s>hello world webkit</s></u> yields <s><u>hello</u> world<u> webkit</u></s> > +PASS underline on second word of <u><s>hello world</s> webkit</u> yields <s><u>hello</u> world</s><u> webkit</u> > +PASS underline on second word of <u>hello <em><code>world webkit</code></em> rocks</u> yields <u>hello</u> <em><code>world<u> webkit</u></code></em><u> rocks</u> > +PASS strikeThrough on all of <s style="color: blue;">hello world</s> yields <span style="color: blue;">hello world</span> > +PASS strikeThrough on first word of <s style="color: blue;"><div>hello</div> <b>world</b> webkit</s> yields <span style="color: blue;"><div>hello</div> <b style="text-decoration: line-through; ">world</b><s> webkit</s></span> > +PASS successfullyParsed is true > + > +TEST COMPLETE > + > Index: LayoutTests/editing/style/push-down-implicit-styles.html > =================================================================== > --- LayoutTests/editing/style/push-down-implicit-styles.html (revision 0) > +++ LayoutTests/editing/style/push-down-implicit-styles.html (revision 0) > @@ -0,0 +1,13 @@ > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> > +<html> > +<head> > +<link rel="stylesheet" href="../../fast/js/resources/js-test-style.css"> > +<script src="../../fast/js/resources/js-test-pre.js"></script> > +</head> > +<body> > +<p id="description"></p> > +<div id="console"></div> > +<script src="script-tests/push-down-implicit-styles.js"></script> > +<script src="../../fast/js/resources/js-test-post.js"></script> > +</body> > +</html> > Index: LayoutTests/editing/style/typing-style-003-expected.txt > =================================================================== > --- LayoutTests/editing/style/typing-style-003-expected.txt (revision 66189) > +++ LayoutTests/editing/style/typing-style-003-expected.txt (working copy) > @@ -31,23 +31,23 @@ EDITING DELEGATE: shouldChangeSelectedDO > EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification > EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification > EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification > -EDITING DELEGATE: shouldChangeSelectedDOMRange:(null) toDOMRange:range from 1 of #text > SPAN > I > B > DIV > BODY > HTML > #document to 1 of #text > SPAN > I > B > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE > +EDITING DELEGATE: shouldChangeSelectedDOMRange:(null) toDOMRange:range from 1 of #text > I > DIV > BODY > HTML > #document to 1 of #text > I > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE > EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification > EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification > -EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 1 of #text > SPAN > I > B > DIV > BODY > HTML > #document to 1 of #text > SPAN > I > B > DIV > BODY > HTML > #document toDOMRange:range from 2 of #text > SPAN > I > B > DIV > BODY > HTML > #document to 2 of #text > SPAN > I > B > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE > +EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 1 of #text > I > DIV > BODY > HTML > #document to 1 of #text > I > DIV > BODY > HTML > #document toDOMRange:range from 2 of #text > I > DIV > BODY > HTML > #document to 2 of #text > I > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE > EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification > EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification > -EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 2 of #text > SPAN > I > B > DIV > BODY > HTML > #document to 2 of #text > SPAN > I > B > DIV > BODY > HTML > #document toDOMRange:range from 3 of #text > SPAN > I > B > DIV > BODY > HTML > #document to 3 of #text > SPAN > I > B > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE > +EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 2 of #text > I > DIV > BODY > HTML > #document to 2 of #text > I > DIV > BODY > HTML > #document toDOMRange:range from 3 of #text > I > DIV > BODY > HTML > #document to 3 of #text > I > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE > EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification > EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification > EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification > -EDITING DELEGATE: shouldChangeSelectedDOMRange:(null) toDOMRange:range from 1 of #text > SPAN > SPAN > I > B > DIV > BODY > HTML > #document to 1 of #text > SPAN > SPAN > I > B > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE > +EDITING DELEGATE: shouldChangeSelectedDOMRange:(null) toDOMRange:range from 1 of #text > DIV > BODY > HTML > #document to 1 of #text > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE > EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification > EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification > -EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 1 of #text > SPAN > SPAN > I > B > DIV > BODY > HTML > #document to 1 of #text > SPAN > SPAN > I > B > DIV > BODY > HTML > #document toDOMRange:range from 2 of #text > SPAN > SPAN > I > B > DIV > BODY > HTML > #document to 2 of #text > SPAN > SPAN > I > B > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE > +EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 1 of #text > DIV > BODY > HTML > #document to 1 of #text > DIV > BODY > HTML > #document toDOMRange:range from 2 of #text > DIV > BODY > HTML > #document to 2 of #text > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE > EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification > EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification > -EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 2 of #text > SPAN > SPAN > I > B > DIV > BODY > HTML > #document to 2 of #text > SPAN > SPAN > I > B > DIV > BODY > HTML > #document toDOMRange:range from 3 of #text > SPAN > SPAN > I > B > DIV > BODY > HTML > #document to 3 of #text > SPAN > SPAN > I > B > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE > +EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 2 of #text > DIV > BODY > HTML > #document to 2 of #text > DIV > BODY > HTML > #document toDOMRange:range from 3 of #text > DIV > BODY > HTML > #document to 3 of #text > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE > EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification > EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification > xxxxxxxxxxxxxxx > @@ -63,10 +63,10 @@ execTypeCharacterCommand: xxx<b>xxx<i>x< > execTypeCharacterCommand: xxx<b>xxx<i>xx</i></b><span id="test"></span> > execTypeCharacterCommand: xxx<b>xxx<i>xxx</i></b><span id="test"></span> > execBoldCommand: xxx<b>xxx<i>xxx</i></b><span id="test"></span> > -execTypeCharacterCommand: xxx<b>xxx<i>xxx<span class="Apple-style-span" style="font-weight: normal;">x</span></i></b><span id="test"></span> > -execTypeCharacterCommand: xxx<b>xxx<i>xxx<span class="Apple-style-span" style="font-weight: normal;">xx</span></i></b><span id="test"></span> > -execTypeCharacterCommand: xxx<b>xxx<i>xxx<span class="Apple-style-span" style="font-weight: normal;">xxx</span></i></b><span id="test"></span> > -execItalicCommand: xxx<b>xxx<i>xxx<span class="Apple-style-span" style="font-weight: normal;">xxx</span></i></b><span id="test"></span> > -execTypeCharacterCommand: xxx<b>xxx<i>xxx<span class="Apple-style-span" style="font-weight: normal;">xxx<span class="Apple-style-span" style="font-style: normal;">x</span></span></i></b><span id="test"></span> > -execTypeCharacterCommand: xxx<b>xxx<i>xxx<span class="Apple-style-span" style="font-weight: normal;">xxx<span class="Apple-style-span" style="font-style: normal;">xx</span></span></i></b><span id="test"></span> > -execTypeCharacterCommand: xxx<b>xxx<i>xxx<span class="Apple-style-span" style="font-weight: normal;">xxx<span class="Apple-style-span" style="font-style: normal;">xxx</span></span></i></b><span id="test"></span> > +execTypeCharacterCommand: xxx<b>xxx</b><i><b>xxx</b>x</i><span id="test"></span> > +execTypeCharacterCommand: xxx<b>xxx</b><i><b>xxx</b>xx</i><span id="test"></span> > +execTypeCharacterCommand: xxx<b>xxx</b><i><b>xxx</b>xxx</i><span id="test"></span> > +execItalicCommand: xxx<b>xxx</b><i><b>xxx</b>xxx</i><span id="test"></span> > +execTypeCharacterCommand: xxx<b>xxx</b><i><b>xxx</b>xxx</i>x<span id="test"></span> > +execTypeCharacterCommand: xxx<b>xxx</b><i><b>xxx</b>xxx</i>xx<span id="test"></span> > +execTypeCharacterCommand: xxx<b>xxx</b><i><b>xxx</b>xxx</i>xxx<span id="test"></span> I assume the result is visually equivalent. Correct? > Index: LayoutTests/editing/style/script-tests/push-down-implicit-styles.js > =================================================================== > --- LayoutTests/editing/style/script-tests/push-down-implicit-styles.js (revision 0) > +++ LayoutTests/editing/style/script-tests/push-down-implicit-styles.js (revision 0) > @@ -0,0 +1,76 @@ > +description('Test to make sure we push down inline styles properly.'); > + > +var testContainer = document.createElement("div"); > +testContainer.contentEditable = true; > +document.body.appendChild(testContainer); > + > +function testSingleToggle(toggleCommand, selector, initialContents, expectedContents) > +{ > + testContainer.innerHTML = initialContents; > + var selected = selector(testContainer); > + document.execCommand('styleWithCSS', false, 'false'); > + document.execCommand(toggleCommand, false, null); > + var action = toggleCommand + ' on ' + selected + ' of ' + initialContents + " yields " + testContainer.innerHTML; > + if (testContainer.innerHTML == expectedContents) > + testPassed(action); > + else > + testFailed(action + ", expected " + expectedContents); > +} > + > +function selectAll(container) { > + window.getSelection().selectAllChildren(container); > + return 'all'; > +} > + > +function selectTest(container) { > + window.getSelection().selectAllChildren(document.getElementById('test')); > + return 'test'; > +} > + > +function selectFirstWord(container) { > + window.getSelection().setPosition(container, 0); > + window.getSelection().modify('extend', 'forward', 'word'); > + return 'first word'; > +} > + > +function selectSecondWord(container) { > + window.getSelection().setPosition(container, 0); > + window.getSelection().modify('move', 'forward', 'word'); > + window.getSelection().modify('extend', 'forward', 'word'); > + return 'second word'; > +} > + > +function selectLastTwoWords(container) { > + window.getSelection().setPosition(container, container.childNodes.length); > + window.getSelection().modify('extend', 'backward', 'word'); > + window.getSelection().modify('extend', 'backward', 'word'); > + return 'last two words'; > +} > + > +testSingleToggle("bold", selectFirstWord, '<b><div>hello</div> world</b>', '<div>hello</div><b> world</b>'); > +testSingleToggle("bold", selectFirstWord, '<b><div>hello</div>world</b>', '<div>hello</div><b>world</b>'); > +testSingleToggle("bold", selectFirstWord, '<b><div>hello</div><em>world</em></b>', '<div>hello</div><em style="font-weight: bold; ">world</em>'); > +testSingleToggle("bold", selectSecondWord, '<b>hello <div>world</div></b>', '<b>hello </b><div>world</div>'); > +testSingleToggle("bold", selectSecondWord, '<b><em>hello</em> <div>world</div></b>', '<em style="font-weight: bold; ">hello</em> <div>world</div>'); > +testSingleToggle("bold", selectAll, '<b> <div>text</div> </b>', ' <div>text</div> '); > +testSingleToggle("bold", selectAll, '<b><s><div>text</div></s></b>', '<s><div>text</div></s>'); > +testSingleToggle("bold", selectFirstWord, '<b><div>hello</div><div>world</div></b>', '<div>hello</div><div style="font-weight: bold; ">world</div>'); > +testSingleToggle("bold", selectFirstWord, '<b><div>hello</div><div style="font-weight: normal;">world</div>webkit</b>', '<div>hello</div><div style="font-weight: normal; ">world</div><b>webkit</b>'); > +testSingleToggle("bold", selectSecondWord, '<b style="font-style: italic;">hello world</b>', '<b style="font-style: italic;">hello</b><span style="font-style: italic;"> world</span>'); > + > +testSingleToggle("underline", selectSecondWord, '<u>hello <b>world</b> webkit</u>', '<u>hello</u> <b>world</b><u> webkit</u>'); > +testSingleToggle("underline", selectLastTwoWords, '<u>hello <b>world</b> webkit</u>', '<u>hello </u><b>world</b> webkit'); > +testSingleToggle("underline", selectLastTwoWords, '<u>hello <b>world webkit</b></u>', '<u>hello </u><b>world webkit</b>'); > +testSingleToggle("underline", selectSecondWord, '<u>hello <b>world webkit</b></u>', '<u>hello</u> <b>world<u> webkit</u></b>'); > +testSingleToggle("underline", selectSecondWord, '<u><b>hello world</b> webkit</u>', '<b><u>hello</u> world</b><u> webkit</u>'); > +testSingleToggle("underline", selectSecondWord, '<u><s>hello world</s></u>', '<s><u>hello</u> world</s>'); > +testSingleToggle("underline", selectSecondWord, '<u><s>hello world webkit</s></u>', '<s><u>hello</u> world<u> webkit</u></s>'); > +testSingleToggle("underline", selectSecondWord, '<u><s>hello world</s> webkit</u>', '<s><u>hello</u> world</s><u> webkit</u>'); > +testSingleToggle("underline", selectSecondWord, '<u>hello <em><code>world webkit</code></em> rocks</u>', '<u>hello</u> <em><code>world<u> webkit</u></code></em><u> rocks</u>'); > + > +testSingleToggle("strikeThrough", selectAll, '<s style="color: blue;">hello world</s>', '<span style="color: blue;">hello world</span>'); > +testSingleToggle("strikeThrough", selectFirstWord, '<s style="color: blue;"><div>hello</div> <b>world</b> webkit</s>', '<span style="color: blue;"><div>hello</div> <b style="text-decoration: line-through; ">world</b><s> webkit</s></span>'); > + > +document.body.removeChild(testContainer); > + > +var successfullyParsed = true; The change looks good and there is a lot more testing now :-)! My comments are just nit picks. Thanks for working on this.
(In reply to comment #16) > > -bool ApplyStyleCommand::removeImplicitlyStyledElement(CSSMutableStyleDeclaration* style, HTMLElement* element, InlineStyleRemovalMode mode) > > +bool ApplyStyleCommand::removeImplicitlyStyledElement(CSSMutableStyleDeclaration* style, HTMLElement* element, InlineStyleRemovalMode mode, CSSMutableStyleDeclaration* extractedStyle) > > { > > + // Current implementation does not support stylePushedDown when mode == RemoveNone because of early exit. > > + ASSERT(!extractedStyle || mode != RemoveNone); > > bool removed = false; > > for (size_t i = 0; i < sizeof(HTMLEquivalents) / sizeof(HTMLEquivalent); i++) { > > const HTMLEquivalent& equivalent = HTMLEquivalents[i]; > > ASSERT(equivalent.element || equivalent.attribute); > > - if (equivalent.element && !element->hasTagName(*equivalent.element)) > > - continue; > > - if (equivalent.attribute && !element->hasAttribute(*equivalent.attribute)) > > + if (extractedStyle && equivalent.pushDownType == ShouldNotBePushedDown > > + || equivalent.element && !element->hasTagName(*equivalent.element) > > + || equivalent.attribute && !element->hasAttribute(*equivalent.attribute)) > > continue; > > > > RefPtr<CSSValue> styleValue = style->getPropertyCSSValue(equivalent.propertyID); > > @@ -1218,6 +1220,14 @@ bool ApplyStyleCommand::removeImplicitly > > else if (styleValue->cssText() == mapValue->cssText()) > > continue; // If CSS value is primitive, then skip if they are equal. > > > > + if (extractedStyle) { > > + ASSERT(equivalent.pushDownType != ShouldNotBePushedDown); > > I think the assert above is redundant. Ok, I wanted to signify the fact above continue is required for this block because I was afraid that this condition wasn't clear from the content. But I can remove if you think it's self-evident. > > Position pushDownStart = start.downstream(); > > + // If the pushDownStart is at the end of a text node, then this node is not fully selected. > > + // Move it to the next deep quivalent position to avoid removing the style from this node. > > + // e.g. if pushDownStart was at Position("hello", 5) in <b>hello<div>world</div></b>, we want Position("world", 0) instead. > > + Node* pushDownStartContainer = pushDownStart.containerNode(); > > + if (pushDownStartContainer && pushDownStartContainer->isTextNode() && pushDownStart.computeOffsetInContainerNode() == pushDownStartContainer->maxCharacterOffset()) > I would split this over two lines to ease of reading. Will do. > > xxxxxxxxxxxxxxx > > @@ -63,10 +63,10 @@ execTypeCharacterCommand: xxx<b>xxx<i>x< > > execTypeCharacterCommand: xxx<b>xxx<i>xx</i></b><span id="test"></span> > > execTypeCharacterCommand: xxx<b>xxx<i>xxx</i></b><span id="test"></span> > > execBoldCommand: xxx<b>xxx<i>xxx</i></b><span id="test"></span> > > -execTypeCharacterCommand: xxx<b>xxx<i>xxx<span class="Apple-style-span" style="font-weight: normal;">x</span></i></b><span id="test"></span> > > -execTypeCharacterCommand: xxx<b>xxx<i>xxx<span class="Apple-style-span" style="font-weight: normal;">xx</span></i></b><span id="test"></span> > > -execTypeCharacterCommand: xxx<b>xxx<i>xxx<span class="Apple-style-span" style="font-weight: normal;">xxx</span></i></b><span id="test"></span> > > -execItalicCommand: xxx<b>xxx<i>xxx<span class="Apple-style-span" style="font-weight: normal;">xxx</span></i></b><span id="test"></span> > > -execTypeCharacterCommand: xxx<b>xxx<i>xxx<span class="Apple-style-span" style="font-weight: normal;">xxx<span class="Apple-style-span" style="font-style: normal;">x</span></span></i></b><span id="test"></span> > > -execTypeCharacterCommand: xxx<b>xxx<i>xxx<span class="Apple-style-span" style="font-weight: normal;">xxx<span class="Apple-style-span" style="font-style: normal;">xx</span></span></i></b><span id="test"></span> > > -execTypeCharacterCommand: xxx<b>xxx<i>xxx<span class="Apple-style-span" style="font-weight: normal;">xxx<span class="Apple-style-span" style="font-style: normal;">xxx</span></span></i></b><span id="test"></span> > > +execTypeCharacterCommand: xxx<b>xxx</b><i><b>xxx</b>x</i><span id="test"></span> > > +execTypeCharacterCommand: xxx<b>xxx</b><i><b>xxx</b>xx</i><span id="test"></span> > > +execTypeCharacterCommand: xxx<b>xxx</b><i><b>xxx</b>xxx</i><span id="test"></span> > > +execItalicCommand: xxx<b>xxx</b><i><b>xxx</b>xxx</i><span id="test"></span> > > +execTypeCharacterCommand: xxx<b>xxx</b><i><b>xxx</b>xxx</i>x<span id="test"></span> > > +execTypeCharacterCommand: xxx<b>xxx</b><i><b>xxx</b>xxx</i>xx<span id="test"></span> > > +execTypeCharacterCommand: xxx<b>xxx</b><i><b>xxx</b>xxx</i>xxx<span id="test"></span> > I assume the result is visually equivalent. Correct? Yes. Notice that the test result isn't unchanged until the line: execBoldCommand: xxx<b>xxx<i>xxx</i></b><span id="test"></span> Here, we're removing the bold from the typing style. We then type xxx, we should put that xxx outside of b so we have: execTypeCharacterCommand: xxx<b>xxx</b><i><b>xxx</b>xxx</i><span id="test"></span> instead of execTypeCharacterCommand: xxx<b>xxx<i>xxx<span class="Apple-style-span" style="font-weight: normal;">xxx</span></i></b><span id="test"></span> Now, after typing xxx, we remove the italic from the typing style as well to get: execItalicCommand: xxx<b>xxx</b><i><b>xxx</b>xxx</i><span id="test"></span> Typing xxx here gives: execTypeCharacterCommand: xxx<b>xxx</b><i><b>xxx</b>xxx</i>xxx<span id="test"></span> which is visually identical to execTypeCharacterCommand: xxx<b>xxx<i>xxx<span class="Apple-style-span" style="font-weight: normal;">xxx<span class="Apple-style-span" style="font-style: normal;">xxx</span></span></i></b><span id="test"></span> IMHO, the new expected result looks nicer. > > +testSingleToggle("bold", selectFirstWord, '<b><div>hello</div> world</b>', '<div>hello</div><b> world</b>'); > > +testSingleToggle("bold", selectFirstWord, '<b><div>hello</div>world</b>', '<div>hello</div><b>world</b>'); > > +testSingleToggle("bold", selectFirstWord, '<b><div>hello</div><em>world</em></b>', '<div>hello</div><em style="font-weight: bold; ">world</em>'); > > +testSingleToggle("bold", selectSecondWord, '<b>hello <div>world</div></b>', '<b>hello </b><div>world</div>'); > > +testSingleToggle("bold", selectSecondWord, '<b><em>hello</em> <div>world</div></b>', '<em style="font-weight: bold; ">hello</em> <div>world</div>'); > > +testSingleToggle("bold", selectAll, '<b> <div>text</div> </b>', ' <div>text</div> '); > > +testSingleToggle("bold", selectAll, '<b><s><div>text</div></s></b>', '<s><div>text</div></s>'); > > +testSingleToggle("bold", selectFirstWord, '<b><div>hello</div><div>world</div></b>', '<div>hello</div><div style="font-weight: bold; ">world</div>'); > > +testSingleToggle("bold", selectFirstWord, '<b><div>hello</div><div style="font-weight: normal;">world</div>webkit</b>', '<div>hello</div><div style="font-weight: normal; ">world</div><b>webkit</b>'); > > +testSingleToggle("bold", selectSecondWord, '<b style="font-style: italic;">hello world</b>', '<b style="font-style: italic;">hello</b><span style="font-style: italic;"> world</span>'); > > + > > +testSingleToggle("underline", selectSecondWord, '<u>hello <b>world</b> webkit</u>', '<u>hello</u> <b>world</b><u> webkit</u>'); > > +testSingleToggle("underline", selectLastTwoWords, '<u>hello <b>world</b> webkit</u>', '<u>hello </u><b>world</b> webkit'); > > +testSingleToggle("underline", selectLastTwoWords, '<u>hello <b>world webkit</b></u>', '<u>hello </u><b>world webkit</b>'); > > +testSingleToggle("underline", selectSecondWord, '<u>hello <b>world webkit</b></u>', '<u>hello</u> <b>world<u> webkit</u></b>'); > > +testSingleToggle("underline", selectSecondWord, '<u><b>hello world</b> webkit</u>', '<b><u>hello</u> world</b><u> webkit</u>'); > > +testSingleToggle("underline", selectSecondWord, '<u><s>hello world</s></u>', '<s><u>hello</u> world</s>'); > > +testSingleToggle("underline", selectSecondWord, '<u><s>hello world webkit</s></u>', '<s><u>hello</u> world<u> webkit</u></s>'); > > +testSingleToggle("underline", selectSecondWord, '<u><s>hello world</s> webkit</u>', '<s><u>hello</u> world</s><u> webkit</u>'); > > +testSingleToggle("underline", selectSecondWord, '<u>hello <em><code>world webkit</code></em> rocks</u>', '<u>hello</u> <em><code>world<u> webkit</u></code></em><u> rocks</u>'); > > + > > +testSingleToggle("strikeThrough", selectAll, '<s style="color: blue;">hello world</s>', '<span style="color: blue;">hello world</span>'); > > +testSingleToggle("strikeThrough", selectFirstWord, '<s style="color: blue;"><div>hello</div> <b>world</b> webkit</s>', '<span style="color: blue;"><div>hello</div> <b style="text-decoration: line-through; ">world</b><s> webkit</s></span>'); > > + > > +document.body.removeChild(testContainer); > > + > > +var successfullyParsed = true; > > The change looks good and there is a lot more testing now :-)! > My comments are just nit picks. > Thanks for working on this. Thanks for the comments.
Created attachment 65765 [details] fixed per enrica's comment
Committed as http://trac.webkit.org/changeset/66324.