Bug 54816 – style.borderSpacing always returns empty string

• Status: RESOLVED
• Resolution: FIXED
• Component: Tables

[Daniel Bates]

Created attachment 83087 [details]
Example

Consider the following HTML snippet:

<table id="table" style="border-spacing: 4px 5px">
    <tr><td>Cell 1</td></tr>
</table>

Then document.getElementById("table").borderSpacing should be equal to the string "4px 5px". But document.getElementById("table").borderSpacing == "" (as of r79140).

Notice that both Mac Firefox 3.6.13 and IE 8 return the string "4px 5px".

[Daniel Bates]

Created attachment 84366 [details]
Patch and layout test

[WebKit Review Bot]

Attachment 84366 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/css/CSSMutableStyleDeclaration.cpp:124:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

(In reply to comment #2)
> [...]
> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:124:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]

We should fix the style for the whole switch block. I suggest we do this in a separate bug so as to demarcate the actual changes for this bug from the style correction for the switch block.

[Darin Adler]

Comment on attachment 84366 [details]
Patch and layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=84366&action=review

I’d also suggest having the test case cover computed style too, even though that code probably already handles this correctly.

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:267
> +    RefPtr<CSSValue> horizontalValue = getPropertyCSSValue(properties[0]);
> +    RefPtr<CSSValue> verticalValue = getPropertyCSSValue(properties[1]);

It’s probably better to just call getPropertyValue instead of calling getPropertyCSSValue and then separately calling cssText.

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:270
> +    if (!horizontalValue || !verticalValue)
> +        return String();

Is this correct? The test case doesn't cover this.

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:274
> +    if (result != verticalValue->cssText())
> +        result += " " + verticalValue->cssText();

Appending to strings is inefficient. The code in this file is consistently less efficient than it can be. The efficient way to combine two strings into a new string with a character between them is to use the makeString function from the StringConcatenate.h header. So assuming that horizontalValue and verticalValue are strings, not CSSValue objects, the code would be:

    if (horizontalValue == verticalValue)
        return horizontalValue;
    return makeString(horizontalValue, ' ', verticalValue);

The rest of the file could also be fixed so it does less of the extremely-inefficient string concatenation. For building up a string a piece at a time, the best approach is to probably use either StringBuilder or Vector<UChar>.

> Source/WebCore/css/CSSMutableStyleDeclaration.h:157
> +    String borderSpacingValue(const int* properties) const;

I think const int properties[2] would be better and clearer here.

[Daniel Bates]

(In reply to comment #4)
> (From update of attachment 84366 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84366&action=review
> 
> I’d also suggest having the test case cover computed style too, even though that code probably already handles this correctly.

Will add test cases for this.

> 
> > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:267
> > +    RefPtr<CSSValue> horizontalValue = getPropertyCSSValue(properties[0]);
> > +    RefPtr<CSSValue> verticalValue = getPropertyCSSValue(properties[1]);
> 
> It’s probably better to just call getPropertyValue instead of calling getPropertyCSSValue and then separately calling cssText.

I didn't make this change since it would add an additional function call compared to the logic proposed in the patch. Notice CSSMutableStyleDeclaration::getPropertyValue() calls CSSMutableStyleDeclaration::getPropertyCSSValue() and then CSSValue::cssText(). And the patch proposes calling CSSMutableStyleDeclaration::getPropertyCSSValue() directly.

> 
> > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:270
> > +    if (!horizontalValue || !verticalValue)
> > +        return String();
> 
> Is this correct? The test case doesn't cover this.

This is correct and I will include test cases for this. Moreover, we can strengthen this to be:

    if (!horizontalValue)
        return String();
    ASSERT(verticalValue); // By <http://www.w3.org/TR/CSS21/tables.html#separated-borders>.

> 
> > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:274
> > +    if (result != verticalValue->cssText())
> > +        result += " " + verticalValue->cssText();
> 
> Appending to strings is inefficient. The code in this file is consistently less efficient than it can be. The efficient way to combine two strings into a new string with a character between them is to use the makeString function from the StringConcatenate.h header. So assuming that horizontalValue and verticalValue are strings, not CSSValue objects, the code would be:
> 
>     if (horizontalValue == verticalValue)
>         return horizontalValue;
>     return makeString(horizontalValue, ' ', verticalValue);
> 

Will change before landing.

> The rest of the file could also be fixed so it does less of the extremely-inefficient string concatenation. For building up a string a piece at a time, the best approach is to probably use either StringBuilder or Vector<UChar>.

I suggest we look into this as part of a separate bug. I filed bug #55851 for this issue.

> 
> > Source/WebCore/css/CSSMutableStyleDeclaration.h:157
> > +    String borderSpacingValue(const int* properties) const;
> 
> I think const int properties[2] would be better and clearer here.

Will change before landing.

[Daniel Bates]

Committed r80439: <http://trac.webkit.org/changeset/80439>