-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HTML: test <img sizes=auto> #34427
HTML: test <img sizes=auto> #34427
Conversation
Not tested yet:
|
html/semantics/embedded-content/the-img-element/sizes/sizes-auto.html
Outdated
Show resolved
Hide resolved
html/semantics/embedded-content/the-img-element/sizes/sizes-auto.html
Outdated
Show resolved
Hide resolved
html/semantics/embedded-content/the-img-element/sizes/sizes-auto.html
Outdated
Show resolved
Hide resolved
html/semantics/embedded-content/the-img-element/sizes/sizes-auto.html
Outdated
Show resolved
Hide resolved
html/semantics/embedded-content/the-img-element/sizes/sizes-auto.html
Outdated
Show resolved
Hide resolved
I'll be off work between now and August 8. I can continue working on this then. |
Enjoy! Feel free to ping me once you need me to review changes, etc |
OK everything I was planning to test is now done. @yoavweiss PTAL |
|
||
t('width attribute changes', function(img) { | ||
img.width = '4'; | ||
}, 'load'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So update the image data re-fires the load event, even if the image hasn't changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I'm not a huge fan, but it's what browsers do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just did a test using only srcset with several options and the same url, and when I resized the browser window, I don't see the on-load event fire. Is this test expectation for auto-sizes correct? I would expect the on-load event to fire if a different image was selected, as it does for srcset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Browsers fire a 'load' event for relevant mutations, see https://software.hixie.ch/utilities/js/live-dom-viewer/saved/11615
Since the spec makes layout size changes a relevant mutation, the implication is that layout changes for the image are expected to fire load
even if currentSrc didn't change. See whatwg/html#8492 about changing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed pass conditions per whatwg/html#8008 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests LGTM assuming we can land this spec change from a compat perspective.
Spec PR: whatwg/html#8008 WPT tests PR: #34427 R=pdr Bug: 1359051 Change-Id: I8e9bc836e9582a9209db53d55e0edc96f90499f2
Spec PR: whatwg/html#8008 WPT tests PR: #34427 R=pdr Bug: 1359051 Change-Id: I8e9bc836e9582a9209db53d55e0edc96f90499f2
Spec PR: whatwg/html#8008 WPT tests PR: #34427 R=pdr Bug: 1359051 Change-Id: I8e9bc836e9582a9209db53d55e0edc96f90499f2
Spec PR: whatwg/html#8008 WPT tests PR: #34427 R=pdr Bug: 1359051 Change-Id: I8e9bc836e9582a9209db53d55e0edc96f90499f2
Spec PR: whatwg/html#8008 WPT tests PR: #34427 R=pdr Bug: 1359051 Change-Id: I8e9bc836e9582a9209db53d55e0edc96f90499f2
Spec PR: whatwg/html#8008 WPT tests PR: #34427 R=pdr Bug: 1359051 Change-Id: I8e9bc836e9582a9209db53d55e0edc96f90499f2
// lazy-loaded images should have fired their first 'load' event at this point. | ||
|
||
t('width attribute changes', function(img) { | ||
img.width = '4'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this test also expect timeout
, like the test bellow which changes img.style.width
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The width attribute is listed as a relevant mutation: https://html.spec.whatwg.org/#relevant-mutations
However, it might be a leftover that should have been removed in whatwg/html#5900 . I can file an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
|
||
t('loading attribute state changes', function(img) { | ||
img.loading = 'eager'; | ||
}, 'load'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the image is already loaded, why would changing the loading
attribute trigger a load event?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
State changes to the loading
attribute is a relevant mutation per https://github.com/whatwg/html/pull/8008/files#diff-41cf6794ba4200b839c53531555f0f3998df4cbb01a4d5cb0b94e3ca5e23947dR30115-R30116
The "auto" keyword is calculated differently depending on the state of loading
. But I'm now uncertain if it should be a relevant mutation. https://html.spec.whatwg.org/#the-img-element:lazy-loading-attribute already handles the state changing to Eager, so also making it a relevant mutation is questionable...
@yoavweiss @domfarolino any opinions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it seems that that specific algorithm replaces the relevant mutation logic (and is more specific). So I'm favorable of removing it from the relevant mutations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Thinking about it again, it's invalid to use auto
for eager images, so if you want to switch to eager you should also change sizes
(which is a relevant mutation). OK I'll fix the spec and tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would happen if authors don't change sizes
in that case? Would that modify the image's intrinsic dimensions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, auto would compute to 100vw, but not until another relevant mutation happens I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, that runs a risk of being confusing.
I wonder if there's room to define auto
+ eager
as taking the known dimensions in case they are indeed known (and only fallback to 100vw when they aren't)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer the invalid mutation case being somewhat confusing to introducing non-deterministic loading of eager images.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair! (and apologies for rehashing a past discussion :D)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
np :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall the change looks good to me. I only have 2 questions on relevant-mutations-lazy.html
.
1c7b50e
to
a442c1c
Compare
Auto Sizes allows developers to omit the sizes attribute (or set it to sizes=auto) for lazy loaded image elements with srcset. For this use case the image's width should be provided. The browser will use the layout width of the image in order to select the source url from the srcset. Spec PR: whatwg/html#8008 WPT tests PR: #34427 Chrome Platform Status link: https://chromestatus.com/feature/5191555708616704 Intent to Prototype link: https://groups.google.com/a/chromium.org/g/blink-dev/c/AYoqvNluyeA R=pdr Bug: 1359051 Change-Id: I8e9bc836e9582a9209db53d55e0edc96f90499f2
Auto Sizes allows developers to omit the sizes attribute (or set it to sizes=auto) for lazy loaded image elements with srcset. For this use case the image's width should be provided. The browser will use the layout width of the image in order to select the source url from the srcset. Spec PR: whatwg/html#8008 WPT tests PR: #34427 Chrome Platform Status link: https://chromestatus.com/feature/5191555708616704 Intent to Prototype link: https://groups.google.com/a/chromium.org/g/blink-dev/c/AYoqvNluyeA R=pdr Bug: 1359051 Change-Id: I8e9bc836e9582a9209db53d55e0edc96f90499f2
Auto Sizes allows developers to omit the sizes attribute (or set it to sizes=auto) for lazy loaded image elements with srcset. For this use case the image's width should be provided. The browser will use the layout width of the image in order to select the source url from the srcset. Spec PR: whatwg/html#8008 WPT tests PR: #34427 Chrome Platform Status link: https://chromestatus.com/feature/5191555708616704 Intent to Prototype link: https://groups.google.com/a/chromium.org/g/blink-dev/c/AYoqvNluyeA R=pdr Bug: 1359051 Change-Id: I8e9bc836e9582a9209db53d55e0edc96f90499f2
Auto Sizes allows developers to omit the sizes attribute (or set it to sizes=auto) for lazy loaded image elements with srcset. For this use case the image's width should be provided. The browser will use the layout width of the image in order to select the source url from the srcset. Spec PR: whatwg/html#8008 WPT tests PR: #34427 Chrome Platform Status link: https://chromestatus.com/feature/5191555708616704 Intent to Prototype link: https://groups.google.com/a/chromium.org/g/blink-dev/c/AYoqvNluyeA R=pdr Bug: 1359051 Change-Id: I8e9bc836e9582a9209db53d55e0edc96f90499f2
Auto Sizes allows developers to omit the sizes attribute (or set it to sizes=auto) for lazy loaded image elements with srcset. For this use case the image's width should be provided. The browser will use the layout width of the image in order to select the source url from the srcset. Spec PR: whatwg/html#8008 WPT tests PR: #34427 Chrome Platform Status link: https://chromestatus.com/feature/5191555708616704 Intent to Prototype link: https://groups.google.com/a/chromium.org/g/blink-dev/c/AYoqvNluyeA R=pdr Bug: 1359051 Low-Coverage-Reason: For html_srcset_parser.cc, it should be covered by `image-srcset-cache.html` but there seems to be cov reporing problem. Change-Id: I8e9bc836e9582a9209db53d55e0edc96f90499f2
Auto Sizes allows developers to omit the sizes attribute (or set it to sizes=auto) for lazy loaded image elements with srcset. For this use case the image's width should be provided. The browser will use the layout width of the image in order to select the source url from the srcset. Spec PR: whatwg/html#8008 WPT tests PR: #34427 Chrome Platform Status link: https://chromestatus.com/feature/5191555708616704 Intent to Prototype link: https://groups.google.com/a/chromium.org/g/blink-dev/c/AYoqvNluyeA R=pdr Bug: 1359051 Low-Coverage-Reason: For html_srcset_parser.cc, it should be covered by `image-srcset-cache.html` but there seems to be cov reporing problem. Change-Id: I8e9bc836e9582a9209db53d55e0edc96f90499f2
Auto Sizes allows developers to omit the sizes attribute (or set it to sizes=auto) for lazy loaded image elements with srcset. For this use case the image's width should be provided. The browser will use the layout width of the image in order to select the source url from the srcset. Spec PR: whatwg/html#8008 WPT tests PR: #34427 Chrome Platform Status link: https://chromestatus.com/feature/5191555708616704 Intent to Prototype link: https://groups.google.com/a/chromium.org/g/blink-dev/c/AYoqvNluyeA R=pdr Bug: 1359051 Low-Coverage-Reason: For html_srcset_parser.cc, it should be covered by `image-srcset-cache.html` but there seems to be cov reporing problem. Change-Id: I8e9bc836e9582a9209db53d55e0edc96f90499f2
Auto Sizes allows developers to omit the sizes attribute (or set it to sizes=auto) for lazy loaded image elements with srcset. For this use case the image's width should be provided. The browser will use the layout width of the image in order to select the source url from the srcset. Spec PR: whatwg/html#8008 WPT tests PR: #34427 Chrome Platform Status link: https://chromestatus.com/feature/5191555708616704 Intent to Prototype link: https://groups.google.com/a/chromium.org/g/blink-dev/c/AYoqvNluyeA R=pdr Bug: 1359051 Low-Coverage-Reason: For html_srcset_parser.cc, it should be covered by `image-srcset-cache.html` but there seems to be cov reporing problem. Change-Id: I8e9bc836e9582a9209db53d55e0edc96f90499f2
Auto Sizes allows developers to omit the sizes attribute (or set it to sizes=auto) for lazy loaded image elements with srcset. For this use case the image's width should be provided. The browser will use the layout width of the image in order to select the source url from the srcset. Spec PR: whatwg/html#8008 WPT tests PR: #34427 Chrome Platform Status link: https://chromestatus.com/feature/5191555708616704 Intent to Prototype link: https://groups.google.com/a/chromium.org/g/blink-dev/c/AYoqvNluyeA R=pdr Bug: 1359051 Low-Coverage-Reason: For html_srcset_parser.cc, it should be covered by `image-srcset-cache.html` but there seems to be cov reporing problem. Change-Id: I8e9bc836e9582a9209db53d55e0edc96f90499f2
Auto Sizes allows developers to omit the sizes attribute (or set it to sizes=auto) for lazy loaded image elements with srcset. For this use case the image's width should be provided. The browser will use the layout width of the image in order to select the source url from the srcset. Spec PR: whatwg/html#8008 WPT tests PR: #34427 Chrome Platform Status link: https://chromestatus.com/feature/5191555708616704 Intent to Prototype link: https://groups.google.com/a/chromium.org/g/blink-dev/c/AYoqvNluyeA R=pdr Bug: 1359051 Low-Coverage-Reason: For html_srcset_parser.cc, it should be covered by `image-srcset-cache.html` but there seems to be cov reporing problem. Change-Id: I8e9bc836e9582a9209db53d55e0edc96f90499f2
Auto Sizes allows developers to omit the sizes attribute (or set it to sizes=auto) for lazy loaded image elements with srcset. For this use case the image's width should be provided. The browser will use the layout width of the image in order to select the source url from the srcset. Spec PR: whatwg/html#8008 WPT tests PR: #34427 Chrome Platform Status link: https://chromestatus.com/feature/5191555708616704 Intent to Prototype link: https://groups.google.com/a/chromium.org/g/blink-dev/c/AYoqvNluyeA R=pdr Bug: 1359051 Low-Coverage-Reason: For html_srcset_parser.cc, it should be covered by `image-srcset-cache.html` but there seems to be cov reporing problem. Change-Id: I8e9bc836e9582a9209db53d55e0edc96f90499f2
Auto Sizes allows developers to omit the sizes attribute (or set it to sizes=auto) for lazy loaded image elements with srcset. For this use case the image's width should be provided. The browser will use the layout width of the image in order to select the source url from the srcset. Spec PR: whatwg/html#8008 WPT tests PR: #34427 Chrome Platform Status link: https://chromestatus.com/feature/5191555708616704 Intent to Prototype link: https://groups.google.com/a/chromium.org/g/blink-dev/c/AYoqvNluyeA R=pdr Bug: 1359051 Low-Coverage-Reason: For html_srcset_parser.cc, it should be covered by `image-srcset-cache.html` but there seems to be cov reporing problem. Change-Id: I8e9bc836e9582a9209db53d55e0edc96f90499f2
Auto Sizes allows developers to omit the sizes attribute (or set it to sizes=auto) for lazy loaded image elements with srcset. For this use case the image's width should be provided. The browser will use the layout width of the image in order to select the source url from the srcset. Spec PR: whatwg/html#8008 WPT tests PR: #34427 Chrome Platform Status link: https://chromestatus.com/feature/5191555708616704 Intent to Prototype link: https://groups.google.com/a/chromium.org/g/blink-dev/c/AYoqvNluyeA R=pdr Bug: 1359051 Low-Coverage-Reason: For html_srcset_parser.cc, it should be covered by `image-srcset-cache.html` but there seems to be cov reporing problem. Change-Id: I8e9bc836e9582a9209db53d55e0edc96f90499f2
Auto Sizes allows developers to omit the sizes attribute (or set it to sizes=auto) for lazy loaded image elements with srcset. For this use case the image's width should be provided. The browser will use the layout width of the image in order to select the source url from the srcset. Spec PR: whatwg/html#8008 WPT tests PR: #34427 Chrome Platform Status link: https://chromestatus.com/feature/5191555708616704 Intent to Prototype link: https://groups.google.com/a/chromium.org/g/blink-dev/c/AYoqvNluyeA R=pdr Bug: 1359051 Low-Coverage-Reason: For html_srcset_parser.cc, it should be covered by `image-srcset-cache.html` but there seems to be cov reporing problem. Change-Id: I8e9bc836e9582a9209db53d55e0edc96f90499f2
See whatwg/html#8008