1196668 - height of IMG tag without src attribute set is 0 in Chrome, vs. line-height in Firefox (standards mode <img alt="..."> behavior)

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[Hallvord R. M. Steen [:hallvors]]

Attachment: imgnosourceheight.htm

If there's markup like <img> or <img width="200"> and you read element.height, what should it return?

Chrome says 0, and we've found a site depending on this behaviour - see 
https://github.com/webcompat/web-bugs/issues/463#issuecomment-132560088

It seems Firefox currently gives the line-height of the context or something like that.

Comment 1

[Timothy Nikkel (:tnikkel)]

In quirks mode the img gets a height of zero. The code responsible is http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsLineLayout.cpp?rev=28673cc5e68b#1796

Comment 2

[Daniel Holbert [:dholbert]]

I think this is because we fall back to rendering like a <span>, to display whatever alt-text happens to be provided. (in this case, the empty string)

i.e. we render these URIs the same:
  data:text/html,<img style="border:1px solid black">
  data:text/html,<span style="border:1px solid black"></span>
...whereas Chrome does not (they agree with us on the <span>; they disagree on the <img>).

Comment 3

[Daniel Holbert [:dholbert]]

IMO, our behavior here makes sense according to CSS2 rules for sizing inline elements.

The CSS2 section on calculating heights distinguishes between "Inline, non-replaced elements" (e.g. a span) and "Inline, replaced elements" (e.g. a video, or an image that has image data to render):
 https://drafts.csswg.org/css2/visudet.html#inline-non-replaced
And an <img> with no image data (no src attribute) is *non-replaced*, in the sense that it's not being filled in with image data. So, it's sized like a *non-replaced* element, i.e. like a <span>.

So, I think our behavior makes sense, according to the spec.

