Skip to content

Commit

Permalink
Rewrite orientation change and visibility handling.
Browse files Browse the repository at this point in the history
  • Loading branch information
@mounirlamouri
mounirlamouri committed Sep 10, 2014
1 parent 0175390 commit b9737da
Showing 1 changed file with 71 additions and 42 deletions.
113 changes: 71 additions & 42 deletions index.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ <h2>
SHOULD be initialized when the <a>document</a> is created, otherwise
they MUST be initialized the first time they are accessed and before
their value is read. The <a>user agent</a> MUST <a>update the
orientation information</a> to initialize them.
orientation information</a> of the <a>document</a> to initialize them.
</p>
<p>
For a given <a>document</a>, the <a>current orientation type</a> and
Expand Down Expand Up @@ -519,24 +519,10 @@ <h2>
<code>undefined</code> and set <var>pending-promise</var> to
<code>null</code>.
</li>
<li>Otherwise, the next time the orientation changes, if the new
<a>current orientation type</a> is included in
<var>orientations</var>, run the following sub-steps:
<ol>
<li>Update the <a>current orientation type</a>.
</li>
<li>Update the <a>current orientation angle</a>.
</li>
<li>Resolve <var>pending-promise</var> with
<code>undefined</code>.
</li>
<li>
<a>Fire a simple event</a> named <code>change</code> at
each <a href=
"#widl-Screen-orientation"><code>screen.orientation</code></a>
object.
</li>
</ol>
<li>Otherwise, the next time the orientation changes, a <code>
change</code> event will be fired and the <var>pending-promise
</var> resolved as described in <a href=
'#handling-screen-orientation-changes'>4.4</a>.

This comment has been minimized.

Sign in to view

Copy link
@marcoscaceres

marcoscaceres Sep 11, 2014

Please don't link to section numbers. That always ends badly.

I don't like the way this is written ("Otherwise, the next time the orientation changes, ... "). If system initiated orientation changes already trigger all this behavior, then we don't need this text at all. If this text has to be here, then it need to be more clearly wired up with the rest of the machinery (i.e., using the task queue or whatever). Right now, it feels really "hand-wavy" to me.

This comment has been minimized.

Sign in to view

Copy link
@mounirlamouri

mounirlamouri Sep 11, 2014

Author Owner

This should not be normative. I can add a note maybe? The idea is that the rest of the algorithm will happen in whatever happens in 4.4: resolving the promise, etc. I don't want to write the algorithm of 4.4 a second time...

This comment has been minimized.

Sign in to view

Copy link
@mounirlamouri

mounirlamouri Sep 11, 2014

Author Owner

I removed that "Otherwise", flattened the queue a task with the "if" and added a note.
</li>
</ol>
</li>
Expand Down Expand Up @@ -632,39 +618,82 @@ <h2>
Handling screen orientation changes
</h2>
<p>
Whenever the <a>current orientation type</a> and the <a>current
orientation angle</a> are meant to change for the current <a>browsing
context</a>, the <a>user agent</a> MUST run the following steps:
Whenever the viewport is drawn at a different angle compared to the
device's natural orientation, the <a>user agent</a> MUST run

This comment has been minimized.

Sign in to view

Copy link
@marcoscaceres

marcoscaceres Sep 11, 2014

"natural orientation" should be linked to its definition.
the following steps:
</p>
<ol>
<li>If the <a>browsing context</a> <a>active document</a> is not
visible per [[!PAGE-VISIBILITY]], abort these steps.
<li>Let <var>browsing contexts</var> be the <a>list of the descendant
browsing contexts</a> of the <a>top-level browsing context</a>'s
<a>document</a>.
</li>
<li>
<a>Queue a task</a> to perform the following actions:
<li>For every <a>browsing context</a> in <var>browsing contexts</var>,

This comment has been minimized.

Sign in to view

Copy link
@marcoscaceres

marcoscaceres Sep 11, 2014

Should be: For each <var>browsing context</var> (browsing context is used as variable here when iterating)

This comment has been minimized.

Sign in to view

Copy link
@mounirlamouri

mounirlamouri Sep 11, 2014

Author Owner

Done.
run the following sub-steps:
</li>
<ol>
<li>Let <var>doc</var> be the <a>browsing context</a>'s <a>active
document</a>.
</li>
<li>If <var>doc</var> is not visible per [[!PAGE-VISIBILITY]], abort
these steps.
</li>
<li><a title="update the orientation information">Update the

This comment has been minimized.

