Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

(ripple): No blur after focus on mobile #809

Closed
touficbatache opened this issue Jun 9, 2017 · 9 comments
Closed

(ripple): No blur after focus on mobile #809

touficbatache opened this issue Jun 9, 2017 · 9 comments
Assignees

Comments

@touficbatache
Copy link
Contributor

touficbatache commented Jun 9, 2017

Bug

What MDC-Web Version are you using?

0.12.1

What browser(s) is this bug affecting?

All mobile browsers

What OS are you using?

Any mobile OS

What are the steps to reproduce the bug?

  1. Go to ripple demo page and scroll down to bounded / unbounded or go to the buttons demo page (Any element that has ripple)
  2. Click on the component (button or list item)
  3. Observe the component's behavior

What is the expected behavior?

The ripple should fire then go away and blur after focus

What is the actual behavior?

The ripple actually fires but the element doesn't blur after focus so there is a grey background on it. This happens only the first time.

Any other information you believe would be useful?

Maybe we could add a blur() to the element after it's focused

EDIT

This actually could be fixed by removing the mdc-ripple-upgraded--background-focused class from the element after the ripple animation finishes @traviskaufman

@traviskaufman
Copy link
Contributor

Thanks for reporting this @touficbatache! So what looks like is happening is the background doesn't fade out the first time the ripple is activated, but does on each subsequent time. Definitely a bug.

@touficbatache
Copy link
Contributor Author

Yes, it doesn't fade only the first time. Forgot to mention that 😄😅

@touficbatache
Copy link
Contributor Author

Is someone working on this? @amsheehan @traviskaufman

@kfranqueiro kfranqueiro self-assigned this Jul 27, 2017
@kfranqueiro
Copy link
Contributor

More specifically, this happens initially upon a ripple target receiving focus when it didn't already have it. Repeated taps when already focused behave fine, but if you blur again and re-focus, the same problem occurs.

Moreover, when testing with timestamps in Chrome's console in device emulation mode, I notice that not only does the addClass from the focus handler fire after the removeClass from animateDeactivation_, but also it fires around 300ms later. I suspect we're dealing with a case of 300ms tap delay affecting when focus actually fires.

@touficbatache
Copy link
Contributor Author

touficbatache commented Oct 27, 2017

Hey @kfranqueiro, I was testing the ripple in the Chrome's console in device emulation mode. I can see the removeClass from animateDeactivation_ on line 428 in the ripple's foundation.js file but I still cannot figure out where the addClass from the focus handler is. Could you please tell me on which line it is? I am using the breakpoints to see where it breaks...

@touficbatache
Copy link
Contributor Author

touficbatache commented Oct 27, 2017

I did some tests in desktop mode and in device emulation mode, I used the compiled JavaScript file, added a breakpoint on line 3235 (this.adapter_.removeClass(BG_FOCUSED);) and that's what I found out:

