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

Difference between region behavior for MouseEvent and Touch #548

Closed
RByers opened this issue Jan 20, 2016 · 16 comments · Fixed by #1942
Closed

Difference between region behavior for MouseEvent and Touch #548

RByers opened this issue Jan 20, 2016 · 16 comments · Fixed by #1942

Comments

@RByers
Copy link

RByers commented Jan 20, 2016

I'm comparing the spec for the region property for MouseEvent and Touch and there are some surprising differences:

  1. It makes sense that Touch is defined with an implicit capture model ("when T was first placed on the surface") instead of based on the current location - that matches the semantics of TouchEvent (which do a hit-test only at touchstart time).
  2. The "The region attribute on MouseEvent objects must return the value it was initialised to." text is presumably not applied to Touch due only to issue TouchInit should be augmented to add 'region' #547 - hopefully fixing that will make the two similar in this regard.
  3. Why does the MouseEvent case have "rerouting" support but Touch doesn't?
  4. Similarily, why does MouseEvent talk about setting the "control" but Touch doesn't?

Can't these two cases be defined using shared spec text for everything except the capture model (when exactly the region is determined)?

@domenic
Copy link
Member

domenic commented Jan 20, 2016

So, in general I agree it would be cleaner to share as much as possible except the capture model. The only thing we should check is if Firefox actually implements the different models. If it does not implement the differences, then yeah, we should just fix this.

Do you have some quick test cases we could use?

@RByers
Copy link
Author

RByers commented Jan 20, 2016

I don't really know anything about these APIs (just doing API owner research for the blink intent-to-ship), @junov WDYT?

@junov
Copy link
Member

junov commented Jan 20, 2016

@foolip WDYT?

@annevk
Copy link
Member

annevk commented Jan 21, 2016

@smaug---- you have looked at touch events, no? @cabanier, any insights?

@foolip
Copy link
Member

foolip commented Jan 21, 2016

I haven't taken the time to understand the details of rerouting, but I do think it would be nice if both Touch and MouseEvent were associated with a region upon creation, or at the very least that the region attribute's values cannot change in response to DOM modification after creation.

I also see a bit of monkey-patching in "When a MouseEvent is to be fired at a canvas element by the user agent in response to a pointing device action" (my emphasis). Where does this requirement actually end up being implemented, and does it have to be only for UA-dispatched events? If it's inside dispatchEvent then shouldn't DOM have an extension point that this uses? @annevk?

@smaug----
Copy link
Collaborator

I haven't looked at the hit-region API too much, and looks like the implementation in Gecko is still behind canvas.hitregions.enabled pref (default to false). And also the implementation in Chrome is behind a flag, so it should be still fine to change the API, especially if we can simplify it. And better to do that before anyone ships the API.
I agree with @RByers that (3) and (4) are odd inconsistencies in the API and should be changed unless there is some good reason for the current model.

In general I'm not too happy the API adding .region to MouseEvent and Touch
If we do rerouting, why do we even need .region? It feels like a hackish event retargeting.
in this case canvasElement.contains(event.target); should work fine.

Comment to @foolip, script initiated events shouldn't usually trigger any default handling or such.
So it makes sense to rely on UA-dispatched events here.
(click event is a very unusual special case where the web relies on <a>.dispatchEvent(new MouseEvent("click")); to trigger the link. )
And "When a MouseEvent is to be fired at a canvas" sounds like something which happens before dispatchEvent, sort of part of hit testing.

@annevk
Copy link
Member

annevk commented Jan 22, 2016

@foolip it is monkey patching "hit testing" which is part of layout in a way and I think fits most in CSS, but they have not worked on this. It's basically the primitive, :hover, mouse events, elementFromPoint() etc. all depend upon.

@foolip
Copy link
Member

foolip commented Jan 26, 2016

For context, this spec issue was triggered by an Intent to Ship: Canvas Hit Regions on blink-dev.

I think @RByers said somewhere he was going to be away for a bit. @junov, do you have a handle on all of these issues and how it should be resolved? Where does the retargeting happen in Blink, and can that be the only place that finds a region from a location? (Now MouseEventHitRegion::region does it too.) How about @smaug----'s question, "If we do rerouting, why do we even need .region?"

@romandev
Copy link
Contributor

FYI, Blink and Mozilla haven't implemented 'event-rerouting' by hit region's control yet.
and followed the W3C recommendation[1] up. (current status of Blink and Mozilla implementation)

[1] https://www.w3.org/TR/2dcontext/#hit-region