(I'm not sure what Chrome's doing, but I suspect they're (1) considering the element to be replaced, and (2) considering the element to *have* an intrinsic height, of 0. Given those assumptions, I think their behavior makes sense.)

Comment 4

[Daniel Holbert [:dholbert]]

For comparison, using <img style="border:1px solid black"> in standards-mode:
 - MS Edge matches Chrome -- it renders just a small dot, and renders a 0-height wide element if I add a 'width' attr.
 - IE 11 does its own thing -- it renders a 30x30 square, and renders a taller square if I add a 'width' attr.
 - Opera 12 (Presto) does yet another different thing -- it renders an approximately-line-height thing that just says the word "Image".

If we disregard IE11 and Opera 12 as deprecated, then that leaves us as the odd one out, in terms of our rendering behavior for <img> without a src attribute, among modern browsers.  So, even though our behavior is defensible from a spec perspective (comment 3), it might be worth considering a switch.

Comment 5

[Daniel Holbert [:dholbert]]

(Note: I didn't test any webkit-based browsers; I'm assuming they behave approximately like Chrome, too.)

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Gecko's behavior of treating image elements without data loaded as much more like regular inlines might not be something all browsers do -- and it might be something that now gets us into Web compatibility trouble.

Comment 7

[Timothy Nikkel (:tnikkel)]

Making img's always create an nsImageFrame (instead of an inline frame before we have decoded the size of the image and then an image frame) would simplify the code too.

Comment 8

[Boris Zbarsky [:bzbarsky]]

True.  It would also make alt text not really usable unless the alt text is extremely short and fits in the image box.  :(  Note that the HTML spec does cover this situation and we follow the spec; Chrome unfortunately does not.

Now what we could do is not fall back to the inline frame case if there is no alt text provided at all.  That might be a sensible thing to do, and may be compliant with the HTML spec; I haven't checked what the exact verbiage there is recently.

Comment 9

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Possibly related:

data:text/html,<!doctype html><img style="border:black" width=100 height=100>

Renders nothing in Nightly and a black border in Chrome. This shows up in Gmail messages with images not shown, since it takes no space the layout collapses awkwardly.

Comment 10

[Boris Zbarsky [:bzbarsky]]

I think we should consider changing the spec for the "no src, no alt text" case to not render as an inline.  The reason to render as an inline is to show the alt text sanely; if there is no alt text that's not a consideration.

Concretely, in spec terms, this would change the "If the element is an img element that represents nothing and the user agent does not expect this to change" case in <https://html.spec.whatwg.org/multipage/rendering.html#images-3>.  That would cover cases with no alt attr or alt="" and cover cases where the src is "" or not set at all or the load failed.

Does that sound plausible?

Comment 11

[Anne (:annevk)]

Yeah, though it's not clear to me what the new expectation would be. Treat the element as a replaced element whose content is nothing?

If the element has intrinsic dimensions we're still supposed to ignore them?

Comment 12

[Boris Zbarsky [:bzbarsky]]

> If the element has intrinsic dimensions we're still supposed to ignore them?

If the element has intrinsic dimensions and no alt attribute, that falls into the second case under https://html.spec.whatwg.org/multipage/rendering.html#images-3 to start with.

If it has explicit alt="", then we would fall into the case comment 10 is talking about.  Honestly, I have no strong opinions on how it should render.  If we don't make any special provisions apart from  my proposed change in comment 10, then it will render just like it would if we added this case to the cases in item 2 (so changed "the element has no alt attribute" to "the element has no alt attribute or its value is empty" or some such).

Comment 13

[Anne (:annevk)]

"the element has no alt attribute or represents nothing"? And then we could remove the fourth case? In any event, changing this seems fine if other browsers already do it.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> And then we could remove the fourth case?

No, because the second case is conditioned on having intrinsic dimensions.

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Attachment: Testcase with all src/alt combinations.

FWIW, Blink and WebKit also honor 'title' instead of alt... :(

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Attachment: Revised testcase (with / without specified dimensions)

Ugh, blink's logic at least (haven't checked WebKit) depends on 'specified dimensions' even in standards mode:

https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/html/html_image_fallback_helper.cc?l=115&rcl=ff6da4e16a772f2026a129de8cba0951c2cc85f6

Comment 19

[Boris Zbarsky [:bzbarsky]]

Yes, Blink generally does nothing resembling the spec here, with the result that it's common to get completely unreadable alt text in Blink.

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1196668 - Make images without alt generate a replaced box regardless of src.

Behavior-wise this only removes the HasAttr(src) check, and adds the IsEmpty()
check to the alt attribute value, since this function is only called for <img>
and <input>.

But it also cleans up a bit.

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1196668 - Update test expectations.

Only a few of the image wrappers compared against about:blank but I updated all
of them so people don't get confused if they try to do it.

test_invalid_img.xhtml was a crashtest.

I had to workaround bug 1505657 in another test.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/803b224d52a0
Make images without alt generate a replaced box regardless of src. r=bz

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e423aa1c94e
Last minute reftest fix.

Comment 24

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e608b4d239b
Backed out changeset 803b224d52a0 for failures at dom/html/reftests/468263-2.html on a CLOSED TREE

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff03b0b8ed81
Make images without alt generate a replaced box regardless of src. r=bz

Comment 26

[Raul Gurzau (:RaulG)]

Backed out 2 changesets (bug 1196668) for assertion count failures at child-constraints/available-size-for-percentages-vrl-htb.https.html

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/c6a0886cba2712f0e7c602734945109b51a87779

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=success%2Cpending%2Crunning%2Ctestfailed%2Cbusted%2Cexception&searchStr=linux%2Cdebug%2Cweb%2Cplatform%2Ctests%2Ctest-linux32%2Fdebug-web-platform-tests-reftests-1%2Cw%28wr1%29&revision=803b224d52a0940b4fb4b3b9cffc6a1fa6e5d4ee

Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=210723007&repo=mozilla-inbound&lineNumber=18095

Log snippet: 

[task 2018-11-09T02:31:39.359Z] 02:31:39     INFO - TEST-START | /css/css-layout-api/child-constraints/available-size-for-percentages-vrl-htb.https.html
[task 2018-11-09T02:31:39.367Z] 02:31:39     INFO - PID 7047 | 1541730699361	Marionette	INFO	Testing https://web-platform.test:8443/css/css-layout-api/child-constraints/available-size-for-percentages-vrl-htb.https.html == http://web-platform.test:8000/css/css-layout-api/green-square-ref.html
[task 2018-11-09T02:31:39.383Z] 02:31:39     INFO - PID 7047 | ++DOMWINDOW == 59 (0xdca24400) [pid = 7047] [serial = 59] [outer = 0xe2afb550]
[task 2018-11-09T02:31:39.444Z] 02:31:39     INFO - PID 7047 | ++DOMWINDOW == 60 (0xdca25800) [pid = 7047] [serial = 60] [outer = 0xe2afb550]
[task 2018-11-09T02:31:39.472Z] 02:31:39     INFO - PID 7047 | [7047, Main Thread] ###!!! ASSERTION: Our containing block must not have unconstrained inline-size!: 'aCBSize.ISize(aWM) != NS_UNCONSTRAINEDSIZE', file /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp, line 6194
[task 2018-11-09T02:31:39.473Z] 02:31:39     INFO - PID 7047 | [7047, Main Thread] ###!!! ASSERTION: Our containing block must not have unconstrained inline-size!: 'aCBSize.ISize(aWM) != NS_UNCONSTRAINEDSIZE', file /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp, line 6194
[task 2018-11-09T02:31:39.474Z] 02:31:39     INFO - PID 7047 | [7047, Main Thread] ###!!! ASSERTION: Our containing block must not have unconstrained inline-size!: 'aCBSize.ISize(aWM) != NS_UNCONSTRAINEDSIZE', file /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp, line 6194
[task 2018-11-09T02:31:39.475Z] 02:31:39     INFO - PID 7047 | [7047, Main Thread] ###!!! ASSERTION: Our containing block must not have unconstrained inline-size!: 'aCBSize.ISize(aWM) != NS_UNCONSTRAINEDSIZE', file /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp, line 6194
[task 2018-11-09T02:31:39.475Z] 02:31:39     INFO - PID 7047 | nsBlockReflowContext: Block(div)(0)@dcae8798 metrics=858994972,6000!
[task 2018-11-09T02:31:39.475Z] 02:31:39     INFO - PID 7047 | [7047, Main Thread] ###!!! ASSERTION: Our containing block must not have unconstrained inline-size!: 'aCBSize.ISize(aWM) != NS_UNCONSTRAINEDSIZE', file /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp, line 6194
[task 2018-11-09T02:31:39.475Z] 02:31:39     INFO - PID 7047 | [7047, Main Thread] ###!!! ASSERTION: Our containing block must not have unconstrained inline-size!: 'aCBSize.ISize(aWM) != NS_UNCONSTRAINEDSIZE', file /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp, line 6194
[task 2018-11-09T02:31:39.477Z] 02:31:39     INFO - PID 7047 | [7047, Main Thread] ###!!! ASSERTION: Our containing block must not have unconstrained inline-size!: 'aCBSize.ISize(aWM) != NS_UNCONSTRAINEDSIZE', file /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp, line 6194
[task 2018-11-09T02:31:39.478Z] 02:31:39     INFO - PID 7047 | [7047, Main Thread] ###!!! ASSERTION: Our containing block must not have unconstrained inline-size!: 'aCBSize.ISize(aWM) != NS_UNCONSTRAINEDSIZE', file /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp, line 6194
[task 2018-11-09T02:31:39.479Z] 02:31:39     INFO - PID 7047 | nsBlockReflowContext: Block(div)(0)@dcae8798 metrics=858994972,6000!
[task 2018-11-09T02:31:39.481Z] 02:31:39     INFO - PID 7047 | [7047, Main Thread] ###!!! ASSERTION: Our containing block must not have unconstrained inline-size!: 'aCBSize.ISize(aWM) != NS_UNCONSTRAINEDSIZE', file /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp, line 6194
[task 2018-11-09T02:31:39.481Z] 02:31:39     INFO - PID 7047 | [7047, Main Thread] ###!!! ASSERTION: Our containing block must not have unconstrained inline-size!: 'aCBSize.ISize(aWM) != NS_UNCONSTRAINEDSIZE', file /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp, line 6194
[task 2018-11-09T02:31:39.482Z] 02:31:39     INFO - PID 7047 | [7047, Main Thread] ###!!! ASSERTION: Our containing block must not have unconstrained inline-size!: 'aCBSize.ISize(aWM) != NS_UNCONSTRAINEDSIZE', file /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp, line 6194
[task 2018-11-09T02:31:39.484Z] 02:31:39     INFO - PID 7047 | [7047, Main Thread] ###!!! ASSERTION: Our containing block must not have unconstrained inline-size!: 'aCBSize.ISize(aWM) != NS_UNCONSTRAINEDSIZE', file /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp, line 6194
[task 2018-11-09T02:31:39.484Z] 02:31:39     INFO - PID 7047 | nsBlockReflowContext: Block(div)(0)@dcae8798 metrics=858994972,6000!
[task 2018-11-09T02:31:39.484Z] 02:31:39     INFO - PID 7047 | [7047, Main Thread] ###!!! ASSERTION: Our containing block must not have unconstrained inline-size!: 'aCBSize.ISize(aWM) != NS_UNCONSTRAINEDSIZE', file /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp, line 6194
[task 2018-11-09T02:31:39.486Z] 02:31:39     INFO - PID 7047 | [7047, Main Thread] ###!!! ASSERTION: Our containing block must not have unconstrained inline-size!: 'aCBSize.ISize(aWM) != NS_UNCONSTRAINEDSIZE', file /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp, line 6194
[task 2018-11-09T02:31:39.487Z] 02:31:39     INFO - PID 7047 | [7047, Main Thread] ###!!! ASSERTION: Our containing block must not have unconstrained inline-size!: 'aCBSize.ISize(aWM) != NS_UNCONSTRAINEDSIZE', file /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp, line 6194
[task 2018-11-09T02:31:39.487Z] 02:31:39     INFO - PID 7047 | [7047, Main Thread] ###!!! ASSERTION: Our containing block must not have unconstrained inline-size!: 'aCBSize.ISize(aWM) != NS_UNCONSTRAINEDSIZE', file /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp, line 6194
[task 2018-11-09T02:31:39.488Z] 02:31:39     INFO - PID 7047 | nsBlockReflowContext: Block(div)(0)@dcae8798 metrics=858994972,6000!
[task 2018-11-09T02:31:39.504Z] 02:31:39     INFO - PID 7047 | 1541730699500	Marionette	INFO	Found 65200 pixels different, maximum difference per channel 255
[task 2018-11-09T02:31:39.521Z] 02:31:39     INFO - Got chrome assert count 16
[task 2018-11-09T02:31:39.523Z] 02:31:39     INFO - TEST-UNEXPECTED-FAIL | /css/css-layout-api/child-constraints/available-size-for-percentages-vrl-htb.https.html | assertion count 16 is more than expected 0 assertions
[task 2018-11-09T02:31:39.524Z] 02:31:39     INFO - TEST-FAIL | /css/css-layout-api/child-constraints/available-size-for-percentages-vrl-htb.https.html | took 165ms

Comment 27

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

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

Comment 28

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

Upstream PR was closed without merging

Comment 29

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

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 30

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

Upstream PR was closed without merging

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8f6772dab9a
Make images without alt generate a replaced box regardless of src. r=bz

Comment 32

[Emilio Cobos Álvarez (:emilio)]

I annotated them since the bug is pre-existing.

Comment 33

[Mats Palmgren (inactive)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #19)
> Yes, Blink generally does nothing resembling the spec here, with the result
> that it's common to get completely unreadable alt text in Blink.

Did someone file a bug on Blink for the case(s) where it deviates from the spec?

Comment 34

[Boris Zbarsky [:bzbarsky]]

I filed https://bugs.chromium.org/p/chromium/issues/detail?id=671871 about two years ago, yes.

Comment 35

[Boris Zbarsky [:bzbarsky]]

And it looks like https://bugs.chromium.org/p/chromium/issues/detail?id=753868 also exists and is a bit over a year old.

Unfortunately, the people engaging from the Blink side seem pretty confused about what the spec is saying.  :(

Comment 36

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

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 37

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/c8f6772dab9a

Comment 38

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

Upstream PR merged

Comment 39

[Jason Laster [:jlast]]

Attachment: Screen Shot 2018-11-11 at 7.44.04 PM.png

Some of the devtools images began to look broken after this change landed on friday.

I think this happens because we are using img with a mask url property so that we can load an SVG and set the color with the background property.

Adding an alt attribute fixes it, but I wonder if it makes sense to require an alt attribute.

Comment 40

[Emilio Cobos Álvarez (:emilio)]

Which markup do you use?

Mind opening a separate bug for that, ideally with a reduced example? (I don't even know what I'm supposed to look at in that screenshot, I guess the debugger icons? :))

I'm happy to look into it. Sorry for the breakage :(