Closed Bug 1765654 Opened 1 year ago Closed 1 year ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
101 Branch
Tracking Status
firefox-esr91 --- unaffected
firefox99 --- unaffected
firefox100 --- unaffected
firefox101 --- fixed

People

(Reporter: dholbert, Assigned: dshin)

References

(Regression)

Details

(Keywords: regression)

Attachments

(6 files)

Attached file 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.

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)

Regressed by: 1716612
Assignee: nobody → dshin
Component: Layout → Layout: Images, Video, and HTML Frames

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

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).

See Also: → 1765664

(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.)

Has Regression Range: --- → yes

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).

Attached file reference case 3
Attachment #9273157 - Attachment description: Bug 1755565 - Simplify nsVideoFrame::GetMinISize, r=dholbert → Bug 1765654 - nsVideoFrame::GetMinISize, fix regression in nsVideoFrame::GetIntrinsicSize r=dholbert
See Also: → 1766304
Attachment #9273157 - Attachment description: Bug 1765654 - nsVideoFrame::GetMinISize, fix regression in nsVideoFrame::GetIntrinsicSize r=dholbert → Bug 1765654 - Simplify audio/video frame intrinsic sizing, and make audio elements report the correct intrinsic width during layout r=dholbert
Pushed by dholbert@mozilla.com:
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/33816 for changes under testing/web-platform/tests
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 101 Branch
Upstream PR merged by moz-wptsync-bot
You need to log in before you can comment on or make changes to this bug.