Bug 209590 – REGRESSION (r254669): Expand media button doesn't work on first try on photos on reddit.com

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Antti Koivisto]

1. Visit old.reddit.com
2. Click the play button that expands an inline media preview
3. A lot of the time photos don’t appear
4. Click the x to collapse the non-visible preview
5. Click the play button again and the preview appears

[Antti Koivisto]

<rdar://problem/60461809>

[Antti Koivisto]

Created attachment 394591 [details]
patch

[Antti Koivisto]

Created attachment 394602 [details]
patch

[Antti Koivisto]

Created attachment 394606 [details]
patch

[Darin Adler]

Comment on attachment 394606 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394606&action=review

> Source/WebCore/rendering/RenderReplaced.cpp:597
> +        // For images with explicit width/height this updates the instrinsic size as a side effect.
> +        computeAspectRatioInformationForRenderBox(embeddedContentBox(), constrainedSize, intrinsicRatio);

Seems obviously a good fix for now.

Are we happy with this side effect relationship here for the longer term?

Also, what guarantees embeddedContentBox() is not nullptr?

[Antti Koivisto]

> Are we happy with this side effect relationship here for the longer term?

Not really, but in the catastrophe that is the current render tree it is a pretty minor detail.

> Also, what guarantees embeddedContentBox() is not nullptr?

It is a valid (and the common) case for it to be nullptr.

[EWS]

Committed r259051: <https://trac.webkit.org/changeset/259051>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394606 [details].

[cathiechen]

Comment on attachment 394606 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394606&action=review

> LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/img-aspect-ratio-expected.txt:3
> +FAIL Image width and height attributes are used to infer aspect-ratio assert_approx_equals: expected 4 +/- 0.001 but got 4.166666666666667

Hmmm, this failure blocks all the cases below...

[Antti Koivisto]

Like the ChangeLog says

"Failure here shifts to a different subtest. This one uses fractional pixels and LayoutUnit accuracy is not sufficient to compute the exact ratio."

It is a badly constructed test. It should be changed so that individual tests generate PASSes independently.

[cathiechen]

(In reply to Antti Koivisto from comment #9)
> Like the ChangeLog says
> 
> "Failure here shifts to a different subtest. This one uses fractional pixels
> and LayoutUnit accuracy is not sufficient to compute the exact ratio."
> 
> It is a badly constructed test. It should be changed so that individual
> tests generate PASSes independently.

Yes, agree...
I'm trying to upload a patch to work around these in https://bugs.webkit.org/show_bug.cgi?id=224664