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

feat: get hideOn clickout to work on iOS #115

Merged

Conversation

@necojackarc
Copy link
Contributor

necojackarc commented Mar 5, 2018

On iOS, document.addEventListener('click', handler) dosen't work.
It means hideOn='clickout' also doesn't work on iOS.
Actually, I couldn't close popped up items by tapping outside of them.

This PR will fix this issue using touch events to simulate clicks on iOS.
The algorithm is quite simple - if a touched position doesn't move from start to finish, regard the action as a click then execute this._hideOnClickOut(event) handler.

Since I have no idea how I can add iOS specific tests, I haven't added any tests...

@kybishop

This comment has been minimized.

Copy link
Owner

kybishop commented Mar 5, 2018

Hey @necojackarc, thanks for the PR!

Note that we add all hide listeners to this._hideListenersOnDocumentByEvent. This prevents memory leaks by allowing us to remove listeners on document.

This will require the hiding method(s) to be named. See the other hide functions for an example, and how we need to bind this to them in the init function.

You can add tests right to the hide-on-click and hide-on-clickout tests.

Note how events are triggered manually, so you can simply trigger touch events within the tests themselves.

@necojackarc necojackarc force-pushed the necojackarc:get-hide-on-clickout-to-work-on-ios branch from 3f0b36c to 89d9d9f Mar 6, 2018
@necojackarc

This comment has been minimized.

Copy link
Contributor Author

necojackarc commented Mar 6, 2018

Hmm, something wrong is happening as one of the tests I added failed only in some environments...
I'll look into it tomorrow or later...

@necojackarc necojackarc force-pushed the necojackarc:get-hide-on-clickout-to-work-on-ios branch 6 times, most recently from 9c97771 to 905f27b Mar 6, 2018
@@ -33,6 +33,8 @@ test('hides when an element outside the target is clicked', async function(asser

await click('#focus-me');

await waitUntil(() => isVisible(attachment) === false);

This comment has been minimized.

Copy link
@necojackarc

necojackarc Mar 6, 2018

Author Contributor

I found that without this waitUntile, this test case would sometimes fail.
Referring to the existing code, I added it.

@necojackarc

This comment has been minimized.

Copy link
Contributor Author

necojackarc commented Mar 6, 2018

@kybishop Okay, I addressed every issue on my PR.
If you have a moment, could you review it? Thanks!

@kybishop

This comment has been minimized.

Copy link
Owner

kybishop commented Mar 12, 2018

things have been a little crazy with emberconf coming up and all. Should have some time to look though this very soon


// On iOS, `document.addEventListener('click', handler)` doesn't work,
// so simulating click events using touch events.
if (navigator.userAgent && navigator.userAgent.match(/iPhone|iPad|iPod/)) {

This comment has been minimized.

Copy link
@kybishop

kybishop Mar 13, 2018

Owner

I bet we can make this solution a little less device specific.

I dug a bit into Tippy.js, a super popular tooltip lib that this addon takes inspiration on, and they look at ontouchstart to see if the device supports touch: https://github.com/atomiks/tippyjs/blob/b755ed52ca93976862974356b09b5811214d4c9f/src/js/core/globals.js#L9

This comment has been minimized.

Copy link
@kybishop

kybishop Mar 13, 2018

Owner

I think we can have a slight perf boost by doing this work only once during the init hook. Basically, we just check if ontouchstart exists in window, then set an instance variable. Something like this:

this.clickoutEvent = 'ontouchstart' in window ? 'touchend' : 'click';

Then we can just use the same logic as before here, but use this.clickoutEvent for the event type and not need a new set of functions either.

document.addEventListener('touchmove', this._disableDocumentClickSimulationOnClickOutOnIos);

this._hideListenersOnDocumentByEvent.touchend = this._hideOnClickOutOnIos;
document.addEventListener('touchend', this._hideOnClickOutOnIos);

This comment has been minimized.

Copy link
@kybishop

kybishop Mar 13, 2018

Owner

We should probably use just one of these events, I'm leaning towards touchend since click is normally fired when the mouse button is released.

Any particular reason we might want to use all three?

This comment has been minimized.

Copy link
@necojackarc

necojackarc Mar 13, 2018

Author Contributor

I'm positive your idea #115 (comment) is the simplest solution!

But, what I'm worried about and why I've used these three events are that using only touchend will change the current behavior of ember-attacher.
Now, on Android devices,

  1. users can click a button then a tooltip will appear
  2. users can scroll the screen with the tooltip being visible

because click events won't be triggered when users are scrolling their screen.

However, touchend events always happen when users lift their fingers off the screen.
Then, users won't be able to scroll with the tooltip being visible.

If you don't mind this change, I think #115 (comment) can be the best!
And I'd like to update the PR following this idea.

How do you feel?

This comment has been minimized.

Copy link
@kybishop

kybishop Mar 13, 2018

Owner

This makes sense. Perhaps we should use touchstart instead of touchend to share the same behavior of hiding tooltips on scroll?

I'll leave the decision up to you. We can always change it if people think it should be one way or the other.

This comment has been minimized.

Copy link
@necojackarc

necojackarc Mar 13, 2018

Author Contributor

Thanks! I'll try to keep the current behavior, but will remove the devise specific code following your idea!

This comment has been minimized.

Copy link
@necojackarc

necojackarc Mar 13, 2018

Author Contributor

This makes sense. Perhaps we should use touchstart instead of touchend to share the same behavior of hiding tooltips on scroll?

Not to change the current behavior, we should use all three events to simulate "a click", I think...

  • touchstart - a user puts their finger on the screen
  • touchmove - a user moves the finger
  • touchend - a user lifts the finger off

Some try to simulate click events "perfectly" then they're tracking the finger position (e.g. If the finger moved only within Npx, this should be regarded as a click), but it may be too much...

Currently, if users don't move their fingers from beginning to end, I regard it as a click.

This comment has been minimized.

Copy link
@kybishop

kybishop Mar 13, 2018

Owner

Ah I see what you mean, if there is a touchmove, it isn't strictly a "click", the user is scrolling the screen.

I poked at Tippy for inspiration and they use touchend as the hide event. Thinking through how this would appear to the user, I believe this to be the best solution, but I'm still open to other ways. Here's my thought process:

  • If a tooltip is currently being shown, the user might want to move the screen around to see the tooltip in a better position. By using touchend, they can still scroll around to see the tooltip in a different position, then have it disappear as soon as they lift their scrolling finger. Theoretically they should be able to move their scrolling finger to the side, allowing them to see the tooltip as they expect.
  • This is a nice and simple solution that requires minimal complex logic.

This comment has been minimized.

Copy link
@necojackarc

necojackarc Mar 14, 2018

Author Contributor

Sounds nice enough! I'll update my PR!

@necojackarc necojackarc force-pushed the necojackarc:get-hide-on-clickout-to-work-on-ios branch from 905f27b to 684d0e6 Mar 14, 2018
@necojackarc

This comment has been minimized.

Copy link
Contributor Author

necojackarc commented Mar 14, 2018

@kybishop I adopted the idea to use touchend instead of click on touch devices.
Could you review it if you have a chance? Thanks!

@kybishop

This comment has been minimized.

Copy link
Owner

kybishop commented Mar 14, 2018

Thanks for the awesome work @necojackarc! I'm just waiting on a fix for ember-decorators before the next release, hopefully very soon :)

@kybishop kybishop merged commit 0f02623 into kybishop:master Mar 14, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@necojackarc necojackarc deleted the necojackarc:get-hide-on-clickout-to-work-on-ios branch Mar 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.