On desktop, those are the functions it calls in order:

  1. (3236) this.runDeactivationUXLogicIfReady_();

  2. (3196) requestAnimationFrame(function () { : This function has 2 if statements, the first one (needsDeactivationUX) returns false and the second one (needsActualDeactivation) returns true.

  3. (3078) return _this5.activationTimerCallback_(); : This function removes everything: activation state, focus, etc... and resets the button to its original state.

  4. (3136) _this6.adapter_.removeClass(FG_DEACTIVATION);

Everything is back to normal

On mobile, those are the functions it calls in order:

(Those are the result if you click on the ripple element the first time, when you click again step 2 returns false for the 1st one and true for the second)

  1. (3236) this.runDeactivationUXLogicIfReady_();

  2. (3196) requestAnimationFrame(function () { : This function has 2 if statements, both return false.

  3. (2870) return _this.adapter_.addClass(MDCRippleFoundation.cssClasses.BG_FOCUSED); : This function focuses on the element.

  4. (3078) return _this5.activationTimerCallback_(); : This function removes everything: activation state, etc... except for the focus, it stays.

  5. (3196) requestAnimationFrame(function () { : This function has 2 if statements, the first one (needsDeactivationUX) returns false and the second one (needsActualDeactivation) returns true.

  6. (3136) _this6.adapter_.removeClass(FG_DEACTIVATION);

Everything is back to normal but the element is still focused

To conclude, the difference is that on mobile it calls the requestAnimationFrame function a first time, it returns false which makes it go and focus the element. Then the return _this5.activationTimerCallback_(); removes everything except the focus. The variable needsActualDeactivation is true when e.type === 'mouseup' which isn't the case on mobile the first time you click on a ripple element. I'm not sure what should be fixed... @kfranqueiro

EDIT:

The problem is
For mobile:
_this7.animateDeactivation_(e, state); at line 3199 isn't firing because needsDeactivationUX isn't true: actualActivationType isn't == expectedActivationType.

@touficbatache touficbatache changed the title Ripple: no blur after focus on mobile (ripple): No blur after focus on mobile Oct 27, 2017
@touficbatache
Copy link
Contributor Author

Hey @mpiroc, your solution for #1124 works fine! Do you have an idea for this one?

@anton-kachurin
Copy link
Contributor

On http://material-components-web.appspot.com/ripple.html using monitorEvents(document.getElementsByClassName('demo-surface')[0], ['pointerdown', 'pointerup', 'focus']):

  1. Desktop (i.e mouse-only):
pointerdown PointerEvent {isTrusted: true, pointerId: 1, width: 1, height: 1, pressure: 0.5, …}
focus FocusEvent {isTrusted: true, relatedTarget: null, view: Window, detail: 0, sourceCapabilities: InputDeviceCapabilities, …}
pointerup PointerEvent {isTrusted: true, pointerId: 1, width: 1, height: 1, pressure: 0, …}
  1. Device emulation (i.e touch events):
pointerdown PointerEvent {isTrusted: true, pointerId: 2, width: 11.5, height: 11.5, pressure: 1, …}
pointerup PointerEvent {isTrusted: true, pointerId: 2, width: 11.5, height: 11.5, pressure: 0, …}
focus FocusEvent {isTrusted: true, relatedTarget: null, view: Window, detail: 0, sourceCapabilities: InputDeviceCapabilities, …}

BG_FOCUS is added on focus and removed on pointerup

The problem here is that on mobile devices pointerup is triggered before focus.

Luckily, this.activationState_.hasDeactivationUXRun flag may be checked before setting BG_FOCUS in focus event handler:

    this.listeners_ = {
      activate: (e) => this.activate_(e),
      deactivate: (e) => this.deactivate_(e),
      focus: () => requestAnimationFrame(
        () => {
          // on touch devices the "focus" event triggered with a delay
          // don't add BG_FOCUS if it's too late
          if (!this.activationState_.hasDeactivationUXRun) {
            this.adapter_.addClass(MDCRippleFoundation.cssClasses.BG_FOCUSED)
          }
        }
      ),
      blur: () => requestAnimationFrame(
        () => this.adapter_.removeClass(MDCRippleFoundation.cssClasses.BG_FOCUSED)
      ),
    };

This change affects only mouse-/touch-related focusing, I tried to:

  • tab/shift+tab into the element
  • switch to another browser tab and then return to the one with a ripple
  • activate ripple programmatically

In all cases BG_FOCUS was added correctly.

Please let me know if I should submit a PR for this change.

@kfranqueiro
Copy link
Contributor

The recent design and development work on States for hover/focus/press styles has resulted in simplifying MDC Ripple logic to no longer forcibly suppress focus styles after press (see #1784), and instead adhere more naturally to native browser behavior (since the element is still actually focused). As a result, this issue is obsolete, since there is no longer inconsistency in how focus is represented.

Thanks @anton-kachurin and @touficbatache for investigating the current behavior. It's useful to have this information for historical purposes and future reference.

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

No branches or pull requests

4 participants