rowspan="02" does not round-trip cleanly, gets corrupted to rowspan="2"

VERIFIED FIXED in Firefox 39

Status

()

defect
VERIFIED FIXED
4 years ago
4 months ago

People

(Reporter: roan.kattouw, Assigned: bzbarsky)

Tracking

(Depends on 1 bug, Blocks 1 bug, {testcase})

36 Branch
mozilla39
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 verified)

Details

(Whiteboard: [bugday-20150610], )

Attachments

(1 attachment)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20150306140254

Steps to reproduce:

In a console, run:

div = document.createElement('div');
div.innerHTML = '<table><tr><td rowspan="02"></td></tr></table>';
div.innerHTML;


Actual results:

"<table><tbody><tr><td rowspan="2"></td></tr></tbody></table>"


Expected results:

"<table><tbody><tr><td rowspan="02"></td></tr></tbody></table>"
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM
Ever confirmed: true
Keywords: testcase
Product: Firefox → Core
Note that this isn't a serialization issue, the attribute corruption occurs on parse:

> div.querySelector('td').getAttribute('rowspan')
"2"

This is even worse because it means there's no way to find out what the original value of the attribute was.

Chrome does not have this bug. IE does have this bug, and it also performs many other normalizations, like width="230px"->width="230", color="Red"->color="red", bgcolor="#FFDEAD"->bgcolor="#ffdead", style="color:#ffd"->style="color: rgb(255, 255, 221);" and even style="invalid-css"->style="". Firefox fortunately doesn't perform any of these as far as I can tell.
Right, the problem is that nsContentUtils::ParseHTMLInteger doesn't detect this as a case in which serialization would produce a value different from the input.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment on attachment 8576687 [details] [diff] [review]
Fix integer attribute parsing to not lose track of leading zeroes

Review of attachment 8576687 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsContentUtils.cpp
@@ +989,5 @@
> +  CheckedInt32 value = 0;
> +
> +  // Check for leading zeros first.
> +  bool hadLeadingZero = false;
> +  bool hadMultipleLeadingZeros = false;

Might be easier to simply count the leading zeros, rather than deal with two state variables.
Attachment #8576687 - Flags: review?(jonas) → review+
> Might be easier to simply count the leading zeros

I considered that, but then the paranoid in me didn't want to deal with that count maybe overflowing.  I guess I could have used a 64-bit counter...  I'll do that.
https://hg.mozilla.org/integration/mozilla-inbound/rev/83ac7b782760 and then https://hg.mozilla.org/integration/mozilla-inbound/rev/35690b2e6d73 to fix issues with getting the value of an invalid CheckedInt and an existing bogus test that required the wrong behavior.
And https://hg.mozilla.org/integration/mozilla-inbound/rev/dd04d7c32b67 for yet another missed wpt annotation.  I wish it were less painful to update those annotation files.  :(
QA Whiteboard: [good first verify]
Successfully reproduce this bug on Firefox 38.0.5 Build ID 20150525141253, User Agent: Mozilla/5.0 (Windows NT 6.1; rv:38.0) Gecko/20100101 Firefox/38.0

The fix works for me on Firefox 39.0, Build ID: 	20150604162752, User Agent: Mozilla/5.0 (Windows NT 6.1; rv:39.0) Gecko/20100101 Firefox/39.0
Status: RESOLVED → VERIFIED
QA Whiteboard: [good first verify] → [good first verify][bugday-20150610]
Whiteboard: [bugday-20150610]
Depends on: 1445898
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.