feat: implement F6 Navigation Helper #4490
Conversation
There was a problem hiding this comment.
Additional remarks:
- If there is OpenUI5 support enabled, we must disable our implementation completely, right? I mean, OpenUI5 is going to attach the listener and do the navigation, we only need to mark our components as fast-navigation-ready. This would mean that inside the F6Navigation feature, we call
getFeature("OpenUI5Support")and if it is registered (i.e. the app is also using OpenUI5) we return early and don't run the instance. Question that follows is: would it work out of the box with OpenUI5's fast navigation for our components? - Currently there is no fast-navigation marking inside shadow roots. I think that's OK but I want to ask again to be sure I understand the requirement completely: are there use cases when developers would want to define 2 or more fast navigation groups inside a single web component (meaning we'd need to set the marker inside the shadow roots)
There was a problem hiding this comment.
Overall:
- please comment on my previous note about OpenUI5. How should it behave if we have both? You can test easily by opening
kitchen.openui5.htmland adding (some more openui5 controls that have F6 handling). Also test by not having any OpenUI5 controls, but our F6 navigation logic. Test a combination of both. It might be necessary to disable our logic if OpenUI5 is included - I did not checkout the change, but did you test if there are no F6 groups, but the feature is imported? The whole code should be shielded from JS errors in that case.
| // Here for both FCL & List the firstFoccusableElement is the same (the ui5-li) | ||
|
|
||
| const firstFocusable = await getFirstFocusableElement(this.groups[nextIndex - 1]); | ||
| const shouldSkipParent = firstFocusable === await getFirstFocusableElement(this.groups[nextIndex]); |
There was a problem hiding this comment.
This looks complicated. Wouldn't it be easier to filter out duplicated DOM elements from this.groups when you construct them (for example after .filter run another .filter that removes all that are already in this.groups)? Even if we dont Also, it is possible that there might be 3 or more groups with the same situation and here you skip only 1 level.
There was a problem hiding this comment.
I don't think that it is a good idea to run this check for each group on the page. In a page built with ui5 web components, we will have 30+ groups and it won't be efficient as this operation will run on every keydown event.
I agree when it comes to the nesting levels. However, a case with more than 2 levels of nesting is not feasible at this point.
| @@ -20,24 +20,39 @@ const attachBoot = listener => { | |||
| }; | |||
|
|
|||
| const boot = async () => { | |||
| if (booted) { | |||
| return; | |||
| if (bootPromise) { | |||
There was a problem hiding this comment.
@pskelin This is the change we want to revert I told you about yesterday
| let nextElement = null; | ||
|
|
||
| if (nextIndex > -1) { | ||
| if (nextIndex - 1 < 0) { |
There was a problem hiding this comment.
Something must be wrong here. On line 50 there is nextIndex > -1 so if we reach line 51, nextIndex must be 0 or higher, therefore the check nextIndex - 1 < 0 can never be true, because this is the same as nextIndex < -1
There was a problem hiding this comment.
This if covers the case where nextIndex is 0.
For nextIndex = 0, we have:
nextIndex > -1: true
nextIndex - 1 < 0: true
For nextIndex = 1, we have:
nextIndex > -1: true
nextIndex - 1 < 0: false
There was a problem hiding this comment.
| if (nextIndex - 1 < 0) { | |
| if (nextIndex - 1 === 0) { |
This PR implements the Fast Navigation Feature:
F6(Ctrl+Alt+Arrow Down) andShift+F6(Ctrl+Alt+Arrow Up)Part of #4454