fix(input): prevent md-select-on-focus from refocusing blurred input #11129

jonbcard · 2018-02-20T05:36:43Z

Eliminate race condition that causes a blurred input with md-select-on-focus from being re-selected.
When there are two inputs with md-select-on-focus this can result in an infinite loop where focus
moves back and forth between the two elements.

PR Checklist

Please check that your PR fulfills the following requirements:

The commit message follows our guidelines
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

md-select-on-focus will re-focus an input, even if it is no longer the active element on the page in order to select the input text. The most bizarre behaviour that this can produce is when there are two fields with md-select-on-focus that end up competing for focus in an infinite loop.

Here is a codepen with instructions to show the issue:
https://codepen.io/jonbcard/pen/PQQwLG

Issue Number: #11015

What is the new behavior?

md-select-on-focus will now only attempt to select the text of the field if it still retains focus.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Splaktar

Thank you for your contribution! This looks really useful! I only made a few minor comments that will hopefully be easy to resolve. Then we can look at getting this merged 😄

Splaktar · 2018-02-21T08:11:40Z

src/components/input/input.js

-        // Use HTMLInputElement#select to fix firefox select issues.
-        // The debounce is here for Edge's sake, otherwise the selection doesn't work.
-        element[0].select();
+        if(document.activeElement === element[0]) {


Can we use $document here and inject it above?

Can you please add a brief comment here explaining why this check is needed?

Splaktar · 2018-02-21T08:12:37Z

src/components/input/input.spec.js

@@ -659,6 +659,23 @@ describe('md-input-container directive', function() {
      }
    }));

+    it('should not refocus the input after focus is lost', inject(function($timeout) {


Same thing, can we inject/use $document here?

jonbcard · 2018-02-21T17:00:55Z

Thanks for taking the time to look this over @Splaktar and for the feedback! I'll make the updates, rebase, and get the updates pushed later today.

googlebot · 2018-02-22T01:04:19Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

Eliminate race condition that causes a blurred input with md-select-on-focus from being re-selected. When there are two inputs with md-select-on-focus this can result in an infinite loop where focus moves back and forth between the two elements.

jonbcard · 2018-02-22T01:42:37Z

Changes have been made as suggested. Really sorry for any noise I generated while I tried to work through a git-mess-up-turned-git-disaster / Failed TravisCI builds. Phew!

jonbcard · 2018-02-25T15:59:43Z

Just looking at googlebot's comment; not sure why it flipped the CLA status from "yes" to "no" (probably something I accidentally pushed while I was trying to rebase the pull request). To be clear, I have signed the CLA and am the only committer on this PR.

Splaktar

LGTM

Splaktar · 2018-02-26T00:34:07Z

Looks good. No problem with the git stuff. I know it happens 😄

…ngular#11129) Eliminate race condition that causes a blurred input with md-select-on-focus from being re-selected. When there are two inputs with md-select-on-focus this can result in an infinite loop where focus moves back and forth between the two elements.

…11129) Eliminate race condition that causes a blurred input with md-select-on-focus from being re-selected. When there are two inputs with md-select-on-focus this can result in an infinite loop where focus moves back and forth between the two elements.

googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Feb 20, 2018

Splaktar reviewed Feb 21, 2018

View reviewed changes

Splaktar self-assigned this Feb 21, 2018

Splaktar added this to the 1.1.8 milestone Feb 21, 2018

Splaktar added in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs ux: integration P2: required Issues that must be fixed. labels Feb 21, 2018

jonbcard force-pushed the fix-input-focus branch 3 times, most recently from dd9326e to 38b82ed Compare February 22, 2018 01:04

googlebot added cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ and removed cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ labels Feb 22, 2018

jonbcard force-pushed the fix-input-focus branch 2 times, most recently from 84ee450 to ed6f840 Compare February 22, 2018 01:15

jonbcard force-pushed the fix-input-focus branch from ed6f840 to 314d751 Compare February 22, 2018 01:34

Splaktar added cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ and removed cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ labels Feb 26, 2018

Splaktar approved these changes Feb 26, 2018

View reviewed changes

Splaktar added pr: merge ready This PR is ready for a caretaker to review pr: lgtm This PR has been approved by the reviewer and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels Feb 26, 2018

Splaktar assigned tinayuangao Feb 26, 2018

tinayuangao merged commit c5ec316 into angular:master Feb 27, 2018

GulajavaMinistudio mentioned this pull request Feb 28, 2018

Update upstream GulajavaMinistudio/material#41

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(input): prevent md-select-on-focus from refocusing blurred input #11129

fix(input): prevent md-select-on-focus from refocusing blurred input #11129

Uh oh!

jonbcard commented Feb 20, 2018 •

edited by Splaktar

Loading

Uh oh!

Splaktar left a comment

Uh oh!

Splaktar Feb 21, 2018

Uh oh!

Splaktar Feb 21, 2018

Uh oh!

Splaktar Feb 21, 2018

Uh oh!

jonbcard commented Feb 21, 2018

Uh oh!

googlebot commented Feb 22, 2018

Uh oh!

jonbcard commented Feb 22, 2018

Uh oh!

jonbcard commented Feb 25, 2018

Uh oh!

Splaktar left a comment

Uh oh!

Splaktar commented Feb 26, 2018

Uh oh!

Uh oh!

fix(input): prevent md-select-on-focus from refocusing blurred input #11129

fix(input): prevent md-select-on-focus from refocusing blurred input #11129

Uh oh!

Conversation

jonbcard commented Feb 20, 2018 • edited by Splaktar Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

Splaktar left a comment

Choose a reason for hiding this comment

Uh oh!

Splaktar Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

Splaktar Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

Splaktar Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

jonbcard commented Feb 21, 2018

Uh oh!

googlebot commented Feb 22, 2018

Uh oh!

jonbcard commented Feb 22, 2018

Uh oh!

jonbcard commented Feb 25, 2018

Uh oh!

Splaktar left a comment

Choose a reason for hiding this comment

Uh oh!

Splaktar commented Feb 26, 2018

Uh oh!

Uh oh!

jonbcard commented Feb 20, 2018 •

edited by Splaktar

Loading