PolymerElements / paper-behaviors

Common behaviors used across paper-* elements.
19 stars 27 forks source link

paper buttons get unexpected focus on Android and iOS Chrome #80

Open jab opened 7 years ago

jab commented 7 years ago

Expected outcome

The button is not focused.

Actual outcome

The button is focused.

Live Demo

https://jab.github.io/paper-behaviors-issue-80/

source: https://github.com/jab/paper-behaviors-issue-80/

Steps to reproduce

  1. Open the live demo in Chrome for Android or iOS (does not reproduce in iOS Safari)
  2. Click the "go to page 2" link as directed
  3. Click the back button as directed

Browsers Affected

Hypothesis

Looking at https://github.com/PolymerElements/paper-behaviors/blob/fd8c137/paper-inky-focus-behavior.html#L26-L33, I think what's happening is that for some reason, the receivedFocusFromKeyboard param is getting passed in as true, even though the only user interactions have been taps (there's no keyboard in sight).

Screenshots

Chrome for iOS

iOS

Chrome for Android

Android

jab commented 7 years ago

(Changed the live demo link in the description from the plnkr.co link I was originally using, since it was flaky)

notwaldorf commented 7 years ago

Hmmm, this sounds a lot like https://github.com/PolymerElements/paper-button/issues/24. Do you think it's the same?

jab commented 7 years ago

It looks like it could be related, though I've never triggered this by switching browser tabs.

FWIW, you can reproduce this in desktop Chrome by turning on mobile device emulation:

screen shot 2017-06-13 at 22 28 27

Looking further, I just set a breakpoint in PaperInkyFocusBehaviorImpl\'s _focusChanged(..) to see why receivedFocusFromKeyboard is getting set to true. Walking up the call stack, I came to IronButtonStateImpl\'s _detectKeyboardFocus(..), where I noticed that the focused param it's getting passed is true:

screen shot 2017-06-13 at 22 43 55

Walking up the stack a little further, I came to IronControlState\'s _focusBlurHandler(..), which is what's incorrectly setting focused on this button to true in the first place:

screen shot 2017-06-13 at 22 38 05

At that point my call stack ends, so I'm not immediately sure what's triggering _focusBlurHandler(..) on this button. If I can figure that out, maybe I can figure out how to stop this from happening in the first place. If you have any ideas, please let me know. Thanks!

jab commented 7 years ago

@notwaldorf I noticed that with the following patch against my repro, the bug goes away:

diff --git a/app-root.html b/app-root.html
index 7918ac2..b6a5de6 100644
--- a/app-root.html
+++ b/app-root.html
@@ -101,7 +101,7 @@
       changePage: function (page) {
         this.async(function () {
           this.set('pageData.page', page);
-        });
+        }, 250);
       },
       _computePage: function (pageData) {
         return pageData.page;

Is this somehow a helpful clue in fixing the bug?

e111077 commented 7 years ago

Looking into this deeper, it seems to be an issue with our state machine in iron-button-state. Due to the sticky behavior of a tap, the button under the finger while you tap changes due to the javascript, and thus a new button is focused.

The problem is that since this is a new button with a different instance of iron-button-state, it cannot tell that the user has their finger down and thus believes that the pointer is up and the button is focused. Thus, the element thinks that this was a keyboard focus.

This is why when you change the state with the async, it gives the user enough time to lift their finger before it changes to a new button. Thus, the element does not gain focus on attached.

TL;DR: The user enters the state machine of iron-button-state at the wrong point

jab commented 7 years ago

Thanks for taking a look at this @e111077!

That explanation makes it sound to me like the finger is being held down in the same spot while button A is swapped out for button B, which is what's causing button B to think there's a finger over it, and therefore that it should be focused.

But when I hold my finger down over button A, nothing happens -- that is, it's not actually swapped out for button B -- until I lift my finger off of it. Which is how it seems like on-tap is supposed to work.

Given that, I'm not following why the system ever thinks there's a finger over button B. Am I misunderstanding?

In any case, it looks like we agree that the line this._setFocused(event.type === 'focus') (highlighted in my most recent screenshot) is problematic. That ends up amounting to _setFocused(true), even though the button should not be focused. Not immediately sure how to change the logic though to fix this. Do you have any ideas?

jab commented 7 years ago

Meant to say, one other thing that would be interesting to address here is why the bug manifests in some environments (e.g. Chrome for iOS and Android) but not others (mobile Safari).

jab commented 6 years ago

We upgraded to Polymer 2 and are still seeing this behavior.

I realized I could create an even simpler reproduction using alert: https://cdn.rawgit.com/jab/paper-behaviors-issue-80/f46a9164/polymer2.html

The source for this is at https://github.com/jab/paper-behaviors-issue-80/blob/master/polymer2.html

@notwaldorf @e111077 et al., do you have any ideas for a fix or workaround?

Thanks so much for taking a look!

jab commented 6 years ago

Meant to say, this new test case reproduces the bug for me no matter what browser or platform I test with. I assume it's the same bug with incorrect "using the keyboard?" detection, but please let me know if it's a separate issue.

keanulee commented 6 years ago

Unfortunately, the comment I previously made on paper-button is still the case (code reference should now be https://github.com/PolymerElements/iron-behaviors/blob/46cc9bd4c09a87650697584cc60dd52a352bccfd/iron-button-state.html#L124).