Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Change algorithm of "collect documents to unfullscreen" #72

Merged
merged 3 commits into from
Feb 24, 2017
Merged

Change algorithm of "collect documents to unfullscreen" #72

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0c61017
Add new concept "simple fullscreen document"
upsuper Jan 6, 2017
3941295
Change collect documents to unfullscreen algorithm
upsuper Jan 6, 2017
d8802c3
Fix my own nits
foolip Feb 24, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 21 additions & 15 deletions fullscreen.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,18 +295,28 @@ attribute's getter must run these steps:
<li><p>Return null.
</ol>

<p>A <a>document</a> is said to be a <dfn>simple fullscreen document</dfn> if its <a>top layer</a>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do we think about documents with a single fullscreen element in the top layer but a dialog above or beneath it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should still be a simple fullscreen document, since only one fullscreen element is in the top layer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, so I would have read this definition as requiring that there is exactly one element in the top layer, and that it has the fullscreen flag set. Perhaps rephrase as "A document is said to be a simple fullscreen document if there is exactly one element in its top layer with the fullscreen flag set" and write a note saying that "In other words, a document with two elements in its top layer, where just one has the fullscreen flag set, is a simple fullscreen document"?

consists of a single <a>element</a> that has its <a>fullscreen flag</a> set.

<p>To <dfn>collect documents to unfullscreen</dfn> given <var>doc</var>, run these steps:

<ol>
<li><p>If <var>doc</var>'s <a>top layer</a> consists of more than a single <a>element</a> that has
its <a>fullscreen flag</a> set, return the empty set.

<li><p>Let <var>docs</var> be an ordered set consisting of <var>doc</var>.

<li><p>While <var>docs</var>'s last <a for=/>document</a> has a <a>browsing context container</a>
whose <a>node document</a>'s <a>top layer</a> consists of a single <a>element</a> that has its
<a>fullscreen flag</a> set and does not have its <a>iframe fullscreen flag</a> set (if any), append
that <a>node document</a> to <var>docs</var>.
<li><p>Run these substeps until terminated:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a while(true) loop? @annevk, any advice from https://infra.spec.whatwg.org/#algorithm-control-flow on how this should be phrased? I think this could plausibly be read as just running once through if not terminated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I might just say "While true:". Perhaps file an issue against Infra to introduce more loop constructs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"While true:" WFM, and I've filed whatwg/infra#66


<ol>
<li><p>Let <var>lastDoc</var> be <var>docs</var>'s last <a for=/>document</a>.

<li><p>If <var>lastDoc</var> is not a <a>simple fullscreen document</a>, terminate these substeps.

<li><p>Let <var>container</var> be <var>lastDoc</var>'s <a>browsing context container</a>
if any, and terminate these substeps otherwise.

<li><p>If <var>container</var>'s <a>iframe fullscreen flag</a> is set, terminate these substeps.

<li><p>Append <var>container</var>'s <a>node document</a> to <var>docs</var>.
</ol>

<li><p>Return <var>docs</var>.
</ol>
Expand All @@ -331,7 +341,8 @@ attribute's getter must run these steps:
<a>active document</a>.
<!-- cross-process -->

<li><p>If <var>topLevelDoc</var> is in <var>docs</var>, then set <var>resize</var> to true.
<li><p>If <var>topLevelDoc</var> is in <var>docs</var>, and it is a
<a>simple fullscreen document</a>, then set <var>resize</var> to true.

<li><p>Return <var>promise</var>, and run the remaining steps <a>in parallel</a>.

Expand All @@ -348,15 +359,10 @@ attribute's getter must run these steps:
<var>doc</var>.
<!-- cross-process -->

<li><p>If <var>resize</var> is true and <var>topLevelDoc</var> is not in <var>exitDocs</var>,
<li><p>If <var>resize</var> is true and <var>topLevelDoc</var> is either not in
<var>exitDocs</var>, or not a <a>simple fullscreen document</a>,
<a>fully exit fullscreen</a> <var>topLevelDoc</var>, reject <var>promise</var> with a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious if Gecko actually has this "fully exit fullscreen" step when exiting? In Blink, at least after the (now reverted) attempt to align with the spec, this didn't make sense. This is the topic of #65

I'm fine with this change if it's just incidental of course, and not the central point of this PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What we actually do in this case is that, after the window gets resized, we synchronously reset the whole document tree to non-fullscreen state regardless of whether the current state is identical to the previous state. Somehow it is simliar to this "fully exit fullscreen" step.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what Blink does as well. I think this change is fine, I'll try to bring it closer to implementation in #65 and ask for your review then.

<code>TypeError</code> exception, and terminate these steps.

<li><p>If <var>exitDocs</var> is the empty set, append <var>doc</var> to <var>exitDocs</var>.

<li><p>If <var>exitDocs</var>'s last <a for=/>document</a> has a
<a>browsing context container</a>, append that <a>browsing context container</a>'s
<a>node document</a> to <var>exitDocs</var>.
<!-- cross-process -->

<li><p>Let <var>descendantDocs</var> be an ordered set consisting of <var>doc</var>'s
Expand Down