@RByers,
In case of (3) and (4),
I'm not sure but it would be impossible in the following situation.

  • There are two hit regions on canvas.
  • Each hit region has each other 'control' (=canvas fallback elemenet).
  • If there are two touches at the same time on different regions,
    where do we have to reroute the touch event to?
  • Touch is not TouchEvent and we have to reroute TouchEvent.
    Multiple touches on canvas will make only one TouchEvent on canvas.

@foolip, @smaug----
I think it's still useful for the following reasons.

  • We can identify the regions when TouchEvent has multiple touches.
  • If there's no control, we can return early.
  • If there are many hit regions on canvas, web developer should declare many fallback elements.
  • Because 'region' attribute == hit region's id, it's a simpler identifying way for region.
    On the other hand, hit region's control is for canvas accessibility.

@smaug----
Copy link
Collaborator

It is surprising to follow a https://www.w3.org/TR/* spec when there is an actively maintained WhatWG spec for the same thing.
The implementation in Gecko is behind the WhatWG spec and should be updated once
the spec has been fixed.

If there are two touches at the same time on different regions, where do we have to reroute the
touch event to?
We should reroute Touch, not TouchEvent. That is what Rick was talking about.

We can identify the regions when TouchEvent has multiple touches.
If we reroute touches, touch.target would be some control

Because 'region' attribute == hit region's id, it's a simpler identifying way for region. On the other hand, hit region's control is for canvas accessibility.
Using .target is even easier.

@annevk
Copy link
Member

annevk commented Oct 19, 2016

FWIW, I'm inclined to remove the feature from the specification given that nobody has shipped anything here thus far and there's disagreement over how it should work. I'd rather remove something that is known to be incorrect and not shipped than keep it in place and hope for updates. And to be fair, we've been doing the latter for nine months now.

Thoughts?

@foolip
Copy link
Member

foolip commented Oct 19, 2016

Looks like CanvasRenderingContext2D.prototype.addHitRegion is undefined everywhere, but both Gecko and Blink have it behind flags in their IDLs. The blink-dev Intent to Ship says that some parts of this have shipped in Firefox, but I can't figure out what parts it is.

@romandev @junov, are you still pursuing this in Blink?

It looks like https://bugzilla.mozilla.org/show_bug.cgi?id=1129147 is the Gecko bug. I can't find Milan Sreckovic on GitHub. @ehsan @cabanier, do you know if anyone's working to get this shipping?

@RByers
Copy link
Author

RByers commented Oct 19, 2016

Since this is a new feature under semi-active development, I'd support removing it from the spec and (assuming people are still interested in pursuing it) iterating on it via incubation instead.

@foolip
Copy link
Member

foolip commented Oct 19, 2016

That would be a fine outcome I think. If the feature can be sensibly described in a standalone doc then HTML could add integration points to limit monkey patching. Or if it requires deep integration it could be maintained as a fork of this repo.

@annevk
Copy link
Member

annevk commented Oct 19, 2016

Basically someone would have to write a PR that adds the feature back in, minus the flaws. Could maybe be a monkey patch for a while, similar to the OffscreenCanvas work.

@smaug----
Copy link
Collaborator

I'd support removing it from the spec and (assuming people are still interested in pursuing it) iterating on it via incubation instead.

Assuming incubation doesn't mean shipping, this sounds ok to me.

annevk added a commit that referenced this issue Oct 21, 2016
As currently defined the feature has a number of objections due to it
altering hit testing without there being a real need to do so. The API
can also be simplified to only work with elements rather than
accommodating arbitrary objects.

Fixes #548. Closes #547, closes #849, and closes #1029, due to hit
regions requiring a fresh PR. #1030 is the follow-up issue for new hit
region work.
annevk added a commit that referenced this issue Oct 21, 2016
As currently defined the feature has a number of objections due to it
altering hit testing without there being a real need to do so. The API
can also be simplified to only work with elements rather than
accommodating arbitrary objects.

Fixes #548. Closes #547, closes #849, and closes #1029, due to hit
regions requiring a fresh PR. #1030 is the follow-up issue for new hit
region work.
alice pushed a commit to alice/html that referenced this issue Jan 8, 2019
As currently defined the feature has a number of objections due to it
altering hit testing without there being a real need to do so. The API
can also be simplified to only work with elements rather than
accommodating arbitrary objects.

Fixes whatwg#548. Closes whatwg#547, closes whatwg#849, and closes whatwg#1029, due to hit
regions requiring a fresh PR. whatwg#1030 is the follow-up issue for new hit
region work.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

7 participants