@jimevans, I think it looks good. But I wanted to check one thing before I approve. I've noticed that input.fileDialogOpened was not updated. Was it intentional, or did you just miss it?

@lutien It was purely an oversight. I've fixed it in a subsequent commit. Thanks for catching it.

I’m wondering what the best way is to ship this to users. With userContext now being required, this becomes a breaking change, especially since browsers do not yet include it in the event payload. Clients would therefore need to be tolerant for the time being. The same concern applies to tests, which we cannot add immediately without causing a significant amount of breakage.

Do you consider this addition critical enough to justify that disruption, or should we take a more gradual approach? Or am I being overly cautious about landing this change?

@whimboo Since a client already has to be able to handle additional values in command responses and events (both are marked as Extensible), existing clients shouldn't see a negative impact. Where things fall down is that a client updates to be strictly compliant with the spec, and the browser implementation hasn't caught up yet. This doesn't seem to be a huge burden on clients, unless they're attempting to force strict spec compliance. As the author of such a client library, I have a strategy for handling this scenario, and I would expect other client authors to do the same.

From the WPT angle, it seems like this is exactly the kind of thing that those tests are intended to enforce, that implementations are compliant with the spec, so I'm fine with WPT test failures (I already have a PR for those), especially if they act as a forcing function for implementations to update themselves. But these are just my opinions, and they're not strongly held.

@jimevans in this case we extend the params of the individual events and for those I cannot see that Extensible is set. Or do I miss something? Here an example for navigation related events:

https://w3c.github.io/webdriver-bidi/#cddl-type-browsingcontextnavigationinfo

@whimboo if your concern is that with the merge all the implementations become invalid, we can mark CDDL property as an optional for a month, which would give us time to implement without breaking the protocol rolls, and make the field required afterwards. I don't think its that complicated and it should work. WDYT?

@whimboo if your concern is that with the merge all the implementations become invalid, we can mark CDDL property as an optional for a month, which would give us time to implement without breaking the protocol rolls, and make the field required afterwards. I don't think its that complicated and it should work. WDYT?

I think that would be a compromise, yes. Then browsers supporting events need to be updated first.

CC'ing @lauromoura regarding the Webkit implementation that we don't want to break as well.

@whimboo if your concern is that with the merge all the implementations become invalid, we can mark CDDL property as an optional for a month, which would give us time to implement without breaking the protocol rolls, and make the field required afterwards. I don't think its that complicated and it should work. WDYT?

I think that would be a compromise, yes. Then browsers supporting events need to be updated first.

CC'ing @lauromoura regarding the Webkit implementation that we don't want to break as well.

While we still have some open issues regarding proper user context support, the changes related to this PR should be fine. Thanks for the heads-up.

Thanks @lauromoura. In that case lets go ahead and add the userContexts as optional for now. @jimevans mind updating the PR accordingly? It would be good to get a new issue filed as well which is about turning them into required fields - something that we can do later this year.

@whimboo I've now made all of the additions of userContext properties optional for the moment. I will open an issue to make these mandatory for implementation at a later time.

On #1071 the userContext argument will be made mandatory.

@jimevans when updating the BiDi support sheet I noticed that we do not have the userContext field for the log.entryAdded event. I assume it's a miss? Also what about the various network, and the script.message and script.realmDestroyed (here we have no realmInfo) events?

@jimevans when updating the BiDi support sheet I noticed that we do not have the userContext field for the log.entryAdded event. I assume it's a miss? Also what about the various network, and the script.message and script.realmDestroyed (here we have no realmInfo) events?

log.entryAdded and script.message share https://www.w3.org/TR/webdriver-bidi/#cddl-type-scriptsource, which contains userContext. Network events share https://www.w3.org/TR/webdriver-bidi/#cddl-type-networkbaseparameters, which also contain userContext. So the only question is about script.realmDestroyed which indeed doesn't contain userContext, but also has no context field, so I think it's also harder to add.

Ah you are right. I missed to check those sub types. So yes, it looks all good in that sense and ignoring realmDestroyed for now might be fine even though realms should be destroyed before the context and as such should have a proper reference to both the context and the user context.

Thank you @jimevans! Would you mind updating the existing wpt tests to check for a proper user context if it is returned?

There is actually web-platform-tests/wpt#56744 for it.