1560055 - img with negative integers for the width or height attributes are ignored

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Karl Dubost💡 :karlcow]

originally reported on https://webcompat.com/issues/32763

<img 
   src="https://upload.wikimedia.org/wikipedia/en/b/b2/Francis_Bacon_-_Portrait_of_George_Dyer_Talking_%28reduced_300px%29.jpg" 
   width="-100">

https://codepen.io/webcompat/pen/ydaKLZ

The image is ignored in Firefox but is displayed in Chrome and Safari

while

<img 
  src="https://upload.wikimedia.org/wikipedia/en/b/b2/Francis_Bacon_-_Portrait_of_George_Dyer_Talking_%28reduced_300px%29.jpg" 
  width="Boo">

is displayed in all browsers.

Comment 1

[Karl Dubost💡 :karlcow]

In fact, the images are displayed but with a value 0 for width and height.

Comment 2

[Karl Dubost💡 :karlcow]

I wonder if it's related to
https://searchfox.org/mozilla-central/rev/b3b401254229f0a26f7ee625ef5f09c6c31e3949/dom/html/HTMLImageElement.h#92-99

Comment 3

[Boris Zbarsky [:bzbarsky]]

It's not. The relevant code in this case is https://searchfox.org/mozilla-central/rev/b3b401254229f0a26f7ee625ef5f09c6c31e3949/dom/html/nsGenericHTMLElement.cpp#1041-1042 which lands you in https://searchfox.org/mozilla-central/source/dom/base/nsAttrValue.cpp#1257,1264 so "-100" gets parsed as "0".

Spec requires "valid non-negative integers" as an author conformance requirement at https://html.spec.whatwg.org/multipage/embedded-content-other.html#attr-dim-width. In terms of the parsing behavior, the spec says to do https://html.spec.whatwg.org/multipage/rendering.html#dimRendering which links to https://html.spec.whatwg.org/multipage/rendering.html#maps-to-the-dimension-property which says to use https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#rules-for-parsing-dimension-values which returns an error at step 7 in this case. So seems like we shouldn't be storing the value as an int if it came back negative... The docs on ParseSpecialIntValue say it implements https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#rules-for-parsing-dimension-values but it doesn't seem to actually do what that algorithm says.

We use ParseSpecialIntValue in the following cases:

• <frame marginwidth>. Spec says to use https://html.spec.whatwg.org/#maps-to-the-pixel-length-property which uses "rules for parsing non-negative integers". Chrome and Safari follow the spec.
• <frame marginheight>. Same thing.
• <iframe marginwidth>. Same thing.
• <iframe marginheight>. Same thing.
• <hr width>. Spec says to use https://html.spec.whatwg.org/#maps-to-the-dimension-property and Chrome/Safari seem to follow the spec.
• <iframe width>. Same thing.
• <iframe height>. Same thing.
• <input width>. Same thing.
• <input height>. Same thing.
• <marquee width>. Same thing.
• <marquee height>. Same thing.
• <video width>. Same thing.
• <video height>. Same thing.
• <object/embed/img width/height>. Same thing.
• <td width>. Spec says to use https://html.spec.whatwg.org/#maps-to-the-dimension-property-(ignoring-zero). Our code ignores the value if it's <= 0 in HTMLTableCellElement::MapAttributesIntoRule so we actually match the spec here.
• <td height>. Same thing.
• <table width>. Same thing, but 0 (and hence negatives) ignored in HTMLTableElement::ParseAttribute in our code. Chrome/Safari allow 0.
• <table height>. Same thing per spec. In our code we will parse and clamp negatives to 0 and if you style the table with display: block you get layout that does not match the spec or Chrome/Safari. That said, Chrome/Safari allow 0, just not negatives. Filed https://github.com/whatwg/html/issues/4715 on this and the <table width> thing.
• <tr height>. Same thing per spec. Safari and Chrome allow 0, but not negatives. Filed https://github.com/whatwg/html/issues/4716
• <col width>. Same thing per spec. In our code we parse and clamp negatives to 0. Something like <table><colgroup><col width="-100"> with all styled as display:block and some script-added text inside the <col> renders wrong (0 width, not auto) in Gecko. Chrome and Safari allow 0, but not negatives. Filed https://github.com/whatwg/html/issues/4717
• <col charoff>. Really doesn't matter what we do here; its value is unused except in reflection.
• <tr width>. Really doesn't matter what we do here; its value is unused. Per spec it's not special.
• table section height attribute. Per spec this is not special. Chrome and Safari allow 0 but not negatives. Filed https://github.com/whatwg/html/issues/4718

Comment 4

[Boris Zbarsky [:bzbarsky]]

Note to self: just disallowing "negatives" would still treat -0 as 0. I did not test that in the tests above, so far...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Also, at least for <img width> Chrome and Safari don't treat +100 as a valid value, though per spec they should, afaict.

Comment 6

[Karl Dubost💡 :karlcow]

I added some cases to https://codepen.io/webcompat/pen/ydaKLZ
and discovered that width="50boo" (I had tested with width="50px" initially) is working in Gecko, Blink and WebKit.

Comment 7

[Boris Zbarsky [:bzbarsky]]

