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

Add support to Contextlost to Canvas 2d API #6797

Merged
merged 10 commits into from
Aug 18, 2021
Merged

Add support to Contextlost to Canvas 2d API #6797

merged 10 commits into from
Aug 18, 2021

Conversation

yiyix
Copy link
Contributor

@yiyix yiyix commented Jun 23, 2021

(See WHATWG Working Mode: Changes for more details.)


/canvas.html ( diff )
/indices.html ( diff )
/webappapis.html ( diff )

@yiyix yiyix changed the title Contextlost Add support to Contextlost to Canvas 2d API Jun 23, 2021
@annevk
Copy link
Member

annevk commented Jun 23, 2021

This does not (yet?) match the discussion in #4809, e.g., #4809 (comment). I think it should.

Why is isContextLost() a method and not a getter?

Why is the name for the second event contextrestored and not contextreset as discussed?

cc @whatwg/canvas

@annevk annevk added addition/proposal New features or enhancements topic: canvas labels Jun 23, 2021
@yiyix
Copy link
Contributor Author

yiyix commented Jul 6, 2021

This does not (yet?) match the discussion in #4809, e.g., #4809 (comment). I think it should.

Sorry, I missed that comment, I moved the event description to 'update the rendering' session.

Why is isContextLost() a method and not a getter?

I think this makes more sense as well! But an attribute cannot start with verb. Maybe i can add an attribute called ContextLost, ContextIsLost? What do you think?

Why is the name for the second event contextrestored and not contextreset as discussed?

In Ken's replay, he explained it is possible not to restore a lost event. So we decide to implement it as 2 events instead of 1.

cc @whatwg/canvas

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated

<li><p>Reset <span data-x="concept-canvas-context-lost-flag">context lost flag</span>.</p></li>

<li><p>Fire an event <code data-x="event-contextrestored">contextrestored</code> at <var>canvas</var>.</p></li>
Copy link
Member

Choose a reason for hiding this comment

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

Interesting that this one doesn't queue a task...

Copy link
Contributor Author

@yiyix yiyix Jul 10, 2021

Choose a reason for hiding this comment

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

I got the idea of how to write this session from webgl contextlost event spec: https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.15.2I. In context lost event, we need to queue a task to call context restore event after. In context restore event, it's done after it's restored, so we don't need to queue a task to call any other events.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@domenic
Copy link
Member

domenic commented Jul 8, 2021

I forgot to say: an attribute called contextLost makes sense to me.

@fserb
Copy link
Contributor

fserb commented Jul 12, 2021

I forgot to say: an attribute called contextLost makes sense to me.

Originally that was our proposal (and implementation).

The problem is WebGL uses isContextLost() (on WebGLRenderingContext). So maybe keeping it the same on CanvasRenderingContext2D makes more sense? WDYT?

@domenic
Copy link
Member

domenic commented Jul 12, 2021

Oh, yeah, in that case staying consistent gets my vote.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated
<li><p>Let <var>canvas</var> be the value of <var>context</var>'s <var>canvas</var> or the <code>canvas</code> associated with <code>OffscreenCanvas</code> object.</p></li>

<li><p>If <var>context</var>'s <span data-x="concept-canvas-context-lost">context lost
flag</span> is not set, abort these steps.</p></li>
Copy link
Member

Choose a reason for hiding this comment

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

When would this condition be true? I.e. when would the user agent try to restore the backing storage for something without its context lost set to true?

BTW the phrasing should be "If context's context lost is false"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got this line from webgl spec, https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.15.3. I don't know how to satisfy this line's condition. I thought about it when i put it down, I still keep it here to avoid canvas end up in some weird place. Do you think it's better to remove it?

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated
attribute <span>EventHandler</span> <span data-x="handler-oncontextmenu">oncontextmenu</span>;
attribute <span>EventHandler</span> <span data-x="handler-oncontextrestored">oncontextrestored</span>;
Copy link
Member

Choose a reason for hiding this comment

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

You should probably add oncontextlost and oncontextrestored to the OffscreenCanvas IDL too. Then you need a small table like the one at the bottom of https://html.spec.whatwg.org/#the-eventsource-interface inside the OffscreenCanvas section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's true, updated.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Overall looking good. I have some grammar nits but we can fix them later. The big question is around cancelability.

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@annevk
Copy link
Member

annevk commented Aug 4, 2021

I don't see point 2 of #4809 (comment) reflected in this PR. In particular I thought one goal was to create a shared abstraction with WebGL.

@yiyix
Copy link
Contributor Author

yiyix commented Aug 9, 2021

I don't see point 2 of #4809 (comment) reflected in this PR. In particular I thought one goal was to create a shared abstraction with WebGL.

Could you clarify what you mean by creating a shared abstraction with WebGL? We are using the same interface and same event names as webgl, I can add some notes, if it's webgl context, then follow the webgl spec.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

I pushed a commit with some minor editorial touchups; PTAL.

Then I have two more substantive questions which I left as inline comments.

source Show resolved Hide resolved
source Show resolved Hide resolved
@yiyix
Copy link
Contributor Author

yiyix commented Aug 11, 2021

Sorry, I am very new to github. When i try to download your previous commit, I somehow messed up my commit history and this includes every commit i did in the past. I reviewed the changed file and it seems to be up to date. If it is too confusing, I can start a new branch and do all commits there.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

OK, this looks good to me! I filed web-platform-tests/wpt#30039 to see if eventually we can convert your tests into web platform tests, but that's not a blocker.

@annevk, do you want to take a final pass? Otherwise I'll merge in the next day or two.

IMO further unification with Web GL, if it's even desirable, should be done as a separate followup. In particular the semantics of the events are backward, so it's not clear how much they can really share.

@domenic domenic merged commit 88c994a into whatwg:main Aug 18, 2021
@annevk annevk mentioned this pull request Sep 29, 2021
@yiyix yiyix deleted the contextlost branch January 4, 2023 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements topic: canvas
Development

Successfully merging this pull request may close these issues.

None yet

6 participants