Add support for fullscreen #10102 #13489

farodin91 · 2016-09-28T21:15:48Z

I'm start working on fullscreen support.
@jdm Should be the entry_point in ScriptReflow a Option if fullscreen is enabled or point on the entry_node? For example the RootNode.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #10102 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2016-09-28T21:15:52Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/webidls/Document.webidl, components/script/dom/document.rs, components/script/dom/webidls/Element.webidl, components/script_layout_interface/message.rs, components/script/dom/element.rs

highfive · 2016-09-28T21:15:53Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

jdm · 2016-09-28T21:18:25Z

I suspect an Option makes the most sense; I expect we still want to reflow the rest of the page even if an element is fullscreen.

emilio · 2016-09-29T10:46:27Z

Do we plan to land this without implementing the per-platform bits? (Not a huge deal, just curious).

paulrouget · 2016-09-29T11:52:05Z

Is the plan to make the DOM element full window, and than send a message to the glutin window to go fullscreen?

jdm · 2016-09-29T11:53:31Z

Yep.

farodin91 · 2016-10-02T16:18:36Z

@jdm For which purpose exists target_element ?
https://github.com/servo/servo/blob/master/components/script/dom/document.rs#L1966

jdm · 2016-10-02T16:20:42Z

@farodin91 https://html.spec.whatwg.org/multipage/browsers.html#target-element

farodin91 · 2016-10-02T16:21:39Z

Thank you

farodin91 · 2016-10-02T17:04:31Z

@jdm How to set a Pseudo-Class (:fullscreen)?

jdm · 2016-10-02T17:38:42Z

I recommend looking at how the :target pseudoclass is implemented (look at target_element).

jdm

Good start!

jdm · 2016-10-04T20:38:52Z

components/script/dom/window.rs

