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

Conversation

codymikol
Copy link
Contributor

add a $timeout that will allow enough time for the click event to
complete so the focus event can properly evaluate

Fixes: #11591

PR Checklist

Please check that your PR fulfills the following requirements:

  • The commit message follows our guidelines
  • Tests for the changes have been added or this is not a bug fix / enhancement
  • Docs have been added, updated, or were not required

PR Type

What kind of change does this PR introduce?

[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?

Currently clicking on a navbar tab will select it, but pull focus to the previously selected tab.

Issue Number: #11591

What is the new behavior?

Now the click event will evaluate before the focus event, updating the state which is necessary to set the focus styling.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

I spent a lot of time spinning my wheels on this, it turns out that there is just a significant delay from when the focus event is fired and when the click event is fired.

@googlebot googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Jan 15, 2019
@codymikol
Copy link
Contributor Author

Looks like the project needs a full lint, travis failed because of something in autocomplete.

@codymikol
Copy link
Contributor Author

I've also realized that this fix does not work 100% of the time. It looks like sometimes it can take more than 100ms between the focus and click events. @Splaktar do you know of any other instances in the project that have to work around this quirk?

@codymikol codymikol changed the title fix(navbar): fix navbar focus on click WIP: fix(navbar): fix navbar focus on click Jan 15, 2019
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 force-pushed the fix-material-nav-bar branch from 3fc4584 to 31ea069 Compare January 15, 2019 04:03
@codymikol codymikol changed the title WIP: fix(navbar): fix navbar focus on click fix(navbar): fix navbar focus on click Jan 15, 2019
@codymikol
Copy link
Contributor Author

It seems to work perfectly with a 200ms delay, but it does feel a bit "wrong" to rely on a timeout to work around this behavior 🤷‍♂️

@Splaktar Splaktar self-assigned this Jan 15, 2019
@Splaktar Splaktar self-requested a review January 15, 2019 22:59
@Splaktar
Copy link
Contributor

The lint issue is fixed in master. If you rebase it will be resolved.

I will have to dig into this a bit deeper next week. I agree with your assessment that we should find a better way than just a longer timeout, if possible.

@Splaktar Splaktar added P1: urgent Urgent issues that should be addressed in the next minor or patch release. in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs severity: regression This issue is related to a regression labels Jan 15, 2019
Splaktar added a commit that referenced this pull request Jan 21, 2019
remove hover style that is inconsistent with spec

Fixes #11591. Relates to #11494. Closes #11598.
@Splaktar
Copy link
Contributor

Splaktar commented Jan 21, 2019

I think that I found a better way in PR #11600. Please review, try it out if possible, and let me know what you think.

Copy link
Contributor

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with your assessment that we should find a better way than just a longer timeout, if possible.

Splaktar added a commit that referenced this pull request Jan 22, 2019
remove hover style that is inconsistent with spec

Fixes #11591. Relates to #11494. Closes #11598.
@codymikol
Copy link
Contributor Author

Closing as you've come up with a much better solution 🐔

@codymikol codymikol closed this Jan 22, 2019
mmalerba pushed a commit that referenced this pull request Feb 1, 2019
<!-- 
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!
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs P1: urgent Urgent issues that should be addressed in the next minor or patch release. severity: regression This issue is related to a regression
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nav-bar: clicking a tab causes focus to move to a previously selected tab
3 participants