-
Notifications
You must be signed in to change notification settings - Fork 55
Conversation
Codecov Report
@@ Coverage Diff @@
## master #413 +/- ##
==========================================
- Coverage 90.41% 88.96% -1.46%
==========================================
Files 41 41
Lines 1356 1386 +30
Branches 173 177 +4
==========================================
+ Hits 1226 1233 +7
- Misses 126 149 +23
Partials 4 4
Continue to review full report at Codecov.
|
src/components/List/List.tsx
Outdated
let maybeSelectableItemProps = {} | ||
|
||
if (this.props.selection) { | ||
const _ref = React.createRef() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some time ago we had a discussion in other PR when underscore was used for a local variable as here and we agreed that it should be removed :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, will remove
src/components/List/List.tsx
Outdated
return _.map(items, (item, idx) => { | ||
itemProps.focusableItemProps = this.focusContainer.createItemProps(idx, items.length) | ||
let maybeSelectableItemProps = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, you may initialize it as
const maybeSelectableItemProps = {}
and then on line 145
maybeSelectableItemProps.tabIndex = 0
maybeSelectableItemProps.ref = _ref
maybeSelectableItemProps.onFocus = () => this.focusHandler.syncFocusedItemIndex(idx)
So no rebinding will happen for the object
src/lib/getKeyDownHandlers.ts
Outdated
eventHandler && eventHandler(event) | ||
|
||
if (eventHandler) { | ||
event.preventDefault() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
having concerns about this line. In FocusZone onKeyDown
handler checks if the event is preventDefaulted
, and if yes, it will do nothing. Just to imagine, let's say the parent container of the component is wrapped with FocusZone, and the same key event is propagated to FocusZone, it won't work as expected.
Indeed, in some cases we need to preventDefault. If you look at chatMessageBehavior
, we have done it using keyActions
keyActions: {
root: {
// prevents default FocusZone behavior, in this case, prevents using arrow keys as navigation (we only want a Tab key to navigate)
preventDefault: {
keyCombinations: [{ keyCode: keyboardKey.ArrowUp }, { keyCode: keyboardKey.ArrowDown }],
},
},
},
So to wrap up my thought, it's not really safe to preventDefault
for all the cases.
We consider making it optional, by passing extra param or using key actions as described above.
Please, let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this change is introduced to prevent scrolling of the page when one would click up-down keys for selectable List (please, take a look on the corresponding example on the Stardust page we have now to see this problem) - each keypress results not just in switching list item, but also in scrolling the page. This is not a desired effect for sure.
Solutions? While we might consider to make preventDefault()
calls for each action handler in the List component, it seems to be redundant. The logic where we have check that client has provided handler for the event seems to be a clear suggestion that this event will be handled by client. So, by default I would definitely expect that event with handler defined will stop bubbling at the point where it is handled.
Just to imagine, let's say the parent container of the component is wrapped with FocusZone, and the same key event is propagated to FocusZone, it won't work as expected.
Sorry, could you, please, suggest what the case is - cannot understand what this is about. Currently have the following picture from these words, but not sure that have got an idea. Maybe you could suggest code example, or further elaborate on the case youare talking about?
<FocusZone>
<Container>
<Element /> <--- here is where event raised
</Container>
</FocusZone>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me try to explain from a different side.
I've tried to make this change in the code and tested the chat prototype. Chat is no more navigable with arrow keys.
The reason for that:
Chat has defined keyActions
in chatBehavior
focus: {
keyCombinations: [{ keyCode: keyboardKey.ArrowLeft }],
},
When we press Arrow up or down on a chat, next happens:
onKeyDown
handler is triggered ingetKeyDownHandler.ts
- after mapping action names to key actions, we've got our handler, which contains filtering logic with keys inside it
event.preventDefault
is called for any keyboard event and any key- the event goes to FocusZone where it checks next
if (ev.isDefaultPrevented()) {
return undefined
}
- so navigation won't work in FocusZone
If we want to stick with event.preventDefault
, we have to move it to keyboardHandlerFilter.ts
, line 19, so it will look like
if (isHandled) {
event.preventDefault()
handler(event)
}
So then event.preventDefault
will be called only for specific keys, when the keyboard handler is triggered.
But still, I am afraid if we miss something and it can break some other use case. I might vote for having it as an optional param.
@kuzhelov Let me know what you think and if you get the idea or we can talk about it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
preventDefault should not be there by default. It should be part of each event handler that requires it. I understand that it requires users to write a bit more code and have this understanding, but it also gives them flexibility in how they want to write their handlers and where do they want to catch events.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree, let me address that
return { focusItemOnIdx: prev.focusItemOnIdx - 1 } | ||
}) | ||
this._focusedItemIndex -= 1 | ||
this.coerceFocusedItemIndex() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a suggestion, maybe we can change coerce
--> constrain
, as it seems more often used for describing such cases.
P.S. I learned one more English word, thank you :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls address comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed this locally and make sense, although I was expecting to be able to circularly navigate throughout the list, but not sure if we want to support this (not saying that we should).
Yes, we should, but this wasn't supported yet. The main goal of this PR was to address the problem of the existing functionality (#370 (comment)), and this circular traversing behavior wasn't supported there as well. See this functionality as something that will be introduced by means of a dedicated PR. |
@kuzhelov I did not investigate further, but with this PR you can use TAB to go throgh the selectable list item. That should not be the case, you should only be able to use the arrow keys. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as mentioned in the comment, with this PR user can go through the list items using TAB.
@jurokapsiar, going to merge these changes, as original ones haven't introduced any keyboard handling capabilities (have checked for master branch). As we are not making things any worse in this aspect, as well as fixing the problems outlined here (#370 (comment)), it seems to be reasonable to introduce fix as a first step. For TAB support - will file an issue, open to have a discussion in regards to approach that is better to be taken for that. |
TODO
This PR aims to address List focus issues, as well as simplify generic logic used for collection's focus behavior. The problems that were mentioned in the following post (#370 (comment)) are addressed, specifically:
findDOMNode
should be avoided at all costs. - avoided 👍this.focusableItem()
which is created by passingthis
to a constructor. Passing instances around is code smell - this smell has been eliminated, only safe immutable data is passed to constructor now 👍focusableItemProps
that doesn't make sense to consumers and should probably be removed. - removed, and, generally speaking, provided focus behavior introduces no changes to public component's API now 👍selection
list, but thethis.focusableItem()
is applied to every instance of the list - this differentiation is made 👍componentDidUpdate
code in the List, especially since it is stateless. - there is no any now 👍