feat: get hideOn clickout to work on iOS #115

necojackarc · 2018-03-05T04:20:49Z

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 · 2018-03-05T05:37:07Z

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 · 2018-03-06T05:01:01Z

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 · 2018-03-06T21:13:39Z

tests/integration/components/ember-attacher/hide-on-clickout-test.js

@@ -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);


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

necojackarc · 2018-03-06T21:14:13Z

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

kybishop · 2018-03-12T06:27:41Z

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

kybishop · 2018-03-12T23:13:08Z

addon/components/attach-popover.js

+
+ // 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/)) {


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

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.

kybishop · 2018-03-12T23:28:21Z

addon/components/attach-popover.js

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


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?

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,

users can click a button then a tooltip will appear

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 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.

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

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.

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.

Sounds nice enough! I'll update my PR!

necojackarc · 2018-03-14T03:19:34Z

@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 · 2018-03-14T06:25:29Z

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

necojackarc force-pushed the get-hide-on-clickout-to-work-on-ios branch from 3f0b36c to 89d9d9f Compare March 6, 2018 03:22

necojackarc force-pushed the get-hide-on-clickout-to-work-on-ios branch 6 times, most recently from 9c97771 to 905f27b Compare March 6, 2018 21:00

necojackarc commented Mar 6, 2018

View reviewed changes

kybishop requested changes Mar 13, 2018

View reviewed changes

necojackarc added 2 commits March 14, 2018 02:49

feat: enable hideOn clickout on touch devices

bcf97bc

chore: add a test to hideOn clickout on touch devices

684d0e6

necojackarc force-pushed the get-hide-on-clickout-to-work-on-ios branch from 905f27b to 684d0e6 Compare March 14, 2018 02:59

kybishop approved these changes Mar 14, 2018

View reviewed changes

kybishop merged commit 0f02623 into tylerturdenpants:master Mar 14, 2018

necojackarc deleted the get-hide-on-clickout-to-work-on-ios branch March 14, 2018 15:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: get hideOn clickout to work on iOS #115

feat: get hideOn clickout to work on iOS #115

necojackarc commented Mar 5, 2018

kybishop commented Mar 5, 2018

necojackarc commented Mar 6, 2018

necojackarc Mar 6, 2018 •

edited

Loading

necojackarc commented Mar 6, 2018

kybishop commented Mar 12, 2018

kybishop Mar 12, 2018

kybishop Mar 13, 2018

kybishop Mar 12, 2018

necojackarc Mar 13, 2018 •

edited

Loading

kybishop Mar 13, 2018

necojackarc Mar 13, 2018

necojackarc Mar 13, 2018

kybishop Mar 13, 2018

necojackarc Mar 14, 2018

necojackarc commented Mar 14, 2018

kybishop commented Mar 14, 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);

feat: get hideOn clickout to work on iOS #115

feat: get hideOn clickout to work on iOS #115

Conversation

necojackarc commented Mar 5, 2018

kybishop commented Mar 5, 2018

necojackarc commented Mar 6, 2018

necojackarc Mar 6, 2018 • edited Loading

Choose a reason for hiding this comment

necojackarc commented Mar 6, 2018

kybishop commented Mar 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

necojackarc Mar 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

necojackarc commented Mar 14, 2018

kybishop commented Mar 14, 2018

necojackarc Mar 6, 2018 •

edited

Loading

necojackarc Mar 13, 2018 •

edited

Loading