1765654 - Simplify nsVideoFrame::GetMinISize (and restore consistent behavior between <audio controls> vs. other replaced elements in orthogonal-flow abspos situations)

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

I'm filing this bug for dshin's followup patch on the contain:inline-size implementation, to do some refactoring.

Note that bug 1716612 subtly changed our behavior here, for <audio controls>; this bug restores our previous behavior, as demonstrated by the attached testcase.

Comment 1

[Daniel Holbert [:dholbert]]

This is effectively fixing a regression from bug 1716612 (albeit not a regression that any web content likely cares about).

--> Marking as-such.

Before bug 1716612, we render testcase 1 with all of the content sticking off the right side of the red box. After bug 1716612, the audio controls element is inside the red box (because we incorrectly think it has an intrinsic size of 0 and that influences our positioning logic, so that it's origin with respect to the vertical-rl containing block (its top-right corner) gets placed as if it were 0 sized. Or something along those lines.

dshin's patch in https://phabricator.services.mozilla.com/D144182 restores our previous behavior (with the audio element running off the right side of the red box like everything else does)

Comment 2

[David Shin[:dshin]]

Attachment: Bug 1765654 - Simplify audio/video frame intrinsic sizing, and make audio elements report the correct intrinsic width during layout r=dholbert

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (possibly usable as reftest)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: reference case 2 (possibly usable as reftest)

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1716612

Comment 6

[Daniel Holbert [:dholbert]]

dshin, when you're writing tests to include in the patch here: my attached "testcase 2 / reference case 2" files might be usable to get you started on a reftest (or WPT rendering test) here, though we should probably include a few additional versions (or a few additional lines in the test) with canvas and video at least, to have some coverage for this general codepath with at least a few different replaced elements (not just audio).

These tests could conceivably go in testing/web-platform/tests/css/css-writing-modes, maybe with files being named abs-pos-replaced-vrl-001.tentative.html or something like that. The .tentative string is important, since it's not clear to me that our "reference case" behavior is truly correct... As it happens, Chrome happens to "pass" my attached testcase 2 / reference case 2 pair-of-files (as does Firefox release and Firefox Nightly with your patch, I think); but WebKit does not, and I sort of think nobody is correct.

(I say that I think nobody is correct because: theoretically, position:absolute content (with default top/right/bottom/left properties) is supposed to be positioned at the same spot where it would be, if it didn't have position:absolute. So if you disable/enable position:absolute in devtools on the attached "testcase 2", the position should not change, unless there's some writing-mode-specific special case that I'm forgetting about. In current Nightly, we actually match that expectation of mine specifically for <audio controls> due to this bug. :) but we match my expectation for bad/broken reasons and it's inconsistent.)

Alternately: given that our tests' required-behavior is ~debatable, we could just put the tests in our own private reftests directory and not bother anyone else with them (though they may turn up interesting assertions/failures in other browsers, so it's kind-of nice to share them).

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #6)

(I say that I think nobody is correct because [...]

Side note, I just filed bug 1765664 on this, i.e. the fact that we're still probably not correct even with this patch (but I do still want to take the patch because it removes an inconsistency.)

Comment 8

[Daniel Holbert [:dholbert]]

Ah, IanK helped me through my "I think nobody is correct" confusion, over in bug 1765664 comment 5.

So I think we can write a valid & sharable WPT for this after all (no need for any .tentative business discussed in comment 6).

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: testcase 3 (better starting material for a WPT test)

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: reference case 3

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b6cd39aa0353
Simplify audio/video frame intrinsic sizing, and make audio elements report the correct intrinsic width during layout r=dholbert,emilio

Comment 12

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/33816 for changes under testing/web-platform/tests

Comment 13

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/b6cd39aa0353

Comment 14

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot