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

md-toolbar and sidenav focus problems #5563

Closed
devversion opened this issue Nov 4, 2015 · 14 comments · Fixed by #7965
Closed

md-toolbar and sidenav focus problems #5563

devversion opened this issue Nov 4, 2015 · 14 comments · Fixed by #7965
Assignees
Labels
has: Pull Request A PR has been created to address this issue

Comments

@devversion
Copy link
Member

For testing purposes I tried to create a sidenav, which should be triggered by the toolbar
But as you can see in the bad quality GIF, the css class is-focused will be added after dismissing the sidebar. So I already checked the code and I found this snippet

return $q(function(resolve){
  // Toggle value to force an async `updateIsOpen()` to run
  scope.isOpen = isOpen;

  $mdUtil.nextTick(function() {
    // When the current `updateIsOpen()` animation finishes
    promise.then(function(result) {

      if ( !scope.isOpen ) {
        // reset focus to originating element (if available) upon close
        triggeringElement && triggeringElement.focus();
        triggeringElement = null;
      }

      resolve(result);
    });
  });

});

gif

I already fixed that in my version, but I want to ask the development team first about their thoughts.
So I created a merge for that, please take a look at it and tell me more about potential side-affects

@jelbourn @ThomasBurleson

devversion added a commit to devversion/material that referenced this issue Nov 4, 2015
This is my first opportunity for fixing the problem with the focus of the md-button in the toolbar.
But the thing I don't understand is, this feature has a sense, so removing isn't a good an idea.
Please @Owners tell me more about that.

Possibility 1: Fixes angular#5563
devversion added a commit to devversion/material that referenced this issue Nov 4, 2015
…oring.

Thats possibility two, to fix that issue with the focus, more information here angular#5563

Fixes angular#5563
@devversion
Copy link
Member Author

I referenced my both commits :) please take a look :)

With Fix 2, you simply need to add the flag nofocus to the md-button

angular
This happens too, on the docs

@jelbourn
Copy link
Member

jelbourn commented Nov 5, 2015

cc @marcysutton

I believe that focus is there on purpose for accessibility reasons (indicates to keyboard users where the focus is).

@devversion
Copy link
Member Author

That's why I thought about creating an extra attribute which prevents focus recovery.
What do you think about that?

If you found a good solution, you can tell me :)

@marcysutton
Copy link
Contributor

Rather than providing a way to break accessibility, a proper solution for this would involve something like #5189: only show the focus style on keyboard interaction. Mouse users wouldn't see it. We definitely should not provide hooks to disable accessibility features, but rather apply those features in more specific ways.

@devversion
Copy link
Member Author

https://github.com/ten1seven/what-input

That isn't a good solution for detecting the interaction type, because then I need to bind a listener on the triggeringElement. The Problem with binding the listener to the element is: I only get the current triggeringElement after the event is already triggered, so this would be useless.

Should I instead check for the global input methods? (like: touch or mouse)

@marcysutton
Copy link
Contributor

Did you look at the library? It is checking global input methods. Consumers of the library don't have to do anything except include it and write some CSS. That said, I didn't think we would necessarily use a third-party lib in Angular Material, but I wanted to float it up as an example.

@devversion
Copy link
Member Author

Yeah, I took a look at it ;), so we should provide an own version of that with less features?

@marcysutton
Copy link
Contributor

That's the idea, yes. We don't need the full event API, just input detection allowing us to apply focus styles on keyboard interaction only.

devversion added a commit to devversion/material that referenced this issue Nov 5, 2015
Normally the Sidenav restores every time the last active element, so the focus won't disappear from the triggered button after closing the sidenav

Fixes angular#5563
@devversion
Copy link
Member Author

I created a PR, I already tested it on my local environment. Now it only restores the focus if I trigger the sidenav with keyboard.

If there are any suggestions please tell me

@ThomasBurleson ThomasBurleson added in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: work labels Nov 5, 2015
devversion added a commit to devversion/material that referenced this issue Nov 6, 2015
Sidenav want's to restore focus after the pane gets closed, this should only happen for keyboard interactions.
The service got created to use that feature for other components too

Fixes angular#5563 References angular#5583
devversion added a commit to devversion/material that referenced this issue Nov 9, 2015
Sidenav want's to restore focus after the pane gets closed, this should only happen for keyboard interactions.
The service got created to use that feature for other components too

Fixes angular#5563 References angular#5583

feat(interaction): add test for interaction which fakes a keyboard input and validates it

test(interaction): add interaction spec for mousedown event + formats 4 spaces to 2

style(interaction): fix styling of the previous commits
devversion added a commit to devversion/material that referenced this issue Nov 9, 2015
Sidenav want's to restore focus after the pane gets closed, this should only happen for keyboard interactions.
The service got created to use that feature for other components too

Fixes angular#5563 Fixes angular#5434 Closes angular#5583 Closes angular#5589

feat(interaction): add test for interaction which fakes a keyboard input and validates it

test(interaction): add interaction spec for mousedown event + formats 4 spaces to 2

style(interaction): fix styling of the previous commits
devversion added a commit to devversion/material that referenced this issue Nov 11, 2015
Sidenav want's to restore focus after the pane gets closed, this should only happen for keyboard interactions.
The service got created to use that feature for other components too

Fixes angular#5563 Fixes angular#5434 Closes angular#5583 Closes angular#5589

feat(interaction): add test for interaction which fakes a keyboard input and validates it

test(interaction): add interaction spec for mousedown event + formats 4 spaces to 2

style(interaction): fix styling of the previous commits

test(sidenav): add sidenav focus restore test triggered by keyboard

test(sidenav): add focus restore test for mouse and patch other one

fix(sidenav): test
devversion added a commit to devversion/material that referenced this issue Nov 12, 2015
Sidenav want's to restore focus after the pane gets closed, this should only happen for keyboard interactions.
The service got created to use that feature for other components too

Fixes angular#5563 Fixes angular#5434 Closes angular#5583 Closes angular#5589

feat(interaction): add test for interaction which fakes a keyboard input and validates it

test(interaction): add interaction spec for mousedown event + formats 4 spaces to 2

style(interaction): fix styling of the previous commits

test(sidenav): add sidenav focus restore test triggered by keyboard

test(sidenav): add focus restore test for mouse and patch other one

fix(sidenav): test

fix(sidenav): replace deprecated methods, and remove aria label

test(sidenav): revert deprecated methods.
@ThomasBurleson ThomasBurleson modified the milestones: 1.0-rc4, 1.0-rc5 Nov 16, 2015
@ThomasBurleson ThomasBurleson modified the milestones: 1.0-rc5, 1.0-rc8 Nov 25, 2015
@cmacdonnacha
Copy link

Hey,

Just wondering what the status on this? I'm currently seeing the same behavior where focus is still set to the button after closing the sidenav.

Thanks!

@devversion
Copy link
Member Author

@cmacdonnacha Yeah, it's unfortunately still pending.

@cmacdonnacha
Copy link

@devversion Thanks for the update.

@ThomasBurleson ThomasBurleson added has: Pull Request A PR has been created to address this issue and removed pr: merge ready This PR is ready for a caretaker to review labels Aug 24, 2016
@ThomasBurleson
Copy link
Contributor

@devversion - what is the PR associated with this issue ?

@devversion
Copy link
Member Author

@ThomasBurleson - #7965

devversion added a commit to devversion/material that referenced this issue Aug 26, 2016
devversion added a commit to devversion/material that referenced this issue Sep 19, 2016
@Splaktar Splaktar removed this from the - Backlog milestone Feb 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
has: Pull Request A PR has been created to address this issue
Projects
None yet
7 participants