Ticket #3402 (closed bug: duplicate)

Opened 23 months ago

Last modified 13 months ago

Handler: Remove "setEvent" on mouse events register when we have touch.

Reported by: jorix Owned by: tschaub
Priority: trivial Milestone: 2.13 Release
Component: Handler Version: 2.11 RC1
Keywords: Cc:
State:

Description

To complete the work of #3210 would be interesting to also unregister setEvent method for mouse events.

Not currently unregistered "mouseout" on touch devices. I think it should be removed as well (this proposal is not on the ticket). I tried it and not affect the use on Android (and more, in the examples of the trunk with Android or iOS Handler.Drag works like documentDrag = true.

The patch adds tests.

Attachments

3402.patch Download (1.6 KB) - added by jorix 23 months ago.
3402.1.patch Download (7.2 KB) - added by jorix 23 months ago.
3402.2.patch Download (15.4 KB) - added by jorix 23 months ago.
3402.2-2.patch Download (14.7 KB) - added by jorix 20 months ago.
Merged with current trunk and changes in tests

Change History

Changed 23 months ago by jorix

  Changed 23 months ago by jorix

  • priority changed from minor to trivial
  • state set to Review

follow-ups: ↓ 4 ↓ 6   Changed 23 months ago by erilem

Thanks Jorix.

Do you think we need this change in 2.11? Does your patch fix a problem that effectively affects mobile users? If not I wouldn't apply this patch for 2.11.

Also, shouldn't we do the same in Handler.Click and Handler.Point? This makes me think that we may want to include this logic in the base class.

  Changed 23 months ago by erilem

  • milestone changed from 2.11 Release to 2.12 Release

Changing Milestone to 2.12. Jorix, please speak up with you think this should be addressed in 2.11.

Changed 23 months ago by jorix

in reply to: ↑ 2 ; follow-up: ↓ 5   Changed 23 months ago by jorix

  • state Review deleted

Replying to erilem:

I saw this problem by working in #3399 (I guess you remember similar problem in #3052) and thinking ahocevar's patch #3363. I thought only drag ;)

Thanks Jorix. Do you think we need this change in 2.11? Does your patch fix a problem that effectively affects mobile users? If not I wouldn't apply this patch for 2.11.

The choice is yours. But I think it does no harm at 2.11, and otherwise a control may see a mouse event in Handler.evt using touch devices, ups!

Also, shouldn't we do the same in Handler.Click and Handler.Point? This makes me think that we may want to include this logic in the base class.

Yeah, right. New patch 3402.1.patch Download attached with a first approach applied in the classes and more tests.

What to do with "mouseout"?

in reply to: ↑ 4   Changed 23 months ago by erilem

Replying to jorix:

What to do with "mouseout"?

I think we should unregister mouseout listeners as well.

Changed 23 months ago by jorix

in reply to: ↑ 2   Changed 23 months ago by jorix

  • owner set to tschaub
  • state set to Review
  • component changed from Handler.Drag to Handler
  • summary changed from Handler.Drag: Remove setEvent on click when we have touch. to Handler: Remove "setEvent" on mouse events register when we have touch.

Replying to erilem:

... This makes me think that we may want to include this logic in the base class.

3402.2.patch Download applies the logic in the base class and add more tests.

Changed 20 months ago by jorix

Merged with current trunk and changes in tests

  Changed 20 months ago by jorix

In the implementation 3402.2-2.patch Download has followed comment:2 and comment:5 of erilem.

Please review, thanks

  Changed 15 months ago by jorix

Please review:  https://github.com/openlayers/openlayers/pull/224

  Changed 13 months ago by jorix

  • status changed from new to closed
  • state Review deleted
  • resolution set to duplicate

follow on GitHub

Note: See TracTickets for help on using tickets.