Last Comment Bug 586786 - Many legacy reflected attributes have nonstandard and probably unnecessary behavior (align, ch, vAlign, table.width, table.border)
: Many legacy reflected attributes have nonstandard and probably unnecessary be...
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- minor (vote)
: mozilla8
Assigned To: David Zbarsky (:dzbarsky)
: Andrew Overholt [:overholt]
Depends on: 667864 668166 668826
  Show dependency treegraph
Reported: 2010-08-12 14:19 PDT by Aryeh Gregor (:ayg) (next working March 28-April 26)
Modified: 2011-07-11 07:32 PDT (History)
5 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch v1 (11.15 KB, patch)
2010-08-13 16:00 PDT, David Zbarsky (:dzbarsky)
bzbarsky: review-
Details | Diff | Splinter Review
Patch (14.43 KB, patch)
2011-06-25 10:48 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review
Patch (14.47 KB, patch)
2011-06-25 10:50 PDT, David Zbarsky (:dzbarsky)
bzbarsky: review+
Details | Diff | Splinter Review
Patch (15.66 KB, patch)
2011-06-30 23:03 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review
Patch (21.18 KB, patch)
2011-07-01 08:15 PDT, David Zbarsky (:dzbarsky)
mounir: review-
Details | Diff | Splinter Review
Patch (16.70 KB, patch)
2011-07-06 10:58 PDT, David Zbarsky (:dzbarsky)
mounir: review+
Details | Diff | Splinter Review
Patch to check in (13.94 KB, patch)
2011-07-07 07:09 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review

Description Aryeh Gregor (:ayg) (next working March 28-April 26) 2010-08-12 14:19:27 PDT
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>
    var col = document.createElement("col");
    var table = document.createElement("table");
    table.setAttribute("width", "abc");
    table.setAttribute("border", "abc");
        "align: " + col.align + "\n" +
        "ch: " + + "\n" +
        "vAlign: " + col.vAlign + "\n" +
        "width: " + table.width + "\n" +
        "border: " + table.border

Chrome dev, Safari 5, and Opera 10.60 alert

    width: abc
    border: abc

which is per spec.

Firefox 4.0b3 alerts

    align: left
    ch: .
    vAlign: middle
    border: 1

IE8 and IE9PP4 alert

    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 Boris Zbarsky [:bz] (still a bit busy) 2010-08-12 14:36:39 PDT
The fact that getAttribute() is affected is definitely a bug.
Comment 2 David Zbarsky (:dzbarsky) 2010-08-13 16:00:04 PDT
Created attachment 465877 [details] [diff] [review]
Patch v1
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2010-09-02 12:40:57 PDT
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.
Comment 4 David Zbarsky (:dzbarsky) 2011-06-25 10:48:48 PDT
Created attachment 541949 [details] [diff] [review]

The border should be fine, if it isn't an integer it is set by default to 1 in MapAttributesIntoRules.
Comment 5 David Zbarsky (:dzbarsky) 2011-06-25 10:50:19 PDT
Created attachment 541950 [details] [diff] [review]
Comment 6 Mounir Lamouri (:mounir) 2011-06-25 13:33:55 PDT
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 Boris Zbarsky [:bz] (still a bit busy) 2011-06-27 20:47:03 PDT
> if it isn't an integer it is set by default to 1 in MapAttributesIntoRules.

Aha, there we go!
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2011-06-27 20:51:22 PDT
Comment on attachment 541950 [details] [diff] [review]

>-    if (aAttribute == nsGkAtoms::cols) {
>+	if (aAttribute == nsGkAtoms::cols ||
>+		aAttribute == nsGkAtoms::border) {

Fix the indent.

r=me with that and Mounir's issue addressed.
Comment 9 Mounir Lamouri (:mounir) 2011-06-28 04:11:06 PDT
David, when the bugs in the DEPENDS field will be fixed, you will be able to test those attribute reflections with one function call.
Comment 10 David Zbarsky (:dzbarsky) 2011-06-30 23:03:33 PDT
Created attachment 543357 [details] [diff] [review]
Comment 11 David Zbarsky (:dzbarsky) 2011-06-30 23:08:17 PDT
So with the previous patch, it was reflecting incorrectly for some reason. However, changing from
NS_IMPL_STRING_ATTR(nsHTMLTableCellElement, Ch, _char)
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 Boris Zbarsky [:bz] (still a bit busy) 2011-06-30 23:10:42 PDT
You have to teach it that anyway, yeah.  See .defaultValue, which reflects the "value" content attribute in many cases.
Comment 13 David Zbarsky (:dzbarsky) 2011-07-01 08:15:01 PDT
Created attachment 543441 [details] [diff] [review]

I don't really like the name, but I couldn't think of anything better.
Comment 14 Mounir Lamouri (:mounir) 2011-07-01 08:25:43 PDT
Comment on attachment 543441 [details] [diff] [review]

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.
Comment 15 Mounir Lamouri (:mounir) 2011-07-01 08:58:46 PDT
FWIW, I proposed a solution in bug 668826. I just need a green light to write the patch.
Comment 16 Mounir Lamouri (:mounir) 2011-07-06 06:12:08 PDT
David, you should now be able to use reflectString() to test this.
Comment 17 David Zbarsky (:dzbarsky) 2011-07-06 10:58:24 PDT
Created attachment 544292 [details] [diff] [review]
Comment 18 Mounir Lamouri (:mounir) 2011-07-06 13:25:37 PDT
Comment on attachment 544292 [details] [diff] [review]

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.
Comment 19 David Zbarsky (:dzbarsky) 2011-07-07 07:09:21 PDT
Created attachment 544469 [details] [diff] [review]
Patch to check in
Comment 20 Mounir Lamouri (:mounir) 2011-07-11 07:32:11 PDT

Note You need to log in before you can comment on or make changes to this bug.