Many legacy reflected attributes have nonstandard and probably unnecessary behavior (align, ch, vAlign, table.width, table.border)

RESOLVED FIXED in mozilla8

Status

()

Core
DOM
--
minor
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: ayg, Assigned: dzbarsky)

Tracking

Trunk
mozilla8
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

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.

Updated

7 years ago
OS: Linux → All
Hardware: x86 → All
(Assignee)

Updated

7 years ago
Assignee: nobody → dzbarsky
(Assignee)

Comment 2

7 years ago
Created attachment 465877 [details] [diff] [review]
Patch v1
(Assignee)

Updated

7 years ago
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-
(Assignee)

Comment 4

6 years ago
Created attachment 541949 [details] [diff] [review]
Patch

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

6 years ago
Created attachment 541950 [details] [diff] [review]
Patch
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
(Assignee)

Comment 10

6 years ago
Created attachment 543357 [details] [diff] [review]
Patch
Attachment #541950 - Attachment is obsolete: true
(Assignee)

Comment 11

6 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.
You have to teach it that anyway, yeah.  See .defaultValue, which reflects the "value" content attribute in many cases.
(Assignee)

Comment 13

6 years ago
Created attachment 543441 [details] [diff] [review]
Patch

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.
(Assignee)

Comment 17

6 years ago
Created attachment 544292 [details] [diff] [review]
Patch
Attachment #543441 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
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+
(Assignee)

Comment 19

6 years ago
Created attachment 544469 [details] [diff] [review]
Patch to check in
Attachment #544292 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.