@@ -1137,13 +1138,23 @@ impl Window {
        let document = self.Document();
        let stylesheets_changed = document.get_and_reset_stylesheets_changed_since_reflow();

+        // fullscreen actions
+        let entry_node = match self.Document().GetFullscreenElement() {


let entry_node = self.Document() .GetFullScreenElement() .map(|e| e.upcast::<Node>().to_trusted_node_address());

jdm · 2016-10-04T20:38:52Z

components/script/dom/window.rs

@@ -1137,13 +1138,23 @@ impl Window {
        let document = self.Document();
        let stylesheets_changed = document.get_and_reset_stylesheets_changed_since_reflow();

+        // fullscreen actions


I don't think this comment adds anything to the code.

jdm · 2016-10-04T20:38:52Z

components/script_layout_interface/message.rs

@@ -121,6 +121,8 @@ pub struct ScriptReflow {
    pub reflow_info: Reflow,
    /// The document node.
    pub document: TrustedNodeAddress,
+    /// The render entry point need for fullscreen.


s/need/needed/

jdm · 2016-10-04T20:38:52Z

components/script/dom/element.rs

@@ -2108,6 +2111,13 @@ impl ElementMethods for Element {
            None => return Err(Error::NotSupported)
        }
    }
+
+    #[allow(unrooted_must_root)]
+    fn RequestFullscreen(&self) -> Rc<Promise> {


There's a missing specification link comment.

jdm · 2016-10-04T20:38:52Z

components/script/dom/element.rs

+    fn RequestFullscreen(&self) -> Rc<Promise> {
+        let doc = document_from_node(self);
+        doc.set_fullscreen_element(Some(self));
+        Promise::new(self.global().r())


This promise needs to be resolved when the fullscreen status has changed. We can probably use this instead:

let global = self.global().r(); Promise::Resolve(global, global.get_cx(), HandleValue::undefined())

jdm · 2016-10-04T20:38:52Z

components/script/dom/document.rs

@@ -2951,6 +2966,28 @@ impl DocumentMethods for Document {

    // https://html.spec.whatwg.org/multipage/#documentandelementeventhandlers
    document_and_element_event_handlers!();
+
+    event_handler!(fullscreenerror, GetOnfullscreenerror, SetOnfullscreenerror);


Specification links are missing for these changes.

jdm · 2016-10-04T20:38:52Z

components/script/dom/document.rs

+    #[allow(unrooted_must_root)]
+    fn ExitFullscreen(&self) -> Rc<Promise> {
+        self.set_fullscreen_element(None);
+        Promise::new(self.global().r())


Same promise comment. set_fullscreen_element should probably return the Promise value so we don't duplicate the code.

bors-servo · 2016-10-07T14:10:17Z

☔ The latest upstream changes (presumably #13596) made this pull request unmergeable. Please resolve the merge conflicts.

farodin91 · 2016-10-12T18:16:11Z

@jdm i read the spec a bit more, due to spec i shouldn't set a new entry in reflow.
https://fullscreen.spec.whatwg.org/#user-agent-level-style-sheet-defaults. I think this would be better.
Any preference?

jdm · 2016-10-12T18:38:34Z

Oh, that's interesting indeed. If we can do it entirely with CSS that would definitely be preferable.

jdm

Sorry, I noticed one last problem :( I've filed #14500 about making this impossible in the future.

jdm · 2016-12-08T20:20:03Z

components/script/dom/bindings/refcounted.rs

@@ -64,6 +64,7 @@ impl TrustedReference {
 /// in asynchronous operations. The underlying DOM object is guaranteed to live at least
 /// as long as the last outstanding `TrustedPromise` instance. These values cannot be cloned,
 /// only created from existing Rc<Promise> values.
+#[derive(Clone)]


This is not safe. We need to make a new TrustedPromise value from the Rc<Promise> value instead.

jdm · 2016-12-08T20:20:03Z

components/script/dom/element.rs

+        let document = document_from_node(element.r());
+        // JSAutoCompartment needs to be manually made.
+        // Otherwise, Servo will crash.
+        let promise = self.promise.clone().root();


Why we can't use self.promise.root() here?

I don't like my new solution.

bors-servo · 2016-12-08T21:30:40Z

☔ The latest upstream changes (presumably #14495) made this pull request unmergeable. Please resolve the merge conflicts.


        Add support for fullscreen #10102

jdm · 2016-12-09T17:52:31Z

@bors-servo: r+

bors-servo · 2016-12-09T17:52:32Z

📌 Commit 55f0e56 has been approved by jdm

bors-servo · 2016-12-09T17:52:35Z

⌛ Testing commit 55f0e56 with merge 8b69e73...


        Auto merge of #13489 - farodin91:fullscreen, r=jdm

@jdm

Add support for fullscreen #10102  I'm start working on fullscreen support. @jdm Should be the entry_point in ScriptReflow a Option if fullscreen is enabled or point on the entry_node? For example the RootNode. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #10102 (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13489)

bors-servo · 2016-12-09T18:40:45Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev

foolip · 2017-02-09T19:07:40Z

Could @farodin91 or someone else take a look at web-platform-tests/wpt#4394 and my comment in 8b69e73?

highfive assigned jdm Sep 28, 2016

highfive added the S-awaiting-review label Sep 28, 2016

jdm added S-needs-code-changes and removed S-awaiting-review labels Sep 28, 2016

farodin91 force-pushed the farodin91:fullscreen branch from a40a7bd to 9db3134 Oct 2, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Oct 2, 2016

jdm requested changes Oct 4, 2016

View changes

jdm added S-needs-code-changes S-fails-tidy and removed S-awaiting-review labels Oct 4, 2016

highfive added the S-needs-rebase label Oct 7, 2016

farodin91 force-pushed the farodin91:fullscreen branch from 9db3134 to c4a1849 Oct 8, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Oct 8, 2016

farodin91 force-pushed the farodin91:fullscreen branch from c4a1849 to 7aa9a93 Oct 8, 2016

jdm removed S-awaiting-review S-needs-rebase labels Nov 28, 2016

farodin91 force-pushed the farodin91:fullscreen branch from c4ab9fa to 574addd Dec 8, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Dec 8, 2016

jdm requested changes Dec 8, 2016

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Dec 8, 2016

highfive added the S-needs-rebase label Dec 8, 2016

farodin91 force-pushed the farodin91:fullscreen branch from 574addd to fe12ab7 Dec 9, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Dec 9, 2016

Add support for fullscreen #10102

Loading status checks…

55f0e56

farodin91 force-pushed the farodin91:fullscreen branch from fe12ab7 to 55f0e56 Dec 9, 2016

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Dec 9, 2016

bors-servo merged commit 55f0e56 into servo:master Dec 9, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Dec 9, 2016

This was referenced Dec 9, 2016

Bump rust-openssl, add support for OpenSSL 1.1.0 [WIP] #14484

Closed

Expose Quirks Mode information in the layout data and code #14430

Merged

Implement discarding Document objects to reclaim space. #14312

Merged

jdm mentioned this pull request Feb 9, 2017

Revert changes to document-onfullscreenerror.html web-platform-tests/wpt#4778

Merged

servo / servo

Add support for fullscreen #10102 #13489

Add support for fullscreen #10102 #13489

farodin91 commented Sep 28, 2016 •

edited

highfive commented Sep 28, 2016

highfive commented Sep 28, 2016

jdm commented Sep 28, 2016

emilio commented Sep 29, 2016

paulrouget commented Sep 29, 2016

jdm commented Sep 29, 2016

farodin91 commented Oct 2, 2016 •

edited

jdm commented Oct 2, 2016

farodin91 commented Oct 2, 2016

farodin91 commented Oct 2, 2016

jdm commented Oct 2, 2016

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Oct 7, 2016

farodin91 commented Oct 12, 2016

jdm commented Oct 12, 2016

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Dec 8, 2016

jdm commented Dec 9, 2016

bors-servo commented Dec 9, 2016

bors-servo commented Dec 9, 2016

bors-servo commented Dec 9, 2016

foolip commented Feb 9, 2017

servo / servo

Join GitHub today

Add support for fullscreen #10102 #13489

Add support for fullscreen #10102 #13489

Conversation

farodin91 commented Sep 28, 2016 • edited

highfive commented Sep 28, 2016

highfive commented Sep 28, 2016

jdm commented Sep 28, 2016

emilio commented Sep 29, 2016

paulrouget commented Sep 29, 2016

jdm commented Sep 29, 2016

farodin91 commented Oct 2, 2016 • edited

jdm commented Oct 2, 2016

farodin91 commented Oct 2, 2016

farodin91 commented Oct 2, 2016

jdm commented Oct 2, 2016

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Oct 7, 2016

farodin91 commented Oct 12, 2016

jdm commented Oct 12, 2016

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Dec 8, 2016

jdm commented Dec 9, 2016

bors-servo commented Dec 9, 2016

bors-servo commented Dec 9, 2016

bors-servo commented Dec 9, 2016

foolip commented Feb 9, 2017

farodin91 commented Sep 28, 2016 •

edited

farodin91 commented Oct 2, 2016 •

edited