Bug 209590 - REGRESSION (r254669): Expand media button doesn't work on first try on photos on reddit.com
Summary: REGRESSION (r254669): Expand media button doesn't work on first try on photos...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-03-26 03:30 PDT by Antti Koivisto
Modified: 2021-04-16 05:59 PDT (History)
10 users (show)

See Also:


Attachments
patch (3.75 KB, patch)
2020-03-26 03:58 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (5.91 KB, patch)
2020-03-26 06:58 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (5.90 KB, patch)
2020-03-26 07:32 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2020-03-26 03:30:55 PDT
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
Comment 1 Antti Koivisto 2020-03-26 03:31:19 PDT
<rdar://problem/60461809>
Comment 2 Antti Koivisto 2020-03-26 03:58:56 PDT
Created attachment 394591 [details]
patch
Comment 3 Antti Koivisto 2020-03-26 06:58:57 PDT
Created attachment 394602 [details]
patch
Comment 4 Antti Koivisto 2020-03-26 07:32:38 PDT
Created attachment 394606 [details]
patch
Comment 5 Darin Adler 2020-03-26 10:28:42 PDT
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?
Comment 6 Antti Koivisto 2020-03-26 10:39:38 PDT
> 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.
Comment 7 EWS 2020-03-26 10:44:51 PDT
Committed r259051: <https://trac.webkit.org/changeset/259051>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394606 [details].
Comment 8 cathiechen 2021-04-16 03:34:03 PDT
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...
Comment 9 Antti Koivisto 2021-04-16 05:31:41 PDT
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.
Comment 10 cathiechen 2021-04-16 05:59:21 PDT
(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