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

contextlost follow-up #7130

Open
annevk opened this issue Sep 29, 2021 · 14 comments
Open

contextlost follow-up #7130

annevk opened this issue Sep 29, 2021 · 14 comments
Labels
clarification Standard could be clearer topic: canvas

Comments

@annevk
Copy link
Member

annevk commented Sep 29, 2021

I briefly looked at #6797 and what might remain there:

  • There's a note below the OffscreenCanvas IDL that's no longer applicable:

    OffscreenCanvas is an EventTarget so that WebGL can fire webglcontextlost and webglcontextrestored events at it. [WEBGL]

  • I thought the idea was that we'd dispatch the WebGL events at the same time. Currently the WebGL events are not integrated to the level whereby they are dispatched from the rendering part of the event loop.

cc @yiyix @whatwg/canvas

@annevk annevk added clarification Standard could be clearer topic: canvas labels Sep 29, 2021
@yiyix
Copy link
Contributor

yiyix commented Oct 9, 2021

I think the note is still applicable. It depends the type of rendering context. if offscreenCanvas has a 2d rendering context, then contextlost event follows the spec I added. If the offscreenCanvas has a webgl context, it follows webgl spec.

@kenrussell: Could you provide your insights on the webgl parts? Thank you!

@annevk
Copy link
Member Author

annevk commented Oct 11, 2021

The problem with the note is that it has other reasons to be an EventTarget now. I suppose we could indicate that WebGL makes use of this as well. Should the WebGL events have on* attributes by the way?

@yiyix
Copy link
Contributor

yiyix commented Oct 19, 2021

Yes, you are right. It's not an EventTarget for webgl only now. I don't think the WebGL events have on* attributes. cc @kenrussell for confirmation.

Maybe we can rephrase the note to something like:
OffscreenCanvas is an EventTarget, so it can trigger context lost related events (ex: contextlost/contexrestored in CanvasContext2D and WebGL).

I can change the spec according this discussion.

@kenrussell
Copy link
Member

To reduce API noise, WebGL didn't add attributes like onContextLost to HTMLCanvasElement - the preference was to handle everything via addEventListener.

@domenic
Copy link
Member

domenic commented Oct 19, 2021

That's unfortunate---event handler IDL attributes are required per the TAG design principles. I hope we can fix this.

@kenrussell
Copy link
Member

Please feel free to file an issue on https://github.com/KhronosGroup/WebGL citing this issue and https://www.w3.org/TR/design-principles/#always-add-event-handlers ; all of the browser vendors meet regularly in the WebGL working group, so consensus can be gathered there.

@annevk
Copy link
Member Author

annevk commented Oct 20, 2021

Another thing that needs updating per blink-dev: these events should only be fired when the corresponding top-level browsing context has focus/visibility. This will make aligning with the WebGL events more important.

Filed KhronosGroup/WebGL#3349 on event handler attributes in WebGL.

@fserb
Copy link
Contributor

fserb commented Oct 20, 2021

So @annevk, it's a bit more nuanced than that. It's okey to spec that they need to be fired after the top-level browsing context has focus, but not "when". That's what I was referring to on the email.

If you force it to fire on visibility, it's a potential privacy risk, as different origins would be able to coordinate events. But I think specifying the mitigations for that may be a bit too much for the spec. WDYT?

The current implementation on Chrome does:

  • Fires Canvas events on document visibility if same-domain as navigation
  • Fires after a random delay after visibility on other domains.
  • Fires after a random delay on OffscreenCanvas

I think it's fine to add a note saying that an implementation needs to be careful about those coordinations. Maybe even spec that it needs to be after visibility (although I can think of situations in which we may want to restore before?). But spec'ing the full behavior sounds a bit too much?

Just be clear, I'm not against describing all behavior on spec.

@domenic
Copy link
Member

domenic commented Oct 20, 2021

I think the main question is whether interop problems could arise from a vague spec. To me that's unclear because I don't know the full implications of lost/restored context.

In particular I am wondering about the period of time between when the context is "actually" lost, and the contextlost event fires. Or, when the context is "actually" restored, and the contextrestored event fires.

Do these periods of time exist, in the Chromium implementation? Can web developers run code during them? If so, what is the observable behavior of the canvas during those periods? Is that something we need to spec?

For example, if during the period between actually-restored and contextrestored, web developers can draw on and read from the canvas, then: (a) I wonder if the privacy issue is still there, since web developers can just infer the timing that way? (b) we need to specify behaviors during this intermediate state, I think.

Or, if these periods of time do not exist, then there is no interop problem; we can just pretend that the context is being lost/restored at the same time contextlost/contextrestored is fired, which just so happens to be at a good privacy-preserving time. I find this unlikely for the contextlost case though...

@kdashg
Copy link

kdashg commented Oct 20, 2021

Firefox re-enables use of the context just prior to dispatching the contextrestored event. I would guess that events might get queued, and so maybe e.g. RAF races and gets run after contextrestored is dispatched, but before contextrestored listeners are run.

Likewise context loss disables effective use of the context immediately and then dispatches contextlost.
I believe that behavior is spec'd around the actual context loss/restore, not the event listeners. The only interaction is that we conditionally (try to) context-restore based on use-default for the contextlost event.

This could be tightened up in implementations I bet, but I'm not sure that it's much of a problem as-is today.

@yiyix
Copy link
Contributor

yiyix commented Oct 28, 2021

Updated the notes in this pull request, #7269

@yiyix
Copy link
Contributor

yiyix commented Oct 29, 2021

In the internal chromium implementation, contextlost will trigger contextlost event immediately. contextrestored will be called when canvas is visible again and contextrestored event is called inside contextrestored. So if a user wants to draw or interact with canvas after contextlost, the canvas needs to be selected and visible first, then it means contextRestored is already fired. So I don't think it's possible for users to interact with canvas in between contextlost and contextrestored event.

domenic pushed a commit that referenced this issue Nov 1, 2021
@yiyix
Copy link
Contributor

yiyix commented Nov 10, 2021

Following up this discussion, what are other concerns? Anything else in the spec/code looks ambiguous? I am happy to update them.

@annevk
Copy link
Member Author

annevk commented Nov 18, 2021

I think this issue still remains:

I thought the idea was that we'd dispatch the WebGL events at the same time. Currently the WebGL events are not integrated to the level whereby they are dispatched from the rendering part of the event loop.

Perhaps this should be filed against WebGL as well? But it would probably require changes to HTML as well. @kenrussell thoughts?

dandclark pushed a commit to dandclark/html that referenced this issue Dec 4, 2021
mfreed7 pushed a commit to mfreed7/html that referenced this issue Jun 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification Standard could be clearer topic: canvas
Development

No branches or pull requests

6 participants