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

Flashing screens on mobile devices #25

Closed
FelixAkk opened this issue May 4, 2018 · 6 comments
Closed

Flashing screens on mobile devices #25

FelixAkk opened this issue May 4, 2018 · 6 comments

Comments

@FelixAkk
Copy link
Contributor

FelixAkk commented May 4, 2018

Hi, I was wondering for what reason this line exists:

document.body.style.cursor = 'pointer';

Symptom

Mobile browsers (Android Chrome, iOS Safari) will make the entire screen flash blue or grey (respectively) when tapping anywhere on the page when the mixin is active:
2018-05-04 14 06 31

Cause

This is because they draw a highlight when an interactive element is tapped (see https://developer.apple.com/library/content/documentation/AppleApplications/Reference/SafariWebContent/AdjustingtheTextSize/AdjustingtheTextSize.html#//apple_ref/doc/uid/TP40006510-SW5 or https://developer.mozilla.org/en-US/docs/Web/CSS/-webkit-tap-highlight-color) to give affirming feedback to the user that they did/didn't tap the element they indended, which is quite useful. Turning of the highlighting as a workaround would deprive the user of this valuable feedback, so I think a better fix would be to change the mixin, because setting cursor: pointer; also makes the browser interpret it as an interactable element.

I noticed that just above (if (!supportsTouchEvents())) there's a check for touch supported devices that looks like it's supposed to account for this. However maybe I found a bug there; it enables this when I would expect it to disable it (if you're on a mobile device, abort adding the line) ... I'm not sure if this is working as intended.

Fix

If the condition above (if !supportsTouchEvents) is indeed intended to avoid this problem, simply removing the ! make it function as intended (see my PR #26).

If this is a unknown side effect of the cursor: pointer; style, but the line is not really essential, simply removing it or making it conditional (so it can be turned of by setting a variable to false when detecting a mobile environment) would already do so ... but then, I don't know if there some reason why it was included that I might be ignorant of :)

@FelixAkk FelixAkk changed the title Flashing screens for mobile devices Flashing screens on mobile devices May 4, 2018
@zeppelin
Copy link
Owner

zeppelin commented May 4, 2018

@FelixAkk Thank you for the awesome bug report!

I'll review this over the weekend.


Related: #13

@FelixAkk
Copy link
Contributor Author

FelixAkk commented May 7, 2018

Ah thanks! :) that PR reference provides a lot of explanation. It's hard to find clear documentation about this iOS behavior but others also seem to have encountered this: aurelia/binding#263

So I'd say the best option is to detect iOS, only engage the behavior then, as to leave other platforms unaffected with this side effect.

@zeppelin
Copy link
Owner

zeppelin commented May 7, 2018

@FelixAkk Had to work on something else on the weekend... :/

Could you update the PR with the solition you outlined?

@FelixAkk
Copy link
Contributor Author

FelixAkk commented May 7, 2018

No problem. Will give it a try today :)

@FelixAkk
Copy link
Contributor Author

Hi @zeppelin, sorry for the late reply. I had a fix implemented in 15min but for some reason I couldn't get the repo linked to test it out. So in the end I took the liberty to build a very primitive demo in the dummy app to verify it (see the PR: #29). What do you think? :)

@zeppelin
Copy link
Owner

Fixed in #29

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants