Closed
Bug 586786
Opened 14 years ago
Closed 13 years ago
Many legacy reflected attributes have nonstandard and probably unnecessary behavior (align, ch, vAlign, table.width, table.border)
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla8
People
(Reporter: ayg, Assigned: dzbarsky)
References
Details
Attachments
(1 file, 6 obsolete files)
|
13.94 KB,
patch
|
Details | Diff | Splinter Review |
HTML5 defines all of the following reflected attributes as regular strings, but Firefox exhibits various odd behaviors when getting and setting:
* {col,tbody,tfoot,thead,tr}.{align,ch,vAlign}
* {td,th}.{ch,vAlign}
* table.{border,width}
Specifically, accessing the reflected attribute align, ch, or vAlign when the corresponding content attribute is not set should give an empty string, but actually gives "left", ".", and "middle" respectively. table.width and table.border are even weirder, with setAttribute() followed by getAttribute() not even returning the same thing, plus various other oddities. The following test case is illustrative, although it doesn't cover every quirk:
<!doctype html>
<script>
var col = document.createElement("col");
var table = document.createElement("table");
table.setAttribute("width", "abc");
table.setAttribute("border", "abc");
alert(
"align: " + col.align + "\n" +
"ch: " + col.ch + "\n" +
"vAlign: " + col.vAlign + "\n" +
"width: " + table.width + "\n" +
"border: " + table.border
);
</script>
Chrome dev, Safari 5, and Opera 10.60 alert
align:
ch:
vAlign:
width: abc
border: abc
which is per spec.
Firefox 4.0b3 alerts
align: left
ch: .
vAlign: middle
width:
border: 1
IE8 and IE9PP4 alert
align:
ch:
vAlign:
width:
border: 1
So on the align/ch/vAlign behavior, Firefox is the only one doing weird things, as far as I can tell. IE agrees on the weird treatment of width and border, but WebKit doesn't, so it's probably not a big compat risk. If width and border need to stay as they are, the behavior needs to be specced.
|
||
Comment 1•14 years ago
|
||
The fact that getAttribute() is affected is definitely a bug.
|
||
Updated•14 years ago
|
OS: Linux → All
Hardware: x86 → All
|
Assignee | |
Updated•14 years ago
|
Assignee: nobody → dzbarsky
|
|
Assignee | |
Comment 2•14 years ago
|
||
|
|
Assignee | |
Updated•14 years ago
|
Attachment #465877 -
Flags: review?(bzbarsky)
|
|
||
Comment 3•14 years ago
|
||
Comment on attachment 465877 [details] [diff] [review] Patch v1 >+++ b/content/html/content/reftests/586786-1.html Can you add a second test for getAttribute too? >+++ b/content/html/content/src/nsHTMLTableElement.cpp >@@ -980,42 +980,33 @@ nsHTMLTableElement::ParseAttribute(PRInt >- if (aAttribute == nsGkAtoms::border) { >- if (!aResult.ParseIntWithBounds(aValue, 0)) { >- // XXX this should really be NavQuirks only to allow non numeric value >- aResult.SetTo(1); >- } >- >- return PR_TRUE; >- } This seems wrong, and I'm surprised it passed tests. Isn't this attr used as an integer in MapAttributesIntoRule? Did you want to just make it possible to pass a non-null string to SetTo, so we could parse as 1, but keep returning the string from getAttribute? >+ return !((type == nsAttrValue::eInteger && >+ aResult.GetIntegerValue() == 0) || >+ (type == nsAttrValue::ePercent && >+ aResult.GetPercentValue() == 0.0f)); Please fix the indentation of the aResult lines here.
Attachment #465877 -
Flags: review?(bzbarsky) → review-
|
|
Assignee | |
Comment 4•13 years ago
|
||
The border should be fine, if it isn't an integer it is set by default to 1 in MapAttributesIntoRules. See http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLTableElement.cpp?force=1#1225
Attachment #465877 -
Attachment is obsolete: true
Attachment #541949 -
Flags: review?(bzbarsky)
|
|
Assignee | |
Comment 5•13 years ago
|
||
Attachment #541949 -
Attachment is obsolete: true
Attachment #541950 -
Flags: review?(bzbarsky)
Attachment #541949 -
Flags: review?(bzbarsky)
|
||
Comment 6•13 years ago
|
||
David, could you add a method in content/html/content/tests/reflect.js for string reflection and use it to test this bug?
|
|
||
Comment 7•13 years ago
|
||
> if it isn't an integer it is set by default to 1 in MapAttributesIntoRules.
Aha, there we go!|
|
||
Comment 8•13 years ago
|
||
Comment on attachment 541950 [details] [diff] [review] Patch >- if (aAttribute == nsGkAtoms::cols) { >+ if (aAttribute == nsGkAtoms::cols || >+ aAttribute == nsGkAtoms::border) { Fix the indent. r=me with that and Mounir's issue addressed.
Attachment #541950 -
Flags: review?(bzbarsky) → review+
|
|
||
Comment 9•13 years ago
|
||
David, when the bugs in the DEPENDS field will be fixed, you will be able to test those attribute reflections with one function call.
|
|
Assignee | |
Comment 10•13 years ago
|
||
Attachment #541950 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 11•13 years ago
|
||
So with the previous patch, it was reflecting incorrectly for some reason. However, changing from NS_IMPL_STRING_ATTR(nsHTMLTableCellElement, Ch, _char) to NS_IMPL_STRING_ATTR(nsHTMLTableCellElement, Ch, ch) made it reflect correctly. Even though ch is widely used as the content attribute, HTML5 says "the ch IDL attribute of the col element must reflect the element's char content attribute". So it looks like we should teach reflect.js that the idl and content attribute names don't always match up.
|
|
||
Comment 12•13 years ago
|
||
You have to teach it that anyway, yeah. See .defaultValue, which reflects the "value" content attribute in many cases.
|
|
Assignee | |
Comment 13•13 years ago
|
||
I don't really like the name, but I couldn't think of anything better.
Attachment #543357 -
Attachment is obsolete: true
Attachment #543441 -
Flags: review?(mounir)
|
|
||
Comment 14•13 years ago
|
||
Comment on attachment 543441 [details] [diff] [review] Patch Review of attachment 543441 [details] [diff] [review]: ----------------------------------------------------------------- I don't think we should create another method for that, we should just be able to get the IDL and the content attributes names. I wrote a few patches in a train today that should make this easier. I will try to attach them to some bugs ASAP.
Attachment #543441 -
Flags: review?(mounir) → review-
|
|
||
Comment 15•13 years ago
|
||
FWIW, I proposed a solution in bug 668826. I just need a green light to write the patch.
|
|
||
Comment 16•13 years ago
|
||
David, you should now be able to use reflectString() to test this.
|
|
Assignee | |
Comment 17•13 years ago
|
||
Attachment #543441 -
Attachment is obsolete: true
|
|
Assignee | |
Updated•13 years ago
|
Attachment #544292 -
Flags: review?(mounir)
|
|
||
Comment 18•13 years ago
|
||
Comment on attachment 544292 [details] [diff] [review] Patch Review of attachment 544292 [details] [diff] [review]: ----------------------------------------------------------------- You are doing checks for col, colgroup, ... for three attributes. That would be better to put those checks in a for statement.
Attachment #544292 -
Flags: review?(mounir) → review+
|
|
Assignee | |
Comment 19•13 years ago
|
||
Attachment #544292 -
Attachment is obsolete: true
|
|
Assignee | |
Updated•13 years ago
|
Keywords: checkin-needed
|
|
||
Updated•13 years ago
|
|
|
||
Comment 20•13 years ago
|
||
Pushed: http://hg.mozilla.org/mozilla-central/rev/06533f841990
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
|
||
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•