Closed Bug 586786 Opened 15 years ago Closed 14 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)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
minor

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla8

People

(Reporter: ayg, Assigned: dzbarsky)

References

Details

Attachments

(1 file, 6 obsolete files)

Patch to check in
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.
The fact that getAttribute() is affected is definitely a bug.
OS: Linux → All
Hardware: x86 → All
Assignee: nobody → dzbarsky
Attached patch Patch v1 (obsolete) — — Splinter Review
Attachment #465877 - Flags: review?(bzbarsky)
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-
Attached patch Patch (obsolete) — — Splinter Review
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)
Attached patch Patch (obsolete) — — Splinter Review
Attachment #541949 - Attachment is obsolete: true
Attachment #541950 - Flags: review?(bzbarsky)
Attachment #541949 - Flags: review?(bzbarsky)
David, could you add a method in content/html/content/tests/reflect.js for string reflection and use it to test this bug?
> if it isn't an integer it is set by default to 1 in MapAttributesIntoRules. Aha, there we go!
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+
David, when the bugs in the DEPENDS field will be fixed, you will be able to test those attribute reflections with one function call.
Depends on: 667856, 667864
Depends on: 668166
No longer depends on: 667856
Attached patch Patch (obsolete) — — Splinter Review
Attachment #541950 - Attachment is obsolete: true
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.
You have to teach it that anyway, yeah. See .defaultValue, which reflects the "value" content attribute in many cases.
Attached patch Patch (obsolete) — — Splinter Review
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 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-
Depends on: 668826
FWIW, I proposed a solution in bug 668826. I just need a green light to write the patch.
David, you should now be able to use reflectString() to test this.
Attached patch Patch (obsolete) — — Splinter Review
Attachment #543441 - Attachment is obsolete: true
Attachment #544292 - Flags: review?(mounir)
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+
Attachment #544292 - Attachment is obsolete: true
Keywords: checkin-needed
Status: NEW → ASSIGNED
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [inbound]
Pushed: http://hg.mozilla.org/mozilla-central/rev/06533f841990
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: