Bug 224197

Thanks, this reproduces on Safari 13.1.2 as well as TOT 14.2 using the provided codepen.

<rdar://problem/76359912>

Created attachment 426098 [details] error-image-test Thanks! I can reproduce it. This seems related to error images. Because the src attribute is empty, so <img> is treated as an error image. The current implementation does not map width height attribute to aspect ratio for error image. However, as this feature developed, looks like it should apply to the empty src <img> without alt. And the wpt test img-aspect-ratio.html has updated. See, https://github.com/whatwg/html/pull/6032 So I think it makes sense to update the behavior of error images.

(In reply to cathiechen from comment #3) > Created attachment 426098 [details] > error-image-test > > Thanks! I can reproduce it. > > This seems related to error images. > Because the src attribute is empty, so <img> is treated as an error image. > The current implementation does not map width height attribute to aspect > ratio for error image. > However, as this feature developed, looks like it should apply to the empty > src <img> without alt. And the wpt test img-aspect-ratio.html has updated. > See, https://github.com/whatwg/html/pull/6032 > > So I think it makes sense to update the behavior of error images. Interestingly, I still experienced the issue after including an alt for each image. Could be a cache issue on my end, as I had previously been testing on a production website before I whipped up that CodePen, but figured I ought to add that information.

(In reply to Owen Sullivan from comment #4) > Interestingly, I still experienced the issue after including an alt for each > image. Could be a cache issue on my end, as I had previously been testing on > a production website before I whipped up that CodePen, but figured I ought > to add that information. Yeah, I analyzed the page a bit. After this page is loaded, <img>s don't have src attribute(use data-src instead). For instance, `<img class="lazyload img-fluid" data-src="https://placehold.jp/1000x2000.png" width="1000" height="2000">` So the browser think this <img> is an error image, and doesn't apply the feature (mapping width height attributes to aspect-ratio) After clicking "Nav 4", <img> will get the src attribute. `<img class="img-fluid ls-is-cached lazyloaded" data-src="https://placehold.jp/1000x2000.png" width="1000" height="2000" src="https://placehold.jp/1000x2000.png">` So <img> is not an error image now. And I don't think <img> should have alt in this case, because an error image with alt won't apply the feature.

Probably a dumb question, but simply because I'm new to WebKit bugs: Smoley mentioned that the issue reproduces on Safari 13.1.2 in addition to Safari 14, although I wasn't aware the feature existed before Safari 14. In any case, can the fix be retroactively merged into existing versions of Safari through a library update? Or does it require a full application update? I'm not at all aware of what processes you all have in place to apply fixes, but if it can somehow be done retroactively it would really help with browser cross-compatibility and web design for users that don't frequently update their browsers.

Created attachment 426536 [details] Patch

Comment on attachment 426536 [details] Patch Hi, I think this patch is ready for review:)

Comment on attachment 426536 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426536&action=review > Source/WebCore/rendering/RenderReplaced.cpp:477 > +bool RenderReplaced::getAspectRatioFromWidthHeight(double& intrinsicRatio) const This should probably return Optional<double>.

Comment on attachment 426536 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426536&action=review This is OK, but I think it could be made better. Suggestions below. > Source/WebCore/rendering/RenderImage.cpp:854 > + if (!element() || !is<HTMLImageElement>(*element())) > + return false; > + > + if (isShowingAltText()) > + return false; > + > + return true; The is<> function includes built-in null checking, so this can be written like this: return is<HTMLIMageElement>(element()) && isShowingAltText(); I think that’s clearer than the multiple return statements. >> Source/WebCore/rendering/RenderReplaced.cpp:477 >> +bool RenderReplaced::getAspectRatioFromWidthHeight(double& intrinsicRatio) const > > This should probably return Optional<double>. Yes! And to follow WebKIt coding style we’d then remove the word "get" from the name. Not even sure that "FromWidthHeight" is a good way to name this. I think it should be named intrinsicAspectRatio. > Source/WebCore/rendering/RenderReplaced.h:61 > + virtual bool canMapWidthHeightAsAspectRatio() const { return false; } I don’t understand the name of this function. I don’t know what "map width height as aspect ratio" means. Maybe this should be named hasIntrinsicAspectRatio? I am not 100% sure what the concept is. For example, if this returns false does that mean we have no intrinsic aspect ratio at all? Or does it just mean it should be ignored in RenderImage::computeIntrinsicRatioInformation? > LayoutTests/imported/w3c/ChangeLog:9 > + The test cases for error images and images without src in img-aspect-ratio.html are passed. This patch > + doesn't change the behavior of the original aspect ratio case, so it's failed like before. Before doing more work here, I think we should take the time to restructure this test so it does multiple PASS/FAIL and not just a single FAIL. I expect that won’t be too difficult.

Comment on attachment 426536 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426536&action=review Hi Darin and Rob, Thanks for the review! >> Source/WebCore/rendering/RenderImage.cpp:854 >> + return true; > > The is<> function includes built-in null checking, so this can be written like this: > > return is<HTMLIMageElement>(element()) && isShowingAltText(); > > I think that’s clearer than the multiple return statements. Done, thanks! Changed to: return is<HTMLIMageElement>(element()) && !isShowingAltText(); >>> Source/WebCore/rendering/RenderReplaced.cpp:477 >>> +bool RenderReplaced::getAspectRatioFromWidthHeight(double& intrinsicRatio) const >> >> This should probably return Optional<double>. > > Yes! > > And to follow WebKIt coding style we’d then remove the word "get" from the name. Not even sure that "FromWidthHeight" is a good way to name this. I think it should be named intrinsicAspectRatio. Done, thanks! intrinsicAspectRatioFromWidthHeight? I think maybe it's good to have "FromWidthHeight" to indicate that the aspect ratio is calculated by attributes width and height? >> Source/WebCore/rendering/RenderReplaced.h:61 >> + virtual bool canMapWidthHeightAsAspectRatio() const { return false; } > > I don’t understand the name of this function. I don’t know what "map width height as aspect ratio" means. Maybe this should be named hasIntrinsicAspectRatio? > > I am not 100% sure what the concept is. For example, if this returns false does that mean we have no intrinsic aspect ratio at all? Or does it just mean it should be ignored in RenderImage::computeIntrinsicRatioInformation? Sorry for the terribly naming. It means we can compute the aspect ratio from attribute width / attribute height before image loaded. It is defined here https://html.spec.whatwg.org/#map-to-the-aspect-ratio-property Currently, only img is allowed. I think next step we can expend it to more elements, like canvas, video, input with image type and picture etc. This function determines which element is allowed to get aspect ratio from width and height. So canMapWidthHeightToAspectRatio? I added some explanations to this function. >> LayoutTests/imported/w3c/ChangeLog:9 >> + doesn't change the behavior of the original aspect ratio case, so it's failed like before. > > Before doing more work here, I think we should take the time to restructure this test so it does multiple PASS/FAIL and not just a single FAIL. I expect that won’t be too difficult. Sure, I think I can work on this:)

Created attachment 426667 [details] Patch

Created attachment 426917 [details] Patch

Committed r276521 (236975@main): <https://commits.webkit.org/236975@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 426917 [details].

Downloaded nightly build 276521 from https://s3-us-west-2.amazonaws.com/minified-archives.webkit.org/mac-bigsur-x86_64%20arm64-release/276521.zip (as well as the most recent nightly) to try and test this fix, and I'm still experiencing the original problem. Should I be expect this and wait for a later commit, or is this a problem?

(In reply to Owen Sullivan from comment #15) > Downloaded nightly build 276521 from > https://s3-us-west-2.amazonaws.com/minified-archives.webkit.org/mac-bigsur- > x86_64%20arm64-release/276521.zip (as well as the most recent nightly) to > try and test this fix, and I'm still experiencing the original problem. > Should I be expect this and wait for a later commit, or is this a problem? Hi Owen, Sorry, for some reason I can't install the nightly build in my Mac. I tried it in my local build(commit: f630ca6238 May 3rd 2021), it works as expected in https://codepen.io/sulliops/full/QWdwRxx As for error-image-test in Attachments, there's a nit(not `width="1000px" height="2000px"`, should be `width="1000" height="2000"` instead). After fixing this, it works as expected too.

Created attachment 427979 [details] error-image-test

(In reply to Owen Sullivan from comment #15) > Downloaded nightly build 276521 ... to try and test this fix, and I'm still experiencing the original problem I came here by way of Safari Technology Preview v125 release notes. I can verify that Safari TP 125 no longer has the error (while I can reproduce the error in Safari 14.1.1). Great CodePen test case! We'll have to look forward to a future Safari update to see this hit Safari proper. Thank you @cathiechen for addressing this! 🙇‍♂️