From 2ccccf1e489005d0b2d14612d4c600f4d81173bf Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Wed, 3 Jul 2019 06:36:17 +0000 Subject: [PATCH] Bug 1562690. Fix parsing of HTML dimension values to follow spec update and align better with other browsers. r=mccr8 See https://github.com/whatwg/html/pull/4747 for the spec changes Differential Revision: https://phabricator.services.mozilla.com/D36643 --HG-- extra : moz-landing-system : lando --- dom/base/nsAttrValue.cpp | 48 ++--- dom/html/nsGenericHTMLElement.cpp | 16 +- .../html/dom/reflection-embedded.html.ini | 30 +++ .../meta/html/dom/reflection-forms.html.ini | 24 +++ .../html/dom/reflection-metadata.html.ini | 24 +++ .../meta/html/dom/reflection-misc.html.ini | 36 ++++ .../html/dom/reflection-obsolete.html.ini | 120 +++++++++++ .../rendering/dimension-attributes.html.ini | 188 ------------------ .../web-platform/tests/html/dom/reflection.js | 24 +-- .../html/rendering/dimension-attributes.html | 29 +-- .../the-hr-element-0/width-ref.html | 4 +- 11 files changed, 299 insertions(+), 244 deletions(-) diff --git a/dom/base/nsAttrValue.cpp b/dom/base/nsAttrValue.cpp index 1cf1a921fe8e..8a2808746f88 100644 --- a/dom/base/nsAttrValue.cpp +++ b/dom/base/nsAttrValue.cpp @@ -1272,7 +1272,7 @@ bool nsAttrValue::DoParseHTMLDimension(const nsAString& aInput, // https://html.spec.whatwg.org/multipage/#rules-for-parsing-dimension-values - // Step 1 and 2. + // Steps 1 and 2. const char16_t* position = aInput.BeginReading(); const char16_t* end = aInput.EndReading(); @@ -1281,36 +1281,19 @@ bool nsAttrValue::DoParseHTMLDimension(const nsAString& aInput, // leading '0' characters, or trailing garbage. bool canonical = true; - // Step 3 + // Step 3. while (position != end && nsContentUtils::IsHTMLWhitespace(*position)) { canonical = false; // Leading whitespace ++position; } - // Step 4 - if (position == end) { + // Step 4. + if (position == end || *position < char16_t('0') || + *position > char16_t('9')) { return false; } - // Step 5 - if (*position == char16_t('+')) { - canonical = false; // Leading '+' - ++position; - - // Step 6. The spec has this happening regardless of whether we found '+', - // but there's no point repeating the step 4 test if we didn't advance - // position. - if (position == end) { - return false; - } - } - - // Step 7. - if (*position < char16_t('0') || *position > char16_t('9')) { - return false; - } - - // Step 8. + // Step 5. CheckedInt32 value = 0; // Collect up leading '0' first to avoid extra branching in the main @@ -1333,28 +1316,35 @@ bool nsAttrValue::DoParseHTMLDimension(const nsAString& aInput, ++position; } - // Step 9 is implemented implicitly via the various "position != end" guards + // Step 6 is implemented implicitly via the various "position != end" guards // from this point on. Maybe doubleValue; - // Step 10. + // Step 7. The return in step 7.2 is handled by just falling through to the + // code below this block when we reach end of input or a non-digit, because + // the while loop will terminate at that point. if (position != end && *position == char16_t('.')) { canonical = false; // Let's not rely on double serialization reproducing // the string we started with. + // Step 7.1. ++position; // If we have a '.' _not_ followed by digits, this is not as efficient as it // could be, because we will store as a double while we could have stored as // an int. But that seems like a pretty rare case. doubleValue.emplace(value.value()); + // Step 7.3. double divisor = 1.0f; - // Per spec we should now return a number if there is no next char or if the - // next char is not a digit, but no one does that. See - // https://github.com/whatwg/html/issues/4736 + // Step 7.4. while (position != end && *position >= char16_t('0') && *position <= char16_t('9')) { + // Step 7.4.1. divisor = divisor * 10.0f; + // Step 7.4.2. doubleValue.ref() += (*position - char16_t('0')) / divisor; + // Step 7.4.3. ++position; + // Step 7.4.4 and 7.4.5 are captured in the while loop condition and the + // "position != end" checks below. } } @@ -1364,7 +1354,7 @@ bool nsAttrValue::DoParseHTMLDimension(const nsAString& aInput, return false; } - // Steps 11-13. + // Step 8 and the spec's early return from step 7.2. ValueType type; if (position != end && *position == char16_t('%')) { type = ePercent; diff --git a/dom/html/nsGenericHTMLElement.cpp b/dom/html/nsGenericHTMLElement.cpp index 979e2fbde1a2..17b73d61e569 100644 --- a/dom/html/nsGenericHTMLElement.cpp +++ b/dom/html/nsGenericHTMLElement.cpp @@ -1418,7 +1418,21 @@ uint32_t nsGenericHTMLElement::GetDimensionAttrAsUnsignedInt( return uint32_t(attrVal->GetDoubleValue()); } - return aDefault; + // Unfortunately, the set of values that are valid dimensions is not a + // superset of values that are valid unsigned ints. In particular "+100" is + // not a valid dimension, but should parse as the unsigned int "100". So if + // we got here and we don't have a valid dimension value, just try re-parsing + // the string we have as an integer. + nsAutoString val; + attrVal->ToString(val); + nsContentUtils::ParseHTMLIntegerResultFlags result; + int32_t parsedInt = nsContentUtils::ParseHTMLInteger(val, &result); + if ((result & nsContentUtils::eParseHTMLInteger_Error) || + parsedInt < 0) { + return aDefault; + } + + return parsedInt; } void nsGenericHTMLElement::GetURIAttr(nsAtom* aAttr, nsAtom* aBaseAttr, diff --git a/testing/web-platform/meta/html/dom/reflection-embedded.html.ini b/testing/web-platform/meta/html/dom/reflection-embedded.html.ini index 079f7ce5aca1..ca6578e72895 100644 --- a/testing/web-platform/meta/html/dom/reflection-embedded.html.ini +++ b/testing/web-platform/meta/html/dom/reflection-embedded.html.ini @@ -905,6 +905,12 @@ [iframe.allowUserMedia: setAttribute() to "5%"] expected: FAIL + [iframe.allowUserMedia: setAttribute() to "+100"] + expected: FAIL + + [iframe.allowUserMedia: setAttribute() to ".5"] + expected: FAIL + [iframe.allowUserMedia: setAttribute() to true] expected: FAIL @@ -956,6 +962,12 @@ [iframe.allowUserMedia: IDL set to "5%"] expected: FAIL + [iframe.allowUserMedia: IDL set to "+100"] + expected: FAIL + + [iframe.allowUserMedia: IDL set to ".5"] + expected: FAIL + [iframe.allowUserMedia: IDL set to false] expected: FAIL @@ -1013,6 +1025,12 @@ [iframe.delegateStickyUserActivation: IDL set to "5%"] expected: FAIL + [iframe.delegateStickyUserActivation: IDL set to "+100"] + expected: FAIL + + [iframe.delegateStickyUserActivation: IDL set to ".5"] + expected: FAIL + [iframe.delegateStickyUserActivation: IDL set to true] expected: FAIL @@ -1079,6 +1097,12 @@ [video.playsInline: IDL set to "5%"] expected: FAIL + [video.playsInline: IDL set to "+100"] + expected: FAIL + + [video.playsInline: IDL set to ".5"] + expected: FAIL + [video.playsInline: setAttribute() to object "test-valueOf"] expected: FAIL @@ -1112,6 +1136,12 @@ [video.playsInline: setAttribute() to "5%"] expected: FAIL + [video.playsInline: setAttribute() to "+100"] + expected: FAIL + + [video.playsInline: setAttribute() to ".5"] + expected: FAIL + [video.playsInline: setAttribute() to object "[object Object\]"] expected: FAIL diff --git a/testing/web-platform/meta/html/dom/reflection-forms.html.ini b/testing/web-platform/meta/html/dom/reflection-forms.html.ini index a7d5628d8a7f..0aa03eb4093a 100644 --- a/testing/web-platform/meta/html/dom/reflection-forms.html.ini +++ b/testing/web-platform/meta/html/dom/reflection-forms.html.ini @@ -1966,6 +1966,12 @@ [input.dirName: setAttribute() to "5%"] expected: FAIL + [input.dirName: setAttribute() to "+100"] + expected: FAIL + + [input.dirName: setAttribute() to ".5"] + expected: FAIL + [input.dirName: setAttribute() to true] expected: FAIL @@ -2014,6 +2020,12 @@ [input.dirName: IDL set to "5%"] expected: FAIL + [input.dirName: IDL set to "+100"] + expected: FAIL + + [input.dirName: IDL set to ".5"] + expected: FAIL + [input.dirName: IDL set to true] expected: FAIL @@ -2494,6 +2506,12 @@ [textarea.dirName: setAttribute() to "5%"] expected: FAIL + [textarea.dirName: setAttribute() to "+100"] + expected: FAIL + + [textarea.dirName: setAttribute() to ".5"] + expected: FAIL + [textarea.dirName: setAttribute() to true] expected: FAIL @@ -2542,6 +2560,12 @@ [textarea.dirName: IDL set to "5%"] expected: FAIL + [textarea.dirName: IDL set to "+100"] + expected: FAIL + + [textarea.dirName: IDL set to ".5"] + expected: FAIL + [textarea.dirName: IDL set to true] expected: FAIL diff --git a/testing/web-platform/meta/html/dom/reflection-metadata.html.ini b/testing/web-platform/meta/html/dom/reflection-metadata.html.ini index 3d877f700e14..a368ea145400 100644 --- a/testing/web-platform/meta/html/dom/reflection-metadata.html.ini +++ b/testing/web-platform/meta/html/dom/reflection-metadata.html.ini @@ -59,6 +59,12 @@ [link.nonce: setAttribute() to "5%"] expected: FAIL + [link.nonce: setAttribute() to "+100"] + expected: FAIL + + [link.nonce: setAttribute() to ".5"] + expected: FAIL + [link.nonce: setAttribute() to true] expected: FAIL @@ -107,6 +113,12 @@ [link.nonce: IDL set to "5%"] expected: FAIL + [link.nonce: IDL set to "+100"] + expected: FAIL + + [link.nonce: IDL set to ".5"] + expected: FAIL + [link.nonce: IDL set to true] expected: FAIL @@ -710,6 +722,12 @@ [style.nonce: setAttribute() to "5%"] expected: FAIL + [style.nonce: setAttribute() to "+100"] + expected: FAIL + + [style.nonce: setAttribute() to ".5"] + expected: FAIL + [style.nonce: setAttribute() to true] expected: FAIL @@ -758,6 +776,12 @@ [style.nonce: IDL set to "5%"] expected: FAIL + [style.nonce: IDL set to "+100"] + expected: FAIL + + [style.nonce: IDL set to ".5"] + expected: FAIL + [style.nonce: IDL set to true] expected: FAIL diff --git a/testing/web-platform/meta/html/dom/reflection-misc.html.ini b/testing/web-platform/meta/html/dom/reflection-misc.html.ini index 8f2bd8561f31..891c3e5f8247 100644 --- a/testing/web-platform/meta/html/dom/reflection-misc.html.ini +++ b/testing/web-platform/meta/html/dom/reflection-misc.html.ini @@ -627,6 +627,12 @@ [script.nonce: setAttribute() to "5%"] expected: FAIL + [script.nonce: setAttribute() to "+100"] + expected: FAIL + + [script.nonce: setAttribute() to ".5"] + expected: FAIL + [script.nonce: setAttribute() to true] expected: FAIL @@ -675,6 +681,12 @@ [script.nonce: IDL set to "5%"] expected: FAIL + [script.nonce: IDL set to "+100"] + expected: FAIL + + [script.nonce: IDL set to ".5"] + expected: FAIL + [script.nonce: IDL set to true] expected: FAIL @@ -729,6 +741,12 @@ [undefinedelement.inputMode: setAttribute() to "5%"] expected: FAIL + [undefinedelement.inputMode: setAttribute() to "+100"] + expected: FAIL + + [undefinedelement.inputMode: setAttribute() to ".5"] + expected: FAIL + [undefinedelement.inputMode: setAttribute() to true] expected: FAIL @@ -897,6 +915,12 @@ [undefinedelement.inputMode: IDL set to "5%"] expected: FAIL + [undefinedelement.inputMode: IDL set to "+100"] + expected: FAIL + + [undefinedelement.inputMode: IDL set to ".5"] + expected: FAIL + [undefinedelement.inputMode: IDL set to true] expected: FAIL @@ -1257,12 +1281,24 @@ [undefinedelement.enterKeyHint: IDL set to "5%"] expected: FAIL + [undefinedelement.enterKeyHint: IDL set to "+100"] + expected: FAIL + + [undefinedelement.enterKeyHint: IDL set to ".5"] + expected: FAIL + [undefinedelement.enterKeyHint: setAttribute() to 1.5] expected: FAIL [undefinedelement.enterKeyHint: setAttribute() to "5%"] expected: FAIL + [undefinedelement.enterKeyHint: setAttribute() to "+100"] + expected: FAIL + + [undefinedelement.enterKeyHint: setAttribute() to ".5"] + expected: FAIL + [undefinedelement.enterKeyHint: setAttribute() to "next"] expected: FAIL diff --git a/testing/web-platform/meta/html/dom/reflection-obsolete.html.ini b/testing/web-platform/meta/html/dom/reflection-obsolete.html.ini index ac36f22b0a1a..48405f82baf6 100644 --- a/testing/web-platform/meta/html/dom/reflection-obsolete.html.ini +++ b/testing/web-platform/meta/html/dom/reflection-obsolete.html.ini @@ -29,6 +29,12 @@ [applet.align: setAttribute() to "5%"] expected: FAIL + [applet.align: setAttribute() to "+100"] + expected: FAIL + + [applet.align: setAttribute() to ".5"] + expected: FAIL + [applet.align: setAttribute() to true] expected: FAIL @@ -77,6 +83,12 @@ [applet.align: IDL set to "5%"] expected: FAIL + [applet.align: IDL set to "+100"] + expected: FAIL + + [applet.align: IDL set to ".5"] + expected: FAIL + [applet.align: IDL set to true] expected: FAIL @@ -131,6 +143,12 @@ [applet.alt: setAttribute() to "5%"] expected: FAIL + [applet.alt: setAttribute() to "+100"] + expected: FAIL + + [applet.alt: setAttribute() to ".5"] + expected: FAIL + [applet.alt: setAttribute() to true] expected: FAIL @@ -179,6 +197,12 @@ [applet.alt: IDL set to "5%"] expected: FAIL + [applet.alt: IDL set to "+100"] + expected: FAIL + + [applet.alt: IDL set to ".5"] + expected: FAIL + [applet.alt: IDL set to true] expected: FAIL @@ -233,6 +257,12 @@ [applet.archive: setAttribute() to "5%"] expected: FAIL + [applet.archive: setAttribute() to "+100"] + expected: FAIL + + [applet.archive: setAttribute() to ".5"] + expected: FAIL + [applet.archive: setAttribute() to true] expected: FAIL @@ -281,6 +311,12 @@ [applet.archive: IDL set to "5%"] expected: FAIL + [applet.archive: IDL set to "+100"] + expected: FAIL + + [applet.archive: IDL set to ".5"] + expected: FAIL + [applet.archive: IDL set to true] expected: FAIL @@ -335,6 +371,12 @@ [applet.code: setAttribute() to "5%"] expected: FAIL + [applet.code: setAttribute() to "+100"] + expected: FAIL + + [applet.code: setAttribute() to ".5"] + expected: FAIL + [applet.code: setAttribute() to true] expected: FAIL @@ -383,6 +425,12 @@ [applet.code: IDL set to "5%"] expected: FAIL + [applet.code: IDL set to "+100"] + expected: FAIL + + [applet.code: IDL set to ".5"] + expected: FAIL + [applet.code: IDL set to true] expected: FAIL @@ -446,6 +494,12 @@ [applet.codeBase: setAttribute() to "5%"] expected: FAIL + [applet.codeBase: setAttribute() to "+100"] + expected: FAIL + + [applet.codeBase: setAttribute() to ".5"] + expected: FAIL + [applet.codeBase: setAttribute() to true] expected: FAIL @@ -503,6 +557,12 @@ [applet.codeBase: IDL set to "5%"] expected: FAIL + [applet.codeBase: IDL set to "+100"] + expected: FAIL + + [applet.codeBase: IDL set to ".5"] + expected: FAIL + [applet.codeBase: IDL set to true] expected: FAIL @@ -557,6 +617,12 @@ [applet.height: setAttribute() to "5%"] expected: FAIL + [applet.height: setAttribute() to "+100"] + expected: FAIL + + [applet.height: setAttribute() to ".5"] + expected: FAIL + [applet.height: setAttribute() to true] expected: FAIL @@ -605,6 +671,12 @@ [applet.height: IDL set to "5%"] expected: FAIL + [applet.height: IDL set to "+100"] + expected: FAIL + + [applet.height: IDL set to ".5"] + expected: FAIL + [applet.height: IDL set to true] expected: FAIL @@ -776,6 +848,12 @@ [applet.hspace: setAttribute() to "5%"] expected: FAIL + [applet.hspace: setAttribute() to "+100"] + expected: FAIL + + [applet.hspace: setAttribute() to ".5"] + expected: FAIL + [applet.hspace: setAttribute() to true] expected: FAIL @@ -848,6 +926,12 @@ [applet.name: setAttribute() to "5%"] expected: FAIL + [applet.name: setAttribute() to "+100"] + expected: FAIL + + [applet.name: setAttribute() to ".5"] + expected: FAIL + [applet.name: setAttribute() to true] expected: FAIL @@ -896,6 +980,12 @@ [applet.name: IDL set to "5%"] expected: FAIL + [applet.name: IDL set to "+100"] + expected: FAIL + + [applet.name: IDL set to ".5"] + expected: FAIL + [applet.name: IDL set to true] expected: FAIL @@ -959,6 +1049,12 @@ [applet.object: setAttribute() to "5%"] expected: FAIL + [applet.object: setAttribute() to "+100"] + expected: FAIL + + [applet.object: setAttribute() to ".5"] + expected: FAIL + [applet.object: setAttribute() to true] expected: FAIL @@ -1016,6 +1112,12 @@ [applet.object: IDL set to "5%"] expected: FAIL + [applet.object: IDL set to "+100"] + expected: FAIL + + [applet.object: IDL set to ".5"] + expected: FAIL + [applet.object: IDL set to true] expected: FAIL @@ -1187,6 +1289,12 @@ [applet.vspace: setAttribute() to "5%"] expected: FAIL + [applet.vspace: setAttribute() to "+100"] + expected: FAIL + + [applet.vspace: setAttribute() to ".5"] + expected: FAIL + [applet.vspace: setAttribute() to true] expected: FAIL @@ -1259,6 +1367,12 @@ [applet.width: setAttribute() to "5%"] expected: FAIL + [applet.width: setAttribute() to "+100"] + expected: FAIL + + [applet.width: setAttribute() to ".5"] + expected: FAIL + [applet.width: setAttribute() to true] expected: FAIL @@ -1307,6 +1421,12 @@ [applet.width: IDL set to "5%"] expected: FAIL + [applet.width: IDL set to "+100"] + expected: FAIL + + [applet.width: IDL set to ".5"] + expected: FAIL + [applet.width: IDL set to true] expected: FAIL diff --git a/testing/web-platform/meta/html/rendering/dimension-attributes.html.ini b/testing/web-platform/meta/html/rendering/dimension-attributes.html.ini index 2d6a0ba4acd1..3be0a2edc443 100644 --- a/testing/web-platform/meta/html/rendering/dimension-attributes.html.ini +++ b/testing/web-platform/meta/html/rendering/dimension-attributes.html.ini @@ -1,224 +1,36 @@ [dimension-attributes.html] - [
mapping to width] - expected: FAIL - bug: https://github.com/whatwg/html/issues/4736 - - [