Resolving the promise after the change event is fired

[Johanna-hub]

Closes #120

The following tasks have been completed:

• Confirmed there are no ReSpec/BikeShed errors or warnings.
• Modified Web platform tests Add screen orientation test: fire event before promise is resolved web-platform-tests/wpt#15267

Implementation commitment:

• WebKit
• Chromium
• Gecko

---

Preview | Diff

[marcoscaceres]

Looks good. I think the gh-pages branch itself needs a tidy tho. I see tidy changes that should be on the gh-pages branch.

[marcoscaceres]

@kenchris how do propose we handle this on the Chrome side? This is a breaking change. I think it’s worth doing, but we will need to convince folks (hi @mounirlamouri! 🤗)

[mounirlamouri]

Is there a recommended pattern by the TAG or commonly used one from other specifications?

[kenchris]

Let me talk with @slightlyoff

[kenchris]

Please have a look at https://www.w3.org/2001/tag/doc/promises-guide

[marcoscaceres]

@mounirlamouri wrote

Is there a recommended pattern by the TAG or commonly used one from other specifications?

I was actually thinking of filing a bug on the TAG as this particular situation is not exactly covered by either the API Design Guide or the Promises Guide.

Regardless, the "pending promise" effectively represents the screen having changed:

const pendingPromise = new Promise(r => {
  screen.orientation.addEventListener("change", ()=> r(), { once: true });
});

[kenchris]

I was actually thinking of filing a bug on the TAG as this particular situation is not exactly covered by either the API Design Guide or the Promises Guide.

Yes, I noticed the same. Please file

[marcoscaceres]

@kenchris, the closest I get in the Promises Guide is this section:
https://www.w3.org/2001/tag/doc/promises-guide#one-time-events

The prototypical example of such an "event" is a loaded indicator: a resource such as an image, font, or even document, could provide a loaded property that is a promise that becomes fulfilled only when the resource has fully loaded (or becomes rejected if there’s an error loading the resource).

The above suggests that the event occurs, and, as a result the promise is resolved and rejected based on what the result is.

Then, authors can always queue up actions to be executed once the resource is ready by doing resource.loaded.then(onLoaded, onFailure).

The above follows...

This will work even if the resource was loaded already, queueing a microtask to execute onLoaded. This is in contrast to an event model, where if the author is not subscribed at the time the event fires, that information is lost.

... and so on...

Anyway, will file a bug :)

[marcoscaceres]

Filed https://github.com/w3ctag/promises-guide/issues/57

[marcoscaceres]

@mounirlamouri, the TAG has kindly updated their API Design doc providing advice around this - thanks @kenchris for making that happen!

Given the TAG's advice, would you be ok with changing the behavior? @Johanna-hub and I can update the implement on the Gecko side, and provide WPTests.

[marcoscaceres]

LGTM

[Johanna-hub]

We now have a test for this web-platform-tests/wpt#15267
@kenchris and anyone else interested, would be great if you could take a look and let us know any comments?

I have also filed a bug on bugzilla https://bugzilla.mozilla.org/show_bug.cgi?id=1525582

[mounirlamouri]

LGTM. I don't know if and when I would update the implementation in Chromium. Though, @Johanna-hub if you want to send a patch, I would happily review :)

[marcoscaceres]

@mounirlamouri, sure, we can probably do it. Being new the Blink code base, if you can quickly just point us to the right files, we can go from there.

[mounirlamouri]

Actually, I realised that I had in my backlog removing all the Android Jelly Bean support from the screen orientation code. Will probably do both at the same time.

[marcoscaceres]

@mounirlamouri, ok cool. We will make sure to have good web platform tests for you at least :)

[marcoscaceres]

oh, before I forget, do you want us to file a bug for this specifically on the Chrome side? Or do you have a link to your refactor bug?

[mounirlamouri]

I've updated the initial message with the bug number. I have a CL ready that seems to work. Should be in soon.

[mounirlamouri]

The change has landed in Blink. Hopefully it will stick. Subtle timing changes like this always worry me :)

[marcoscaceres]

Ok, excellent. I’ll make sure the changes happen on the Mozilla side ASAP. Then our implementations can be in sync.

[marcoscaceres]

Ok, let's go with this. We need to fix this up to use task queues, however.

[marcoscaceres]

Working on the Gecko patch.

[marcoscaceres]

Ok, landed the changes in Gecko too. Fingers crossed it doesn't get backed out.