feat(dropdown): add keyboard navigation support

[maxjoehnk]

Before submitting a pull request, please make sure you have at least performed the following:

• read and followed the CONTRIBUTING.md guide.
• built and tested the changes locally.
• added/updated any applicable tests.
• added/updated any applicable API documentation.
• added/updated any applicable demos.

This is a resubmit for #1417 by @wolfgangteves updated for the latest master branch

[tonte-pouncil]

So this will not be generally available until v3.3.0? What is the best way to get this feature in the meantime?

[Kumar-Aakash]

What is expected date to release V3.3.0. I think I might have missed somewhere. :)

[maxokorokov]

Hi, thanks for the PR, it's doing in the good direction!

It needs a bit of work though.
Several use cases are not taken into account.

• in bootstrap you can have arbitrary HTML and disabled items in the popup.
• some of the dropdown items might be disabled

See https://getbootstrap.com/docs/4.1/components/dropdowns/#menu-content

For example, please take a look at the dropdown implementation for bootstrap JS to be inspired:
https://github.com/twbs/bootstrap/blob/19f70f9d4ccca132f196011958c1b72462c698e7/js/src/dropdown.js#L398

It aslo might be a good idea to implement Home and End, but sure it's optional

Also some comments on style and naming, please see: https://github.com/ng-bootstrap/ng-bootstrap/wiki/Contributions%3A-Code-conventions

P.S. I haven't looked into tests yet.

Cheers

[maxjoehnk]

@pouncilt while this isn't merged you can add this git repo as your dependency. Should probably not used in production until it is merged into upstream.

[maxokorokov]

Your last changes look good, thanks!

I was looking through bootstrap demos and I think that last use case that is broken is:
https://stackblitz.com/edit/js-ahkcg3?file=index.html

It doesn't work the same way with implementation in this PR: when the form input is focused and press arrow up/down, home or end, the focus shouldn't move to any of dropdown-items.

I guess we should listen that event is triggered by either one of the trigger elements or the dropdown-item to fix this. Will you have time to take a look at it?

(Otherwise I guess it can be handled separately)

[maxjoehnk]

Yeah, seems like a good idea.

[maxokorokov]

Also @pkozlowski-opensource wanted to explore a different implementation, ex. like adding a ngbDropdownItem directive on .dropdown-item to simplify querying. I wanted to play with it to see advantages/disadvantages before merging this implementation.

[maxjoehnk]

yeah, I've actually thought about that too. I could implement it for this PR too if you want.

[maxjoehnk]

I've stumbled over a use case, which won't work in the current implementation.
I have a tag component with predefined tags, so a div with selected tags and an input which filters the menu items of a dropdown.

Now the dropdownAnchor is the surrounding div, so the dropdown width is equal to the component itself and not to the size of the input field, but with the latest changes in this PR, the dropdown can't be opened from the input field because it's not the anchor.

I could implement this behavior in my component, but I feel like this should somehow be handled by the dropdown component.

I think we should add another directive which opens the dropdown on ArrowDown. Not sure about the naming of such a directive though.

[maxokorokov]

Hey, @maxjoehnk !

I've taken a look at the last implementation, and looks like it's going to overly complex direction.

What I meant was this: #2785 (opened a PR for demonstration purposes, rough implmentation): a new directive ngbDropdownItem to enable keyboard support for buttons, links, disabled item handling

@pkozlowski-opensource could you also take a glance, please? That's what you were talking about?

I would personally:

• either introduce a new directive as in feat(dropdown): example of keyboard support #2785
• rollback to the previous implementaiton with querySelectorAll() as a much simpler on in 6d8e8be

[agenaille]

+1 bumping this. Our app needs these fixes after a 508 assessment.

[maxokorokov]

@maxjoehnk thanks a lot for working on this!
It should land in 4.1.0, I took the liberty to rebase and fix the issues after.
Also fixed forgotten export in index.ts

[codecov-io]

Codecov Report

Merging #2683 into master will increase coverage by 0.04%.
The diff coverage is 95.83%.

@@            Coverage Diff             @@
##           master    #2683      +/-   ##
==========================================
+ Coverage   92.34%   92.39%   +0.04%     
==========================================
  Files          91       91              
  Lines        2862     2907      +45     
  Branches      458      467       +9     
==========================================
+ Hits         2643     2686      +43     
- Misses        162      163       +1     
- Partials       57       58       +1

Flag	Coverage Δ
#e2e	59.99% <25%> (-0.63%)	⬇️
#unit	90.9% <95.83%> (+0.07%)	⬆️

Impacted Files	Coverage Δ
src/dropdown/dropdown.module.ts	100% <100%> (ø)	⬆️
src/dropdown/dropdown.ts	98.31% <95.74%> (-1.69%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f670ed...a6d3fab. Read the comment docs.