Bug 102112

Summary: When we do setAttribute("border", null) on a table we should create a border like every other browser
Product: WebKit Reporter: Robert Hogan <robert>
Component: Layout and RenderingAssignee: Robert Hogan <robert>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, glenn, ian, jchaffraix, Ms2ger, ojan.autocc, rniwa, robert, webkit.review.bot, xn--mlform-iua
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Reduction
none
Patch
none
Patch
none
Patch
none
Patch none

Robert Hogan
Reported 2012-11-13 11:47:13 PST
When we do setAttribute("border", null), the Attribute object interprets null as the string "null"
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
DetailsFormatted DiffDiff
Patch (11.52 KB, patch)
2013-01-17 11:49 PST, Robert Hogan 		no flags
DetailsFormatted DiffDiff
Patch (11.54 KB, patch)
2013-01-17 14:18 PST, Robert Hogan 		no flags
DetailsFormatted DiffDiff
Patch (11.09 KB, patch)
2013-01-21 12:11 PST, Robert Hogan 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Robert Hogan
Comment 1 2012-11-13 11:51:29 PST
Created attachment 173938 [details] Reduction
Glenn Adams
Comment 2 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
Robert Hogan
Comment 3 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.
Robert Hogan
Comment 4 2012-12-26 02:45:08 PST
Created attachment 180736 [details] Patch
Ian 'Hixie' Hickson
Comment 5 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/
Robert Hogan
Comment 6 2013-01-17 11:49:51 PST
Created attachment 183234 [details] Patch
Build Bot
Comment 7 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
Build Bot
Comment 8 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
Robert Hogan
Comment 9 2013-01-17 14:18:45 PST
Created attachment 183276 [details] Patch
Ryosuke Niwa
Comment 10 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.
Robert Hogan
Comment 11 2013-01-21 12:11:55 PST
Created attachment 183817 [details] Patch
Ryosuke Niwa
Comment 12 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?
WebKit Review Bot
Comment 13 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>
WebKit Review Bot
Comment 14 2013-01-22 10:23:34 PST
All reviewed patches have been landed. Closing bug.
Daniel Bates
Comment 15 2014-04-16 17:27:48 PDT
*** Bug 56982 has been marked as a duplicate of this bug. ***
Note You need to log in before you can comment on or make changes to this bug.