Bug 102112 - When we do setAttribute("border", null) on a table we should create a border like every other browser
Summary: When we do setAttribute("border", null) on a table we should create a border ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Robert Hogan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-13 11:47 PST by Robert Hogan
Modified: 2014-04-16 17:27 PDT (History)
10 users (show)

See Also:


Attachments
Reduction (467 bytes, text/html)
2012-11-13 11:51 PST, Robert Hogan
no flags Details
Patch (6.28 KB, patch)
2012-12-26 02:45 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (11.52 KB, patch)
2013-01-17 11:49 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (11.54 KB, patch)
2013-01-17 14:18 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (11.09 KB, patch)
2013-01-21 12:11 PST, Robert Hogan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Hogan 2012-11-13 11:47:13 PST
When we do setAttribute("border", null), the Attribute object interprets null as the string "null"
Comment 1 Robert Hogan 2012-11-13 11:51:29 PST
Created attachment 173938 [details]
Reduction
Comment 2 Glenn Adams 2012-11-13 21:09:10 PST
this is correct behavior per DOM4 [1] and WebIDL [2] specs;

[1] http://www.w3.org/TR/domcore/#element 
[2] http://www.w3.org/TR/WebIDL/#TreatNullAs

if setAttribute() were redefined as:

void setAttribute(DOMString name, [TreatNullAs=EmptyString] DOMString value);

then the behavior you observe would be a bug, but at present it is not;

it is also not related to CSS

you need to take this up with the W3C (and make sure other browser behave as you suggest) before reopening this; in the mean time, i'm marking as WORKSFORME
Comment 3 Robert Hogan 2012-11-14 11:05:29 PST
(In reply to comment #2)
> this is correct behavior per DOM4 [1] and WebIDL [2] specs;
> 
> [1] http://www.w3.org/TR/domcore/#element 
> [2] http://www.w3.org/TR/WebIDL/#TreatNullAs
> 
> if setAttribute() were redefined as:
> 

So per the spec we're correct to stringify null when setting an attribute. But we're the only browser that regards setAttribute("border", null) as not setting a border so we should fix that here I guess.
Comment 4 Robert Hogan 2012-12-26 02:45:08 PST
Created attachment 180736 [details]
Patch
Comment 5 Ian 'Hixie' Hickson 2013-01-15 12:29:51 PST
border="null" is equivalent to border="1" according to the HTML standard; see near the end of:

   http://whatwg.org/html/#tables

Note that the current (most up to date) URL for the DOM4 spec is: http://dom.spec.whatwg.org/
Comment 6 Robert Hogan 2013-01-17 11:49:51 PST
Created attachment 183234 [details]
Patch
Comment 7 Build Bot 2013-01-17 12:29:48 PST
Comment on attachment 183234 [details]
Patch

Attachment 183234 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/15942137
Comment 8 Build Bot 2013-01-17 13:06:24 PST
Comment on attachment 183234 [details]
Patch

Attachment 183234 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15914982
Comment 9 Robert Hogan 2013-01-17 14:18:45 PST
Created attachment 183276 [details]
Patch
Comment 10 Ryosuke Niwa 2013-01-17 16:21:22 PST
Comment on attachment 183276 [details]
Patch

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

> Source/WebCore/html/HTMLElement.cpp:140
> +    bool isOK = false;

I don't think isOK is a descriptive name.
How about parsedCorrectly?

> Source/WebCore/html/HTMLElement.cpp:143
> +    return (!isOK && hasLocalName(tableTag)) ? 1 : borderWidth;

No need for parenthesis around the condition.

Personally, I would have done:
if (value.isEmpty() || !parseHTMLNonNegativeInteger(value, borderWidth))
    return 1;
return borderWidth.
instead.

> LayoutTests/fast/dom/HTMLTableElement/table-with-invalid-border.html:1
> +<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd">

Why are we using HTML4.01 DOCTYPE as opposed to HTML5 DOCTYPE?

> LayoutTests/fast/dom/HTMLTableElement/table-with-invalid-border.html:14
> +    <table id="table" border="djska" cellpadding="5" cellspacing="0" width="100%" height="100%">
> +        <tr>
> +            <td id="cell3" height="26"></td>

Why do we need to have ids and cellpadding, etc…?

> LayoutTests/fast/dom/HTMLTableElement/table-with-invalid-border.html:20
> +    shouldBeEqualToString('document.defaultView.getComputedStyle(table, null).getPropertyValue("border-top-width")', "1px");

Please explicitly get the table via document.querySelector('table') for example.
Named properties on window object is a WebKit extension.
Comment 11 Robert Hogan 2013-01-21 12:11:55 PST
Created attachment 183817 [details]
Patch
Comment 12 Ryosuke Niwa 2013-01-21 12:20:43 PST
Comment on attachment 183817 [details]
Patch

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

> Source/WebCore/html/HTMLTableElement.cpp:377
>          // FIXME: This attribute is a mess.

Maybe remove this comment?
Comment 13 WebKit Review Bot 2013-01-22 10:23:30 PST
Comment on attachment 183817 [details]
Patch

Clearing flags on attachment: 183817

Committed r140436: <http://trac.webkit.org/changeset/140436>
Comment 14 WebKit Review Bot 2013-01-22 10:23:34 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Daniel Bates 2014-04-16 17:27:48 PDT
*** Bug 56982 has been marked as a duplicate of this bug. ***