HTML: img width/height when not rendered are density-corrected - #61080
HTML: img width/height when not rendered are density-corrected#61080annevk wants to merge 3 commits into
Conversation
| assert_equals(img.width, expectedNotRenderedWidth, | ||
| assert_equals(img.width, expectedNaturalWidth, | ||
| 'width when not rendered'); | ||
| assert_equals(img.height, expectedNotRenderedHeight, | ||
| assert_equals(img.height, expectedNaturalHeight, |
There was a problem hiding this comment.
I think this change is wrong, actually - for the cases where the img has a specified width or height attribute, we expect the width/height JS APIs to return the value of that attribute, rather than returning the naturalWidth.
(This is true regardless of whether we're doing density-correction.)
e.g. the first image in this patch has:
<img src="resources/cat.jpg" width="10" height="10"
title="raster image with width/height attributes"
data-natural-width="320" data-natural-height="240"
data-width="10" data-height="10">
And we should expecting .width and .height to both return 10 here, not 320 and 240.
There was a problem hiding this comment.
Yeah, I think probably this commit needs to change:
- We need to preserve the
data-not-rendered-widthattributes that are manually specified in this test. - We need to preserve this mechanism for checking those attributes and using them as the expected result here.
- The part of this commit that's correct is the bit around line 330 ("Preserve the original 'data-natural-{width,height}' as the 'data-not-rendered-{width,height}' expectation") -- that's the specific bit that was testing this odd special-case which makes sense to remove.
But we don't want to remove the entire data-not-rendered-width mechanism, because it's not quite the same as data-width etc. (there are cases where one is specified but not the other, which are important to preserve. We can't just expect it to match the expected naturalWidth everywhere -- e.g. where the width attribute is manually specified, they'll be different. And we can't just expect data-width to give us the correct expectation for the not-rendered case -- e.g. where our rendered width comes from the broken image icon or the size of the containing block. Those are cases where the presence/absence of data-not-rendered-width helps us disambiguate to figure out the right expectation.
There was a problem hiding this comment.
When you say "we expect", what is the basis for that? The specification does not appear to consult the width and height content attributes and as far as I can tell never has.
There was a problem hiding this comment.
I'm not sure whether/where it's specced; I'm just going off of how browsers behave.
My intuition is that img.width and img.height are meant to be fancy accessors for the literal values of those attributes on the img element, which gracefully fall back to returning img.naturalWidth and img.naturalHeight if there are no specified attribute values.
As this patch currently stands, I think it's removing the part of the test that checks that these getters return the attribute-values, when we perform the test at 2x resolution (because it's simplifying the test to simply expect that they always match naturalWidth and naturalHeight for that part, basically, and removing that nuance about the connection to the element-attribute).
There was a problem hiding this comment.
This is aligning the test with the specification. If we want to preserve the attribute stuff somehow (but that only impacts the non-rendered path? seems silly) that would require a change to the specification as far as I can tell.
There was a problem hiding this comment.
Sorry for not replying til now, this slipped off my radar.
I agree that it looks like a spec change is needed for the non-rendered-img-with-width/height-attrs case. In the meantime, I'm reluctant to change the test in a way that removes all the code that handles the subtlety of how browsers handle that case (the data-not-rendered-height stuff).
My intent when writing this test in the first place was to test what browsers were currently doing (which had much more subtlety than what the spec said at the time). That's why it was .tentative at first. You've filled in most of the spec gaps, which I much appreciate! I'm not convinced that this remaining gap is one where it makes sense to change the test expectations (as opposed to leaving the test the same & changing the spec), though.
There was a problem hiding this comment.
In particular -- IIRC, the simplest relevant scenario I'm concerned about is this (no density correction needed/involved):
<img src="resources/cat.jpg" width="10" height="10"
title="raster image with width/height attributes"
[...]
>
...where cat.jpg is https://wpt.live/html/semantics/embedded-content/the-img-element/resources/cat.jpg , a 320x240 image.
Right now (before this PR is merged), the test expects (and we have interop on) the width/height and naturalWidth/naturalHeight APIs returning the following, regardless of whether the image is displayed:
img.width: 10
img.height: 10
img.naturalWidth: 320
img.naturalHeight: 240
Whereas, this PR as-it-currently-stands would change the expectation so that we'll expect img.width and img.height to return 320 and 240 for this element -- but only when the img is not displayed. When it is displayed, the test will still expect them to be 10 (matching the width and height attributes).
Independent of what we should do about density-correction (which is what this PR was originally about), this change doesn't make obvious sense to me.
There was a problem hiding this comment.
That^ simple scenario corresponds to the 4th row on https://wpt.fyi/results/html/semantics/embedded-content/the-img-element/naturalWidth-naturalHeight-width-height.html right now, where we're green across the board -- this is the test with Test Name raster image with width/height attributes (when not rendered).
You can see this test's subtests on the live test by expanding the 4th details element, which shows we're expecting the values that I described above (320, 240, 10, 10).
Whereas, if I replace the WPT with the version from this PR, I instead see this test (raster image with width/height attributes (when not rendered)) change its expectations such that it'll fail in all browsers, with this subtest-failure:
Fail assert_equals(10, 320, "width when not rendered")
(the height would fail too, but we bail as soon as we get one subtest failure in each section).
I can't say for sure whether real websites are bothering to set width/height attributes and then read them back on non-displayed images, but I would in no way be surprised if they were doing that, because I've seen folks doing similar-ish things. And in general, it seems super odd if width/height suddenly changed such that they return completely different things (reflecting vs. disregarding the width/height attributes) depending on whether the image is displayed vs. undisplayed.
There was a problem hiding this comment.
@annevk so that's why I'm concerned here.
I'm happy to take whatever behavior makes sense on density-correction, for consistency/simplicity. But the piece of this PR that expects us to disregard the width/height attributes for undisplayed images seems like a separate & bigger change that merits more consideration instead of being a ride-along to the density change that this PR is focusing on.
If that means this test is reflecting interoperable-reality and disagreeing with the spec, and if that's an uncomfortable place to be, then we could go back to renaming the test to .tentative; I'd prefer that rather than changing test-expectations for the currently-interoperable and seemingly-reasonable (to me at least) behavior around honoring the width/height attributes on non-displayed images just as we do for displayed images.
There was a problem hiding this comment.
Yeah that's fair.
- I restored the tests.
- Added some tests for
<input type=image>. - Created Consult the dimension attributes for width and height when not rendered whatwg/html#12840 so HTML remains aligned with the tests.
An available img that is not being rendered reports its density-corrected natural width and height (matching naturalWidth/naturalHeight), per the current specification; the width/height content attributes and rendered size do not apply.
Restore the data-not-rendered-{width,height} mechanism. The width and
height content attributes take precedence over the natural dimensions
when the element is not being rendered, per axis, in all three engines,
so only the density-correction half of these expectations changes here.
Also add coverage for input type=image, which behaves the same way and
had none.
212293d to
57e67ba
Compare
|
Opus+Codex review (continuation of whatwg/html#12840 (comment) ): 5. The new input test fails in Chrome Canary — stable over three runs. "width attribute only" and "height attribute only" fail on the rendered assertion: the test expects 6. Subtest deltas from applying the wpt PR (baseline taken with
Every img regression is a 7. Coverage gaps. The tests don't exercise the two places where the newly-cited parsing algorithm actually does anything: out-of-range values (finding 2) and non-canonical ones like Nits in |
|
Please take another look. |
6 participants
An available img that is not being rendered reports its density-corrected natural width and height (matching naturalWidth/naturalHeight), per the current specification; the width/height content attributes and rendered size do not apply.