Changeset 98794 in webkit


Ignore:
Timestamp:
Oct 28, 2011, 10:27:07 PM (14 years ago)
Author:
rniwa@webkit.org
Message:

The copy and paste result in nested scrollbars on http://dojotoolkit.org/widgets
https://bugs.webkit.org/show_bug.cgi?id=70799

Reviewed by Enrica Casucci.

Source/WebCore:

The bug was caused by WebKit's treating a fully selected root with background property as a special common ancestor.
A variant of this bug was caused by treating any element with text-decoration property as a presentational element.

Fixed the above two bugs by not serializing the said nodes. The effective background color was already serialized
by wrappingStyleForSerialization, there was nothing to be done besides stop including it in highestAncestorToWrapMarkup.

For text-decoration property, added the logic to compute the effective value in EditingStyle::init. Also treat it
as a non-inheritable editing property so that the rest of EditingStyle just works.

Test: editing/pasteboard/avoid-copying-body-with-background.html

  • editing/EditingStyle.cpp: Added CSSPropertyTextDecoration to the list of editing properties.

(WebCore::copyEditingProperties):
(WebCore::EditingStyle::init): Compute the effective text decoration when propertiesToInclude is
EditingPropertiesInEffect.
(WebCore::EditingStyle::prepareToApplyAt):
(WebCore::EditingStyle::mergeInlineStyleOfElement):
(WebCore::EditingStyle::wrappingStyleForSerialization):
(WebCore::EditingStyle::removeStyleFromRulesAndContext):

  • editing/EditingStyle.h: Renamed EditingInheritablePropertiesAndBackgroundColorInEffect to

EditingPropertiesInEffect.

  • editing/markup.cpp:

(WebCore::StyledMarkupAccumulator::appendStyleNodeOpenTag): Removed an assertion that's no longer valid.
(WebCore::isElementPresentational): Don't consider an element with text-decoration as a presentational element.
(WebCore::highestAncestorToWrapMarkup): Don't consider fully selected root as a special common ancestor ever.
Background color is computed property when we compute the wrapping style.
(WebCore::createMarkup):

LayoutTests:

Added a test to copy contents inside a body with background and text-decoration properties.

WebKit should not copy body element.

  • editing/deleting/delete-line-break-before-underlined-content-expected.txt: an erroneous inline div

is replaced by a span.

  • editing/deleting/deleting-line-break-preserves-underline-color-expected.txt: two style spans are

merged into one.

  • editing/pasteboard/19644-2-expected.txt: div is replaced by span. This is okay because it's the only

content in the body. Even though we now only put the gray background under text as inline style as
opposed to apply at the block level, that's what execCommand('BackColor'...) does and what user expects.

  • editing/pasteboard/avoid-copying-body-with-background-expected.txt: Added.
  • editing/pasteboard/avoid-copying-body-with-background.html: Added.
  • editing/pasteboard/preserve-underline-color-expected.txt:
  • platform/mac/editing/pasteboard/5134759-expected.txt:
Location:
trunk
Files:
2 added
10 edited

Legend:

