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...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- minor (vote)
: mozilla8
Assigned To: David Zbarsky (:dzbarsky)
:
Mentors:
Depends on: 667864 668166 668826
Blocks:
  Show dependency treegraph
 
Reported: 2010-08-12 14:19 PDT by :Aryeh Gregor (away until August 15)
Modified: 2011-07-11 07:32 PDT (History)
5 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
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 (away until August 15) 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>
<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 Boris Zbarsky [:bz] 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] 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]
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
Comment 5 David Zbarsky (:dzbarsky) 2011-06-25 10:50:19 PDT
Created attachment 541950 [details] [diff] [review]
Patch
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] 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] 2011-06-27 20:51:22 PDT
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.
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]
Patch
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)
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 Boris Zbarsky [:bz] 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]
Patch

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]
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.
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]
Patch
Comment 18 Mounir Lamouri (:mounir) 2011-07-06 13:25:37 PDT
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.
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
Pushed:
http://hg.mozilla.org/mozilla-central/rev/06533f841990

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