So the spec allows decimals, not just integers, in these various dimension values. Testing in Safari and Chrome with this testcase:

<body style="width: 800px">
<img src="https://searchfox.org/static/icons/search.png" width="100.5" onclick="alert(this.getBoundingClientRect().width)">
<img src="https://searchfox.org/static/icons/search.png" width="20.5%" onclick="alert(this.getBoundingClientRect().width)">
</body>

shows that those browsers in fact do that: I get "100.5" and "164" as the widths, whereas I get "100" and "160" in Firefox.

We could try to fix that, but that will require some thought in terms of the nsAttrValue storage. My temptation is to parse such values but then drop the fractional part for now, and file a followup bug for deciding what to do with that fractional part.

Comment 8

[Boris Zbarsky [:bzbarsky]]

and discovered that width="50boo" (I had tested with width="50px" initially) is working in Gecko, Blink and WebKit.

Yes, and the spec requires it to. width="50%boo" would also work, as a percentage.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1560055 part 1. Introduce a function to do what the HTML spec calls "parsing dimension values". r=mccr8

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1560055 part 2. Switch frame/iframe marginwidth/height parsing to follow the spec. r=mccr8

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1560055 part 3. Switch various "width" and "height" attributes on HTML elements to mostly follow the spec for parsing the attribute. r=mccr8

The spec allows non-integer values, but we don't have a good way to store them
in nsAttrValue yet. See https://bugzilla.mozilla.org/show_bug.cgi?id=1561440

HTMLTableCellElement::MapAttributesIntoRule can now call
MapImageSizeAttributesInto instead of manually mapping width and height, because
0 values (which it was excluding before) are now excluded at attribute parse
time.

For 'width' on HTMLTableElement I kept our old behavior for 0, which matches the spec
but not Safari or Chrome.

For 'height' on HTMLTableElement I kept our old behavior for 0, which matches
Safari and Chrome but not the spec. https://github.com/whatwg/html/issues/4715
tracks a possible spec change.

Same thing for 'height' on HTMLTableRowElement.

Same thing for 'width' on HTMLTableColElement.

The ParseImageAttribute call in HTMLMediaElement is not needed, because
HTMLAudioElement does not map any of those to style and HTMLVideoElement only
maps width/height, which it already parses.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1560055 part 4. Remove remaining uses of ParseSpecialIntValue. r=mccr8

We don't need to parse 'width' on <tr> because we never use the parsed value for
anything and neither does the spec.

We don't need to parse 'charoff' on <col> because we never use that for anything
either, and neither does the spec.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1560055 part 5. Fix various reflection cases that should be using GetUnsignedIntAttr but are using GetIntAttr. r=mccr8

It mostly works out, because we return an int32_t then just cast it to uint32_t,
but it would be better to return the right thing to start with.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1560055 part 6. Add a test for reflection of percentage values and fix our one failure there. r=mccr8

Our new behavior should align with the Blink/WebKit behavior and the current
spec, though I also filed https://github.com/whatwg/html/issues/4737 on the spec
for the fact that dimension attributes reflecting as integers is a bit weird

Comment 15

[Boris Zbarsky [:bzbarsky]]

https://github.com/whatwg/html/issues/4736 tracks the one other spec change that's probably needed here for compat with browsers.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1560055 part 7. Remove now-unused percentage handling in ParseHTMLInteger. r=mccr8

All callers treat '%' being found the same as not having consumed all the input,
so we can just stop consuming it.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9aa44cea14ee
part 1.  Introduce a function to do what the HTML spec calls "parsing dimension values".  r=mccr8
https://hg.mozilla.org/integration/autoland/rev/0a2e02d520c9
part 2.  Switch frame/iframe marginwidth/height parsing to follow the spec.  r=mccr8
https://hg.mozilla.org/integration/autoland/rev/f094b8e00c4c
part 3.  Switch various "width" and "height" attributes on HTML elements to mostly follow the spec for parsing the attribute.  r=mccr8
https://hg.mozilla.org/integration/autoland/rev/98d562c4a456
part 4.  Remove remaining uses of ParseSpecialIntValue.  r=mccr8
https://hg.mozilla.org/integration/autoland/rev/513a8dfd574e
part 5.  Fix various reflection cases that should be using GetUnsignedIntAttr but are using GetIntAttr.  r=mccr8
https://hg.mozilla.org/integration/autoland/rev/269942e95ea4
part 6.  Add a test for reflection of percentage values and fix our one failure there. r=mccr8
https://hg.mozilla.org/integration/autoland/rev/735e85ed0b38
part 7.  Remove now-unused percentage handling in ParseHTMLInteger.  r=mccr8

Comment 18

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/9aa44cea14ee
https://hg.mozilla.org/mozilla-central/rev/0a2e02d520c9
https://hg.mozilla.org/mozilla-central/rev/f094b8e00c4c
https://hg.mozilla.org/mozilla-central/rev/98d562c4a456
https://hg.mozilla.org/mozilla-central/rev/513a8dfd574e
https://hg.mozilla.org/mozilla-central/rev/269942e95ea4
https://hg.mozilla.org/mozilla-central/rev/735e85ed0b38

Comment 19

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/17708 for changes under testing/web-platform/tests

Comment 20

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged