material icon indicating copy to clipboard operation
material copied to clipboard

select: use `$document[0].activeElement` instead of `opts.focusedNode`

Open epelc opened this issue 7 years ago • 5 comments

Actual Behavior:

  • What is the issue? *
    mdSelectMenu uses a local variable to keep track of the currently focused element instead of using the builtin document.activeElement. opts.focusedNode is not exposed to the outside world. This means that if you need to handle up/down arrow events yourself for moving between a search input in the header down to your list of options you can not fix a couple edge cases.

You end up with the down arrow jumping from your list to just under the last focused node, with no way to fix it besides handling all up/down events for the whole md-select.

  • What is the expected behavior?
    I'm pretty sure there is no reason mdSelectMenu needs to use a local variable(opts.focusedNode) to keep track of the currently focused node. It can easily be switched to document.activeElement($document[0].activeElement in angular). This would allow 3rd parties to handle a subset of the keyboard events themselves.

https://developer.mozilla.org/en-US/docs/Web/API/Document/activeElement

CodePen (or steps to reproduce the issue): *

  • CodePen Demo which shows your issue:
  • Details: No codepen here. But my use case is making the up/down arrows work when you put a search input in an md-select-header.

@ThomasBurleson please advise. I'll make a pr to switch to $document[0].activeElement if you agree that is the right course of action. The other option is to expose opts.focusedNode somehow. I think the first way would be better unless I'm overlooking some reason why opts.focusedNode was used in the first place(fix some potential bug maybe?).

Angular Versions: *

  • Angular Version: 1.5.8
  • Angular Material Version: 1.1.1

Additional Information:

  • Browser Type: * chrome
  • Browser Version: * 53
  • OS: * osx 10.11.6
  • Stack Traces:

Shortcut to create a new CodePen Demo. Note: * indicates required information. Without this information, your issue may be auto-closed.

Do not modify the titles or questions. Simply add your responses to the ends of the questions. Add more lines if needed.

epelc avatar Sep 06 '16 17:09 epelc

@epelc where did you see references to this? I tried searching the entire codebase for focusNode, but couldn't find anything.

crisbeto avatar Oct 18 '16 19:10 crisbeto

@crisbeto Ops, I miss spelled the title. It's opts.focusedNode See all over this file/line here.

Search for focusedNode or opts.focusedNode. It is basically reproducing what window.activeElement handles natively(but not exposing it).

epelc avatar Oct 18 '16 19:10 epelc

Thanks, that looks weird.

crisbeto avatar Oct 18 '16 19:10 crisbeto

@epelc sorry for the delay on this. I would be happy to presubmit a PR from you that made this change. I agree that it sounds like a better approach. We just need to verify that it doesn't cause any breaking changes.

Splaktar avatar Jul 05 '18 20:07 Splaktar

@splaktar no worries. I’m not using this project anymore(using all angular 2+ now). I think i ended up upgrading the component that needed this to ng2 because at the time I was half angularjs and half angular.

epelc avatar Jul 05 '18 20:07 epelc