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.

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

BREAKING CHANGE: unlock() returns a pormise #183

Closed
wants to merge 1 commit into from

Conversation

marcoscaceres
Copy link
Member

@marcoscaceres marcoscaceres commented Jul 23, 2019

Closes #104

The following tasks have been completed:

  • Confirmed there are no ReSpec/BikeShed errors or warnings.
  • Modified Web platform tests - Gecko implementation will update the tests

Implementation commitment:


Preview | Diff

@marcoscaceres
Copy link
Member Author

Ok, this is not exactly right... [[defaultOrientation]] is a list, but it should really just be a single value... even if the OS supports multiple.

@marcoscaceres
Copy link
Member Author

@mounirlamouri, could you check in Blink what you actually use for defaultOrientation when unlocking? From a quick search in the Blink code, it seems you also use a single value - but I wasn't able to find where it is defined (does Google have something like searchfox?). In Gecko, we use pass eScreenOrientation_None to the HAL.

We could basically do the same thing in the spec by adding "default" or "none" to the OrientationType enum. The API itself would never return the value, but then we could pass it internally to unlock() to represent the "default orientations".

@mounirlamouri
Copy link
Member

The thing is for all orientation locks, the UA can figure out whether we are already in an orientation that matches the lock or whether we should wait for a change. With unlock() or default, the UA would need to know what is the system lock (in most cases, when the manifest has an orientation, we should know). If we can implement this reliably on Android and iOS, I think it would be a fine addition to the spec but I'm not sure if we want to just return immediately.

@marcoscaceres
Copy link
Member Author

If we can implement this reliably on Android and iOS, I think it would be a fine addition to the spec but I'm not sure if we want to just return immediately.

Problem is that we get into situations outlined in the original bug, where screen orientation becomes racy with exitFullScreen() and potentially other things (see @kenchris's issue also). Even if the UA can't determine if the OS changed/unlocked orientation, it should be sufficient to know that everything possible was done by the UA to change the orientation: the task was queued, request was sent to the OS, etc.

@mounirlamouri
Copy link
Member

I would be okay to have unlock() return a promise but I think we should have the spec be clearer about the situation. The change as it is assumes that you can apply an orientation lock to the default orientotion which isn't possible actually. #150 kind of touch this issue. Unless we want to solve it first (maybe we do?), one way to resolve this is to simply make the unlock() steps async and have it return a promise when it's done so it would be clear that we are not attempting to notify that the unlock orientation change happened but that the call was registered by the system. A note in the spec making this clear would be good too.

@w3c w3c deleted a comment from weyyserabut Oct 4, 2021
@w3c w3c deleted a comment from weyyserabut Oct 4, 2021
@saschanaz saschanaz deleted the unlock_promise branch October 4, 2021 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Promise for unlock()?
2 participants