Unmodified
Added
Removed
  • TabularUnified trunk/LayoutTests/ChangeLog

    r98791 r98794  
     12011-10-28  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        The copy and paste result in nested scrollbars on http://dojotoolkit.org/widgets
     4        https://bugs.webkit.org/show_bug.cgi?id=70799
     5
     6        Reviewed by Enrica Casucci.
     7
     8        Added a test to copy contents inside a body with background and text-decoration properties.
     9
     10        WebKit should not copy body element.
     11
     12        * editing/deleting/delete-line-break-before-underlined-content-expected.txt: an erroneous inline div
     13        is replaced by a span.
     14        * editing/deleting/deleting-line-break-preserves-underline-color-expected.txt: two style spans are
     15        merged into one.
     16        * editing/pasteboard/19644-2-expected.txt: div is replaced by span. This is okay because it's the only
     17        content in the body. Even though we now only put the gray background under text as inline style as
     18        opposed to apply at the block level, that's what execCommand('BackColor'...) does and what user expects.
     19        * editing/pasteboard/avoid-copying-body-with-background-expected.txt: Added.
     20        * editing/pasteboard/avoid-copying-body-with-background.html: Added.
     21        * editing/pasteboard/preserve-underline-color-expected.txt:
     22        * platform/mac/editing/pasteboard/5134759-expected.txt:
     23
    1242011-10-28  Adam Klein  <adamk@chromium.org>
    225
  • TabularUnified trunk/LayoutTests/editing/deleting/delete-line-break-before-underlined-content-expected.txt

    r86605 r98794  
    11This tests for a bug where underlined content would lose its underliningwhen deleting the line break before the paragraph that contained it.
    22| "This shouldn't be underlined.<#selection-caret>"
    3 | <div>
    4 |   id="div"
    5 |   style="text-decoration: underline; display: inline !important; "
     3| <span>
     4|   style="text-decoration: underline; "
    65|   "This should be underlined."
  • TabularUnified trunk/LayoutTests/editing/deleting/deleting-line-break-preserves-underline-color-expected.txt

    r96870 r98794  
    1414|   "This should not be underlined.<#selection-caret>"
    1515|   <span>
    16 |     style="text-decoration: underline; color: blue;"
    17 |     <span>
    18 |       style="color:red;"
    19 |       "This should be underlined."
     16|     style="color: red; text-decoration: underline; "
     17|     "This should be underlined."
  • TabularUnified trunk/LayoutTests/editing/pasteboard/19644-2-expected.txt

    r42722 r98794  
    1 <div style="background-color: rgb(187, 187, 187); ">This tests to make sure that we wrap copied markup in a div to hold a fully selected body's attributes and style when the that body has a background-color. If you copy and paste this text into Mail, it should have a grey background.</div>
     1<span style="background-color: rgb(187, 187, 187); ">This tests to make sure that we wrap copied markup in a div to hold a fully selected body's attributes and style when the that body has a background-color. If you copy and paste this text into Mail, it should have a grey background.</span>
  • TabularUnified trunk/LayoutTests/editing/pasteboard/preserve-underline-color-expected.txt

    r86619 r98794  
    11This test for a bug copy/pasting underlined text. The color of the underline should be the color of the element that has the text-decoration property.
    22| <span>
    3 |   style="text-decoration: underline; color: blue; "
    4 |   <span>
    5 |     style="color: red; "
    6 |     "This should be underlined.<#selection-caret>"
     3|   style="color: rgb(255, 0, 0); text-decoration: underline; "
     4|   "This should be underlined.<#selection-caret>"
    75| <br>
  • TabularUnified trunk/LayoutTests/platform/mac/editing/pasteboard/5134759-expected.txt

    r63291 r98794  
    1818            text run at (0,0) width 39: "Hello "
    1919          RenderInline {SPAN} at (0,0) size 45x18
    20             RenderInline {DIV} at (0,0) size 45x18
    21               RenderText {#text} at (39,0) size 45x18
    22                 text run at (39,0) width 45: "World!"
     20            RenderText {#text} at (39,0) size 45x18
     21              text run at (39,0) width 45: "World!"
    2322        RenderBlock (anonymous) at (0,18) size 784x0
    24 caret: position 6 of child 0 {#text} of child 0 {DIV} of child 1 {SPAN} of child 0 {DIV} of child 2 {DIV} of body
     23caret: position 6 of child 0 {#text} of child 1 {SPAN} of child 0 {DIV} of child 2 {DIV} of body
  • TabularUnified trunk/Source/WebCore/ChangeLog

    r98793 r98794  
     12011-10-28  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        The copy and paste result in nested scrollbars on http://dojotoolkit.org/widgets
     4        https://bugs.webkit.org/show_bug.cgi?id=70799
     5
     6        Reviewed by Enrica Casucci.
     7
     8        The bug was caused by WebKit's treating a fully selected root with background property as a special common ancestor.
     9        A variant of this bug was caused by treating any element with text-decoration property as a presentational element.
     10
     11        Fixed the above two bugs by not serializing the said nodes. The effective background color was already serialized
     12        by wrappingStyleForSerialization, there was nothing to be done besides stop including it in highestAncestorToWrapMarkup.
     13
     14        For text-decoration property, added the logic to compute the effective value in EditingStyle::init. Also treat it
     15        as a non-inheritable editing property so that the rest of EditingStyle just works.
     16
     17        Test: editing/pasteboard/avoid-copying-body-with-background.html
     18
     19        * editing/EditingStyle.cpp: Added CSSPropertyTextDecoration to the list of editing properties.
     20        (WebCore::copyEditingProperties):
     21        (WebCore::EditingStyle::init): Compute the effective text decoration when propertiesToInclude is
     22        EditingPropertiesInEffect.
     23        (WebCore::EditingStyle::prepareToApplyAt):
     24        (WebCore::EditingStyle::mergeInlineStyleOfElement):
     25        (WebCore::EditingStyle::wrappingStyleForSerialization):
     26        (WebCore::EditingStyle::removeStyleFromRulesAndContext):
     27        * editing/EditingStyle.h: Renamed EditingInheritablePropertiesAndBackgroundColorInEffect to
     28        EditingPropertiesInEffect.
     29        * editing/markup.cpp:
     30        (WebCore::StyledMarkupAccumulator::appendStyleNodeOpenTag): Removed an assertion that's no longer valid.
     31        (WebCore::isElementPresentational): Don't consider an element with text-decoration as a presentational element.
     32        (WebCore::highestAncestorToWrapMarkup): Don't consider fully selected root as a special common ancestor ever.
     33        Background color is computed property when we compute the wrapping style.
     34        (WebCore::createMarkup):
     35
    1362011-10-28  Adam Barth  <abarth@webkit.org>
    237
  • TabularUnified trunk/Source/WebCore/editing/EditingStyle.cpp

    r97480 r98794  
    5656static const int editingProperties[] = {
    5757    CSSPropertyBackgroundColor,
     58    CSSPropertyTextDecoration,
    5859
    5960    // CSS inheritable properties
     
    8081};
    8182
    82 static PassRefPtr<CSSMutableStyleDeclaration> copyEditingProperties(CSSStyleDeclaration* style, bool includeBackgroundColor = false)
    83 {
    84     if (includeBackgroundColor)
     83static PassRefPtr<CSSMutableStyleDeclaration> copyEditingProperties(CSSStyleDeclaration* style, bool includeNonInheritableProperties = false)
     84{
     85    if (includeNonInheritableProperties)
    8586        return style->copyPropertiesInSet(editingProperties, WTF_ARRAY_LENGTH(editingProperties));
    86     return style->copyPropertiesInSet(editingProperties + 1, WTF_ARRAY_LENGTH(editingProperties) - 1);
     87    return style->copyPropertiesInSet(editingProperties + 2, WTF_ARRAY_LENGTH(editingProperties) - 2);
    8788}
    8889
     
    363364    m_mutableStyle = propertiesToInclude == AllProperties && computedStyleAtPosition ? computedStyleAtPosition->copy() : editingStyleFromComputedStyle(computedStyleAtPosition);
    364365
    365     if (propertiesToInclude == EditingInheritablePropertiesAndBackgroundColorInEffect) {
     366    if (propertiesToInclude == EditingPropertiesInEffect) {
    366367        if (RefPtr<CSSValue> value = backgroundColorInEffect(node))
    367368            m_mutableStyle->setProperty(CSSPropertyBackgroundColor, value->cssText());
     369        if (RefPtr<CSSValue> value = computedStyleAtPosition->getPropertyCSSValue(CSSPropertyWebkitTextDecorationsInEffect))
     370            m_mutableStyle->setProperty(CSSPropertyTextDecoration, value->cssText());
    368371    }
    369372
     
    855858    // If this function was modified in the future to delete all redundant properties, then add a boolean value to indicate
    856859    // which one of editingStyleAtPosition or computedStyle is called.
    857     RefPtr<EditingStyle> style = EditingStyle::create(position, EditingInheritablePropertiesAndBackgroundColorInEffect);
     860    RefPtr<EditingStyle> style = EditingStyle::create(position, EditingPropertiesInEffect);
    858861
    859862    RefPtr<CSSValue> unicodeBidi;
     
    901904        return;
    902905    case OnlyEditingInheritableProperties:
    903     case EditingInheritablePropertiesAndBackgroundColorInEffect:
    904         mergeStyle(copyEditingProperties(element->inlineStyleDecl(), propertiesToInclude == EditingInheritablePropertiesAndBackgroundColorInEffect).get(), mode);
     906    case EditingPropertiesInEffect:
     907        mergeStyle(copyEditingProperties(element->inlineStyleDecl(), propertiesToInclude == EditingPropertiesInEffect).get(), mode);
    905908        return;
    906909    }
     
    937940    RefPtr<EditingStyle> wrappingStyle;
    938941    if (shouldAnnotate) {
    939         wrappingStyle = EditingStyle::create(context, EditingStyle::EditingInheritablePropertiesAndBackgroundColorInEffect);
     942        wrappingStyle = EditingStyle::create(context, EditingStyle::EditingPropertiesInEffect);
    940943
    941944        // Styles that Mail blockquotes contribute should only be placed on the Mail blockquote,
     
    956959        if (node->isStyledElement()) {
    957960            wrappingStyle->mergeInlineAndImplicitStyleOfElement(static_cast<StyledElement*>(node), EditingStyle::DoNotOverrideValues,
    958                 EditingStyle::EditingInheritablePropertiesAndBackgroundColorInEffect);
     961                EditingStyle::EditingPropertiesInEffect);
    959962        }
    960963    }
     
    10691072
    10701073    // 2. Remove style present in context and not overriden by matched rules.
    1071     RefPtr<EditingStyle> computedStyle = EditingStyle::create(context, EditingInheritablePropertiesAndBackgroundColorInEffect);
     1074    RefPtr<EditingStyle> computedStyle = EditingStyle::create(context, EditingPropertiesInEffect);
    10721075    if (computedStyle->m_mutableStyle) {
    10731076        computedStyle->removePropertiesInElementDefaultStyle(element);
  • TabularUnified trunk/Source/WebCore/editing/EditingStyle.h

    r97480 r98794  
    6262public:
    6363
    64     enum PropertiesToInclude { AllProperties, OnlyEditingInheritableProperties, EditingInheritablePropertiesAndBackgroundColorInEffect };
     64    enum PropertiesToInclude { AllProperties, OnlyEditingInheritableProperties, EditingPropertiesInEffect };
    6565    enum ShouldPreserveWritingDirection { PreserveWritingDirection, DoNotPreserveWritingDirection };
    6666    enum ShouldExtractMatchingStyle { ExtractMatchingStyle, DoNotExtractMatchingStyle };
  • TabularUnified trunk/Source/WebCore/editing/markup.cpp

    r97499 r98794  
    186186void StyledMarkupAccumulator::appendStyleNodeOpenTag(StringBuilder& out, CSSStyleDeclaration* style, Document* document, bool isBlock)
    187187{
    188     // All text-decoration-related elements should have been treated as special ancestors
    189     // If we ever hit this ASSERT, we should export StyleChange in ApplyStyleCommand and use it here
    190     ASSERT(propertyMissingOrEqualToNone(style, CSSPropertyTextDecoration) && propertyMissingOrEqualToNone(style, CSSPropertyWebkitTextDecorationsInEffect));
     188    // wrappingStyleForSerialization should have removed -webkit-text-decorations-in-effect
     189    ASSERT(propertyMissingOrEqualToNone(style, CSSPropertyWebkitTextDecorationsInEffect));
    191190    DEFINE_STATIC_LOCAL(const String, divStyle, ("<div style=\""));
    192191    DEFINE_STATIC_LOCAL(const String, styleSpanOpen, ("<span style=\""));
     
    520519static bool isElementPresentational(const Node* node)
    521520{
    522     if (node->hasTagName(uTag) || node->hasTagName(sTag) || node->hasTagName(strikeTag)
    523         || node->hasTagName(iTag) || node->hasTagName(emTag) || node->hasTagName(bTag) || node->hasTagName(strongTag))
    524         return true;
    525     RefPtr<EditingStyle> style = styleFromMatchedRulesAndInlineDecl(node);
    526     return style && style->style() && !propertyMissingOrEqualToNone(style->style(), CSSPropertyTextDecoration);
    527 }
    528 
    529 static bool shouldIncludeWrapperForFullySelectedRoot(Node* fullySelectedRoot)
    530 {
    531     if (fullySelectedRoot->isElementNode() && static_cast<Element*>(fullySelectedRoot)->hasAttribute(backgroundAttr))
    532         return true;
    533    
    534     RefPtr<EditingStyle> style = styleFromMatchedRulesAndInlineDecl(fullySelectedRoot);
    535     if (!style || !style->style())
    536         return false;
    537 
    538     return style->style()->getPropertyCSSValue(CSSPropertyBackgroundImage) || style->style()->getPropertyCSSValue(CSSPropertyBackgroundColor);
    539 }
    540 
    541 static Node* highestAncestorToWrapMarkup(const Range* range, Node* fullySelectedRoot, EAnnotateForInterchange shouldAnnotate)
     521    return node->hasTagName(uTag) || node->hasTagName(sTag) || node->hasTagName(strikeTag)
     522        || node->hasTagName(iTag) || node->hasTagName(emTag) || node->hasTagName(bTag) || node->hasTagName(strongTag);
     523}
     524
     525static Node* highestAncestorToWrapMarkup(const Range* range, EAnnotateForInterchange shouldAnnotate)
    542526{
    543527    ExceptionCode ec;
     
    573557    if (Node *enclosingAnchor = enclosingNodeWithTag(firstPositionInNode(specialCommonAncestor ? specialCommonAncestor : commonAncestor), aTag))
    574558        specialCommonAncestor = enclosingAnchor;
    575 
    576     if (shouldAnnotate == AnnotateForInterchange && fullySelectedRoot && shouldIncludeWrapperForFullySelectedRoot(fullySelectedRoot))
    577         specialCommonAncestor = fullySelectedRoot;
    578559
    579560    return specialCommonAncestor;
     
    621602    if (body && areRangesEqual(VisibleSelection::selectionFromContentsOfNode(body).toNormalizedRange().get(), range))
    622603        fullySelectedRoot = body;
    623     Node* specialCommonAncestor = highestAncestorToWrapMarkup(updatedRange.get(), fullySelectedRoot, shouldAnnotate);
     604    Node* specialCommonAncestor = highestAncestorToWrapMarkup(updatedRange.get(), shouldAnnotate);
    624605    StyledMarkupAccumulator accumulator(nodes, shouldResolveURLs, shouldAnnotate, updatedRange.get(), specialCommonAncestor);
    625606    Node* pastEnd = updatedRange->pastLastNode();
Note: See TracChangeset for help on using the changeset viewer.