Skip to content
This repository has been archived by the owner on Sep 5, 2024. It is now read-only.

nav-bar: clicking a tab causes focus to move to a previously selected tab #11591

Closed
rohit1018 opened this issue Jan 10, 2019 · 12 comments · Fixed by #11600
Closed

nav-bar: clicking a tab causes focus to move to a previously selected tab #11591

rohit1018 opened this issue Jan 10, 2019 · 12 comments · Fixed by #11600
Assignees
@Splaktar
Labels
for: internal contributor The team will address this issue and community PRs are not requested. has: Pull Request A PR has been created to address this issue P1: urgent Urgent issues that should be addressed in the next minor or patch release. resolution: fixed severity: regression This issue is related to a regression type: bug
Milestone
1.1.13

Comments

@rohit1018
Copy link

rohit1018 commented Jan 10, 2019 •
edited
Loading

Detailed Reproduction Steps:

  1. Click on below md-nav-bar items
    https://material.angularjs.org/latest/demo/navBar

What is the expected behavior?

md-nav-bar focus remove on previously selected nav-item

What is the current behavior?

md-nav-bar focus shows on previously selected nav-item

Which versions of AngularJS, Material, OS, and browsers are affected?

  • AngularJS: 1.5.11
  • AngularJS MateriaC: 1.1.12
  • OS: Mac
  • Browsers: Chrome

Is there anything else we should know? Stack Traces, Screenshots, etc.

screen shot

https://material.angularjs.org/latest/demo/navBar
@codymikol
Copy link
Contributor

Your codepen is not valid, but this is reproducible in the docs here https://material.angularjs.org/latest/demo/navBar

@codymikol
Copy link
Contributor

codymikol commented Jan 10, 2019 •
edited
Loading

Looks like this is a regression introduced in 1.1.11

This is the commit that introduced the regression: 4d29450

@codymikol
Copy link
Contributor

codymikol commented Jan 10, 2019 •
edited
Loading

I've Isolated this to a unit test, now working on a fix.

This is actually a difficult issue, it looks like the focus handling is fired immediately, but there are a bunch of timeouts on updating the actual state of the tabs so we have to wait for that to complete before setting focus.

It also looks like the updating functionality is called inside of a timeout and then internally it also calls a bunch of timeouts.

EDIT: This completely fixes the issue and I absolutely cannot see anything wrong with this

image

20 timeouts didn't seem to do the trick, but 30 works great.

