Make inline elements work in fullscreen mode #24034

ferjm · 2019-08-22T14:55:24Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #22358

This change is

ferjm · 2019-08-22T14:56:10Z

r? @emilio I am not sure if this is the right fix. But this patch makes fullscreen mode work for video elements.

emilio

This goes against https://fullscreen.spec.whatwg.org/#user-agent-level-style-sheet-defaults.

Also, how does this even work when the video has a relatively-positioned ancestor?

ferjm · 2019-08-29T16:52:05Z

r? @emilio

emilio

So I'm not a fan of this because this is just working around #19771.

If we were to do this, I'd rather either:

Rename the second argument of set_adjusted_display to something more meaningful, or...
(preferrably) move this hack to construct.rs, that is:
- Remove the second argument of set_adjusted_display.
- Make the munged_display stuff conditional on !self.style.is_top_layer() && parent.is_flex_container() && !element.is_root(). Or such.

emilio · 2019-08-29T21:57:32Z

components/style/style_adjuster.rs

@@ -219,9 +220,10 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> {
        let display = self.style.get_box().clone_display();
        let blockified_display = display.equivalent_block_display(is_root);
        if display != blockified_display {
+            let is_fullscreen = self.style.get_box()._servo_top_layer == InTopLayer::Top;


I think this could be self.style.in_top_layer(), fwiw.

ferjm · 2019-09-03T19:45:19Z

@bors-servo try=wpt


        Auto merge of #24034 - ferjm:video.fullscreen, r=<try>

Make inline elements work in fullscreen mode - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #22358  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24034)

bors-servo · 2019-09-03T19:45:25Z

⌛ Trying commit d6d7500 with merge d0a2908...

ferjm · 2019-09-03T19:45:34Z

r? @emilio

emilio

This makes much more sense.

Adding a test for this would be pretty nice. The test-case in #19771 should be trivially convertible into a reftest, test without display: block, reftest with it, and same for inline-block below.

Another request: Please double-check that the element actually remains fixed to the viewport, file a followup if not?

And a final one: Please file a followup to cleanup / potentially remove the is_item_or_root stuff?

r=me with that and the comment addressed. Thank you!

components/layout/construct.rs

bors-servo · 2019-09-03T20:32:11Z

💔 Test failed - linux-rel-wpt

ferjm · 2019-09-05T10:05:41Z

@bors-servo try=wpt

bors-servo · 2019-09-05T10:05:46Z

⌛ Trying commit bdbb439 with merge 90e8362...


        Auto merge of #24034 - ferjm:video.fullscreen, r=<try>

Make inline elements work in fullscreen mode - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #22358  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24034)

bors-servo · 2019-09-05T10:35:12Z

💔 Test failed - linux-rel-css


        Make inline elements work in fullscreen mode

ferjm · 2019-09-06T08:51:43Z

@bors-servo r=emilio

bors-servo · 2019-09-06T08:51:43Z

📌 Commit aa0c055 has been approved by emilio

bors-servo · 2019-09-06T09:27:04Z

⌛ Testing commit aa0c055 with merge e852d02...


        Auto merge of #24034 - ferjm:video.fullscreen, r=emilio

Make inline elements work in fullscreen mode - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #22358  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24034)

bors-servo · 2019-09-06T11:36:22Z

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: emilio
Pushing e852d02 to master...

highfive assigned emilio Aug 22, 2019

highfive added the S-awaiting-review label Aug 22, 2019

emilio requested changes Aug 22, 2019

View changes

ferjm changed the title ~~Make video elements work in fullscreen mode~~ Make inline elements work in fullscreen mode Aug 29, 2019

ferjm force-pushed the ferjm:video.fullscreen branch from 90e0fa2 to 513ade6 Aug 29, 2019

ferjm mentioned this pull request Aug 29, 2019

Fullscreen support for <video> elements #22358

Closed

emilio requested changes Aug 29, 2019

View changes

ferjm force-pushed the ferjm:video.fullscreen branch from 513ade6 to d6d7500 Sep 3, 2019

emilio approved these changes Sep 3, 2019

View changes

components/layout/construct.rs Show resolved Hide resolved

highfive added the S-tests-failed label Sep 3, 2019

ferjm force-pushed the ferjm:video.fullscreen branch from d6d7500 to 0883fd0 Sep 4, 2019

highfive removed the S-tests-failed label Sep 4, 2019

highfive added the S-tests-failed label Sep 5, 2019

ferjm force-pushed the ferjm:video.fullscreen branch from bdbb439 to bb7517c Sep 5, 2019

highfive removed the S-tests-failed label Sep 5, 2019

ferjm force-pushed the ferjm:video.fullscreen branch from bb7517c to e53ddb5 Sep 5, 2019

Make inline elements work in fullscreen mode

Loading status checks…

aa0c055

ferjm force-pushed the ferjm:video.fullscreen branch from e53ddb5 to aa0c055 Sep 5, 2019

ferjm mentioned this pull request Sep 5, 2019

Blockified inlines not rendered correctly. #19771

Open

highfive added S-awaiting-merge and removed S-awaiting-review labels Sep 6, 2019

bors-servo merged commit aa0c055 into servo:master Sep 6, 2019
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details

Taskcluster (pull_request) TaskGroup: success
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Sep 6, 2019

ferjm deleted the ferjm:video.fullscreen branch Sep 6, 2019

servo / servo

Make inline elements work in fullscreen mode #24034

Make inline elements work in fullscreen mode #24034

ferjm commented Aug 22, 2019 •

edited by SimonSapin

ferjm commented Aug 22, 2019 •

edited

emilio left a comment

ferjm commented Aug 29, 2019

emilio left a comment

This comment has been minimized.

ferjm commented Sep 3, 2019

bors-servo commented Sep 3, 2019

ferjm commented Sep 3, 2019

emilio left a comment

bors-servo commented Sep 3, 2019

ferjm commented Sep 5, 2019

bors-servo commented Sep 5, 2019

bors-servo commented Sep 5, 2019

ferjm commented Sep 6, 2019

bors-servo commented Sep 6, 2019

bors-servo commented Sep 6, 2019

bors-servo commented Sep 6, 2019

servo / servo

Join GitHub today

Make inline elements work in fullscreen mode #24034

Make inline elements work in fullscreen mode #24034

Conversation

ferjm commented Aug 22, 2019 • edited by SimonSapin

ferjm commented Aug 22, 2019 • edited

emilio left a comment

ferjm commented Aug 29, 2019

emilio left a comment

This comment has been minimized.

emilio Aug 29, 2019

ferjm commented Sep 3, 2019

bors-servo commented Sep 3, 2019

ferjm commented Sep 3, 2019

emilio left a comment

bors-servo commented Sep 3, 2019

ferjm commented Sep 5, 2019

bors-servo commented Sep 5, 2019

bors-servo commented Sep 5, 2019

ferjm commented Sep 6, 2019

bors-servo commented Sep 6, 2019

bors-servo commented Sep 6, 2019

bors-servo commented Sep 6, 2019

ferjm commented Aug 22, 2019 •

edited by SimonSapin

ferjm commented Aug 22, 2019 •

edited