Skip to content
This repository has been archived by the owner on Jul 18, 2022. It is now read-only.

Add external hooks to now {visible,hidden} algorithms #47

Merged
merged 1 commit into from
Jun 26, 2019

Conversation

yoavweiss
Copy link
Contributor

@yoavweiss yoavweiss commented Jun 25, 2019

Fixes #40


Preview | Diff

@yoavweiss
Copy link
Contributor Author

/cc @mounirlamouri

@mounirlamouri
Copy link
Member

Thanks @yoavweiss :)

@toddreifsteck
Copy link
Member

First.. THANKS to Yoav for writing this first draft. Now.. read the following to understand why I haven't yet made progress. I'm curious on the thinking around these and how we landed on this PR.
@mounirlamouri

This update seems to allow a synchronous hook. This does solve the request.. but.. I'm not certain it is the proper design on first glance without understanding the features we are adding it for. How are they implemented? Do we have a place in a spec intends to reference this hook to help us reason about whether this is a synchronous execution hook OR should be a hook to start our processing model OR should we triggered an enqueued task for other subscribers?

Copy link
Member

@toddreifsteck toddreifsteck left a comment

Choose a reason for hiding this comment

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

Please reply to comments and tag me. :-)

@yoavweiss
Copy link
Contributor Author

@toddreifsteck - The main user for this change is the Screen Orientation change algorithm, which seems to be using the sync hook to register async actions. Maybe we can trigger the hook async to begin with?

@mounirlamouri - thoughts?

@mounirlamouri
Copy link
Member

Running the hooks sync has the benefit of allowing sync hooks if needed and in practice avoid issues of re-entrency.

@toddreifsteck
Copy link
Member

Now that I've read the consumer specification, I think the Screen orientation spec could be improved by altering the animation frame tasks to be a step during render steps that checks visibility and executes at that point. As written, if a state change from visible to not visible occurs, that will cause 2 sets of steps to run which seems suboptimal. If re-written that way, I don't believe a new hook is needed in Page Visibility.

@mounirlamouri What do you think?

@toddreifsteck
Copy link
Member

Said another way.. each animation step needs to check 2 things: 1. Did the orientation change from last time? If so, fire that subset of steps. 2. Did the tracked state of visibility change from false to true? If so, run the visibility transition from false to true set of steps. (If the visibility flipped from false to true to false within a frame before the animation steps, I don't think we want the animation steps to execute the transition.)

Does what I'm saying make sense?

@mounirlamouri
Copy link
Member

I don't think we should optimise the number of steps. Hooking into the animation steps would have us run a check on every frame which I doubt any UA would implement. Unless I misunderstood?

@toddreifsteck
Copy link
Member

K, understood. I'm signing off and merging this. I feel it would be "cleaner" to add a call from Page Visibility to invoke Screen Orientation but I don't have a reason to block the "register/execute" spec design. :-)

I've also added an issue to Screen Orientation to track what seems to be a processing model issue. w3c/screen-orientation#179 Thanks!

@rakuco
Copy link
Member

rakuco commented May 25, 2021

Were these definitions intentionally not exported with data-export? I can't seem to be able to reference them in other specs.

@marcoscaceres
Copy link
Member

The latest published version on TR has them as exported, but they have not been picked up yet by WebRef for some reason.

I'll see if I can find out why.

@marcoscaceres
Copy link
Member

@rakuco, filed https://github.com/w3c/respec-web-services/issues/200 ... will try to fix it there.

rakuco added a commit to rakuco/page-visibility that referenced this pull request Jun 4, 2021
This was added in w3c#47 but not exported. Commit 730b632 ("chore: export
'external now visible algorithm'") exported the "external now visible
algorithm" definition but left this one out.
@rakuco
Copy link
Member

rakuco commented Jun 4, 2021

Were these definitions intentionally not exported with data-export? I can't seem to be able to reference them in other specs.

For future reference: Marcos exported one of the definitions in 730b632, and I'm fixing the other in #72.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a clear way to hook into "document becomes visible/hidden"
5 participants