(Obviously this is a joke, please don't do this)

Still working on an elegant solution, hopefully I'll have more luck tomorrow.

@rohit1018
Copy link
Author

I've Isolated this to a unit test, now working on a fix.

This is actually a difficult issue, it looks like the focus handling is fired immediately, but there are a bunch of timeouts on updating the actual state of the tabs so we have to wait for that to complete before setting focus.

It also looks like the updating functionality is called inside of a timeout and then internally it also calls a bunch of timeouts.

EDIT: This completely fixes the issue and I absolutely cannot see anything wrong with this

image

20 timeouts didn't seem to do the trick, but 30 works great.

(Obviously this is a joke, please don't do this)

Still working on an elegant solution, hopefully I'll have more luck tomorrow.

Okay thank you

@Splaktar Splaktar changed the title md-nav-bar focus shows on previously selected nav-item nav-bar: focus shows on previously selected nav-item Jan 11, 2019
@Splaktar Splaktar self-assigned this Jan 11, 2019
@Splaktar Splaktar added the needs: more info The issue does not contain enough information for the team to determine if it is a real bug label Jan 11, 2019
@Splaktar
Copy link
Member

Thank you for taking the time to submit this issue. However it does not follow our required issue template. Providing a CodePen demo or link to a demo in the docs along with reproduction steps is required. This gives the team the information needed to investigate the bug or prioritize the feature.

Please provide reproduction steps as I do not see an issue with the existing demo. Note that pressing the arrow keys only moves the focus, it does not change the tab selection. You need to press the space bar for the tab to be selected.

Also note that the focus is indicated by the darker background and the selection is indicated by the colored label and inkbar.

@codymikol
Copy link
Contributor

codymikol commented Jan 11, 2019 •
edited
Loading

@Splaktar you can view the regression here

Just click on a nav-bar button and you will see that the focus is pulled onto the previously selected tab.

Browser: Chrome v71
OS: Mac OS Mojave 10.14
AngularJS: 1.7.4
Angular Material: 1.1.12

nav-regression

@Splaktar
Copy link
Member

OK, thank you, it is helpful to know that clicking the mouse is required to reproduce this issue. It looks like in 1.1.10 the focus decoration would disappear, but we don't want that to happen for a11y reasons. But it certainly should stay on the clicked tab and not jump away like that.

@Splaktar Splaktar added needs: investigation The cause of this issue is not well understood and needs to be investigated by the team or community severity: regression This issue is related to a regression ux: polish P1: urgent Urgent issues that should be addressed in the next minor or patch release. and removed needs: more info The issue does not contain enough information for the team to determine if it is a real bug labels Jan 11, 2019
@Splaktar Splaktar added this to the 1.1.13 milestone Jan 11, 2019
@codymikol
Copy link
Contributor

I have a branch that isolates this into a test and part of a fix prepared, I'll probably be able to wrap it up tonight.

@Splaktar Splaktar changed the title nav-bar: focus shows on previously selected nav-item nav-bar: clicking a tab causes focus to move to a previously selected tab Jan 11, 2019
@Splaktar
Copy link
Member

@codymikol awesome, thank you for looking into this!

@Splaktar
Copy link
Member

@codymikol any progress on this? Do you want me to take a look at what you have so far?

codymikol added a commit to codymikol/material that referenced this issue Jan 15, 2019
@codymikol
fix(navbar): fix navbar focus on click
3fc4584 
add a $timeout that will allow enough time for the click event to
complete so the focus event can properly evaluate

Fixes: angular#11591
@codymikol codymikol mentioned this issue Jan 15, 2019
Closed
3 tasks
@codymikol
Copy link
Contributor

@Splaktar That should fix it. It's really strange, apparently there is just a significant delay from when focus events are fired to when click events are fired.

codymikol added a commit to codymikol/material that referenced this issue Jan 15, 2019
@codymikol
fix(navbar): fix navbar focus on click
31ea069 
add a $timeout that will allow enough time for the click event to
complete so the focus event can properly evaluate

Fixes: angular#11591
@Splaktar Splaktar added has: Pull Request A PR has been created to address this issue and removed needs: Pull Request labels Jan 15, 2019
Splaktar added a commit that referenced this issue Jan 21, 2019
@Splaktar
fix(nav-bar): improve focus behavior for click events
982bd04 
remove hover style that is inconsistent with spec

Fixes #11591. Relates to #11494. Closes #11598.
@Splaktar Splaktar added the for: internal contributor The team will address this issue and community PRs are not requested. label Jan 21, 2019
@Splaktar Splaktar added type: bug and removed needs: investigation The cause of this issue is not well understood and needs to be investigated by the team or community ux: polish labels Jan 21, 2019
@Splaktar Splaktar mentioned this issue Jan 21, 2019
Merged
3 tasks
@Splaktar
Copy link
Member

PR #11600 takes a different approach at fixing this w/o an extra timeout. Please take a look.

Splaktar added a commit that referenced this issue Jan 22, 2019
@Splaktar
fix(nav-bar): improve focus behavior for click events
86aeb74 
remove hover style that is inconsistent with spec

Fixes #11591. Relates to #11494. Closes #11598.
mmalerba pushed a commit that referenced this issue Feb 1, 2019
@Splaktar @mmalerba
fix(nav-bar): improve focus behavior for click events (#11600)
e64875d 
<!-- 
Filling out this template is required! Do not delete it when submitting a Pull Request! Without this information, your Pull Request may be auto-closed.
-->
## PR Checklist
Please check that your PR fulfills the following requirements:
- [x] The commit message follows [our guidelines](https://github.com/angular/material/blob/master/.github/CONTRIBUTING.md#-commit-message-format)
- [x] Tests for the changes have been added or this is not a bug fix / enhancement
- [x] Docs have been added, updated, or were not required

## PR Type
What kind of change does this PR introduce?
<!-- Please check the one that applies to this PR using "x". -->
```
[x] Bugfix
[ ] Enhancement
[ ] Documentation content changes
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Infrastructure changes
[ ] Other... Please describe:
```

## What is the current behavior?
Clicking on nav items with a mouse causes the focus to move to the previously clicked nav item instead of the newly clicked nav item.
<!-- Please describe the current behavior that you are modifying and link to one or more relevant issues. -->
Issue Number: 
Fixes #11591. Relates to #11494. Closes #11598.

## What is the new behavior?
- Focus is moved to the new nav item after it is clicked.
- remove hover style that is inconsistent with spec
- add test from @codymikol 

## Does this PR introduce a breaking change?
```
[ ] Yes
[x] No
```
<!-- If this PR contains a breaking change, please describe the impact and migration path for existing applications below. -->
<!-- Note that breaking changes are highly unlikely to get merged to master unless the validation is clear and the use case is critical. -->

## Other information
Thank you to @codymikol for doing the initial investigation into this regression!
@codymikol codymikol mentioned this issue Feb 6, 2019
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@Splaktar Splaktar

Labels
for: internal contributor The team will address this issue and community PRs are not requested. has: Pull Request A PR has been created to address this issue P1: urgent Urgent issues that should be addressed in the next minor or patch release. resolution: fixed severity: regression This issue is related to a regression type: bug
Projects
None yet
Milestone
1.1.13
3 participants
@Splaktar @codymikol @rohit1018