Sign in to view

Copy link
@marcoscaceres

marcoscaceres Sep 11, 2014

Nit: you don't need the title attribute. Should link fine without it.

This comment has been minimized.

Sign in to view

Copy link
@mounirlamouri

mounirlamouri Sep 11, 2014

Author Owner

Done.
orientation information</a> of <var>doc</var>.
</li>
<li>If <var>doc</var>'s <a>pending promise</a> is not <code>
null</code>:
<ol>
<li>Resolve <var>doc</var>'s <a>pending promise</a> with <code>
undefined</code>.</li>
<li>Set <var>doc</var>'s <a>pending promise</a> to <code>null
</code>.
</li>
</ol>
</li>
<li>
<a>Fire a simple event</a> named <code>change</code> at <var>doc
</var>'s <a href="#widl-Screen-orientation"><code>
screen.orientation</code></a> object.
</li>
</ol>
</ol>

This comment has been minimized.

Sign in to view

Copy link
@marcoscaceres

marcoscaceres Sep 11, 2014

Nit: whitespace

This comment has been minimized.

Sign in to view

Copy link
@mounirlamouri

mounirlamouri Sep 11, 2014

Author Owner

Let's keep that.
<p>
Whenever a <a>document</a> becomes visible per [[!PAGE-VISIBILITY]],
when the <code>visibilitychange</code> has been received and that the
new <code>visibilityState</code> is <code>visible</code>, the <a>user
agent</a> MUST run the following steps:

This comment has been minimized.

Sign in to view

Copy link
@marcoscaceres

marcoscaceres Sep 11, 2014

Wait? Are you doing this at the turn of the event loop?

BTW: This is a bug in the [!PAGE-VISIBILITY]] spec - it doesn't define a task source. I would expect that what you specify would happen before scripts run again at the turn of the event loop. Right now, it reads like this code runs after the visibility event fires and the visibilityState state is updated in JS. This would cause a race condition, obviously.

This comment has been minimized.

Sign in to view

Copy link
@mounirlamouri

mounirlamouri Sep 11, 2014

Author Owner

I'm not sure what you mean here. The visibilityState is updated before the event fires.

This comment has been minimized.

Sign in to view

Copy link
@marcoscaceres

marcoscaceres Sep 11, 2014

I'm just saying we need a hook for this in page visibility. So you can inject all this stuff there right before JavaScript starts executing again.
</p>
<ol>
<li>Let <var>type</var> and <var>angle</var> be respectively the <a>
document</a>'s <a>current orientation type</a> and <a>current
orientation angle</a>.
</li>
<li><a title="update the orientation information">Update the

This comment has been minimized.

Sign in to view

Copy link
@marcoscaceres

marcoscaceres Sep 11, 2014

nit: don't need title here either.

This comment has been minimized.

Sign in to view

Copy link
@mounirlamouri

mounirlamouri Sep 11, 2014

Author Owner

Done.
orientation information</a> of the <a>document</a>.
</li>
<li>If <var>type</var> is different from the <a>document</a>'s <a>
current orientation type</a> or <var>angle</var> from the <a>document
</a>'s <a>current orientation angle</a>, run the following sub-steps:
<ol>
<li>Update the <a>current orientation type</a>.
</li>
<li>Update the <a>current orientation angle</a>.
</li>
<li>
<a>Fire a simple event</a> named <code>change</code> at each
<a href=
"#widl-Screen-orientation"><code>screen.orientation</code></a>
object.
</li>
<li>If <a>pending promise</a> is not <code>null</code>:
<li>If the <a>document</a>'s <a>pending promise</a> is not <code>
null</code>:
<ol>
<li>Resolve <a>pending promise</a> with
<code>undefined</code>.
<li>Resolve the <a>document</a>'s <a>pending promise</a> with
<code>undefined</code>.</li>
<li>Set the <a>document</a>'s <a>pending promise</a> to <code>
null</code>.
</li>
<li>Set <a>pending promise</a> to <code>null</code>.
</li>
</ol>
</li>
<li>
<a>Fire a simple event</a> named <code>change</code> at the <a>
document</a>'s <a href="#widl-Screen-orientation"><code>
screen.orientation</code></a> object.
</li>
</ol>
</li>
</ol>

<div class="note">
<p>
Developers need to be aware that a <a href=
Expand Down

1 comment on commit b9737da

@marcoscaceres
Copy link

Choose a reason for hiding this comment

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

looks good

Please sign in to comment.