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 enablePointerEvents to Canvas init option #5589

Merged
merged 4 commits into from May 3, 2019

Conversation

@arcatdmz
Copy link
Contributor

commented Mar 23, 2019

In response to discussions in #5587, this PR adds enablePointerEvents option to the Canvas constructor.

When the option is enabled by new Canvas(.., { enablePointerEvents: true }), PointerEvent is used instead of MouseEvent.
The use of PointerEvent opens up the potential to retrieve pen pressure information and implement a pressure-sensitive brush.

canvasEvtIn: 'mouse:over',
evtIn: 'mouseover',
evtIn: eventTypePrefix + 'over',

This comment has been minimized.

Copy link
@asturur

asturur Mar 24, 2019

Member

those can stay mouse i guess. Renaming this is gonna break application badly. Those are fabricJS events that devs uses, there is no reason to rename them

This comment has been minimized.

Copy link
@arcatdmz

arcatdmz Mar 25, 2019

Author Contributor

makes sense -- let's keep the external API as same as possible. I found several other occurrences of the same issue and addressed that in 6c6f4de.

@asturur

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

to me this looks like good.
I would try to merge it as it is. I want to check first if we can adapt UTs to run on both of them.

Current supported browsers are ie10+, edge, chrome, firefox, safari and a i think they all support pointer events in a way or another.

@arcatdmz

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

Thank you @asturur for your timely feedback! I hope the rest of the tasks before merging wouldn't be tough on your side.

Regarding the next step toward pressure-sensitive brushes (#5587), I've looked into the mechanism on how pointer events are passed to free drawing brushes and found _onMouse*InDrawingMode methods defined in canvas_events.mixin.js.
e.g., https://github.com/fabricjs/fabric.js/blob/master/src/mixins/canvas_events.mixin.js#L523

They're calling this.freeDrawingBrush.onMouse*(pointer) methods where the pointer parameter is retrieved by calling getPointer method. (onMouseUp is a bit special since it has no parameter.)

Once the PR is accepted, how about adding the second (or the first for onMouseUp) parameter e.g., options which is an object like { pressure: [a pressure value ranging from 0 to 1] }?

@asturur

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

i have been buys, getting back to this asap

@arcatdmz

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

@asturur No hurries, but any updates on this? We'd like to move things forward to support pressure-sensitive brushes.

@asturur

This comment has been minimized.

Copy link
Member

commented May 2, 2019

i jun i ll merge this asap. But i want to talk before you dive into brush changes. brush api can be improved

@arcatdmz

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

@asturur Thanks for your reply, and I'm looking forward to the talk.

@asturur

This comment has been minimized.

Copy link
Member

commented May 3, 2019

so i m merging this now, tomorrow giving it a run and see if i can extend tests somehow.

@asturur asturur merged commit ca94b2d into fabricjs:master May 3, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@asturur

This comment has been minimized.

Copy link
Member

commented May 3, 2019

i wonder, should we work to remove touch related events, are those now unnecessary? what could support touch but not pointerevents that make sense to support?

@arcatdmz

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

Thank you for merging the code!

I personally love the simplicity of PointerEvent, but it is unfortunately not supported on iOS Safari.
Given that iOS Safari (on an iPad Pro) is a great target for dealing with pressure-sensitive input, I think we still need to keep support for TouchEvent.
https://caniuse.com/#feat=pointer

@arcatdmz

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

Another source for checking unsupported browsers:
https://developer.mozilla.org/en-US/docs/Web/API/PointerEvent

@asturur

This comment has been minimized.

Copy link
Member

commented May 7, 2019

ios safari has so many limitations on canvas too.

pointer events can eventually take the place of mouse events when we are sure they work perfectly

@arcatdmz

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

So, in the end, we would only need to support PointerEvent without looking at MouseEvent nor TouchEvent -- what a beautiful world!

Meanwhile, I think we still need to wait for some more time, and removing support for TouchEvent (or MouseEvent) sounds too early. In my particular case, iPad pencil support is very important (which was the initial motivation behind this issue.)

@asturur

This comment has been minimized.

Copy link
Member

commented May 7, 2019

nothing of this is gonna happen soon.
pointerEvents are more supported where you have a mouse than touch, i wanted only to say this.
All desktop browser, IE10 included, support pointer events.

@asturur

This comment has been minimized.

Copy link
Member

commented May 7, 2019

ohhh stupid Safari.

@dirkluijk

This comment has been minimized.

Copy link

commented May 22, 2019

There is actually a very good polyfill for PointerEvent: https://www.npmjs.com/package/pepjs. It only needs touch-action="none" to be present on the element to track the events.

Has that been tested in this PR?

I think the true profit of this PR would be to remove all the old Mouse and Touch events in favour of PointerEvent. That would make the Fabric.js code base a lot cleaner.

@asturur

This comment has been minimized.

Copy link
Member

commented May 22, 2019

i would agree, but we need a solution for ie10 too that adds the msPointer prefix. if then safari polyfill works and can be embedded optionally we can remove touch and mouse all at once.

@dirkluijk

This comment has been minimized.

Copy link

commented May 23, 2019

IE10? Are you kidding me? 😄 (no offence here)
https://www.xfive.co/blog/stop-supporting-ie10-ie9-ie8/

+1 for removing touch and mouse events in the future.

@asturur

This comment has been minimized.

Copy link
Member

commented May 23, 2019

Well, it has a canvas, supports es5 mostly, not supporting it is a choice not a necessity. So many people for small projects do want to still being able to run mostly everywhere.

I'm more like ios on latest iphones not supporting pointer events are you kidding me? :D

@dirkluijk

This comment has been minimized.

Copy link

commented May 23, 2019

I'm more like ios on latest iphones not supporting pointer events are you kidding me? :D

That's so true.

@DetrianHallem

This comment has been minimized.

Copy link

commented Aug 13, 2019

@arcatdmz Has there been any more progress on this?

@asturur

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

Pointer events have been merged and work great.
The event is passed raw to the brush and can be used.
What kind of progress are you looking for?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.