Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(arrowNavigation) - update way to listen for focus/blur events #10326

Merged
merged 3 commits into from
Aug 30, 2023

Conversation

Antreesy
Copy link
Contributor

☑️ Resolves

  • Regression from fix(LeftSidebar) redo arrow navigation #9980
  • Before composable add new eventListener each time (on navigation initialization => after each fetch). Thus some items could receive several event listeners from it. Now it keep listeners in memory to remove them later (on reset method => before fetch).
  • Before if search text was changed, and first item was removed from the list, but navigation is not initialized yet, on Enter key code tries to click on removed element, which causes page reload - fixed with additional check

🖼️ Screenshots

No visual changes

🚧 Tasks

  • Manual testing
  • Code review

🏁 Checklist

@Antreesy
Copy link
Contributor Author

/backport to stable27

@nickvergessen

This comment was marked as resolved.

Copy link
Contributor

@DorraJaouad DorraJaouad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing is blocking, except for a suggestion for a possible improvement.

src/components/LeftSidebar/LeftSidebar.vue Outdated Show resolved Hide resolved
@Antreesy Antreesy force-pushed the fix/noid/arrow-navigation-events branch from 2f00be9 to bdeedc8 Compare August 27, 2023 12:29
@Antreesy Antreesy changed the title fix(arrowNavigation) - remove old listeners from items fix(arrowNavigation) - update way to listen for focus/blur events Aug 27, 2023
Copy link
Contributor

@DorraJaouad DorraJaouad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better now but ..

src/composables/useArrowNavigation.js Outdated Show resolved Hide resolved
src/composables/useArrowNavigation.js Outdated Show resolved Hide resolved
Copy link
Contributor

@DorraJaouad DorraJaouad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, it seems ready now :

  • Navigation including tabbing and keys works correctly.
  • Mouse focuses are processed in the same way whether they are outside or on SearchBox.
  • No more extra listeners reassignments to conversations.

Tested on both lists and it functions as expected 🦅

src/composables/useArrowNavigation.js Show resolved Hide resolved
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@Antreesy Antreesy force-pushed the fix/noid/arrow-navigation-events branch from 115a0e1 to b513640 Compare August 29, 2023 12:56
@@ -20,7 +20,8 @@
-->

<template>
<li class="participant-row"
<li :id="`${participant.source}_${participant.id}`"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really required to be id here and anchor in the LeftSidebar?

ID must be unique on a whole page, and I'm really not sure that IDs like user_${item.id} are guaranteed to be unique across all applications on the page.

If it is actually only used as additional Node data to get item id from event.target?.id can we use some custom attr here? For example, data-arrow-navigation-id or at least data-id?

P.S. duplicated IDs is one of the most large a11y issue we currently have in Nextcloud...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the LeftSidebar they are used by RouterLink in <Conversation/> components, but not for the <NcListItem/> and <Participant/>
So I think, we could find a way to avoid it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the LeftSidebar they are used by RouterLink in <Conversation/>

But they were not used in Talk, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Router works without them, and I couldn't find anythig related or based on DOM anchor element id

They have an empty id attribute, but that's an upstream regression of NcListItem
image

Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@Antreesy Antreesy requested a review from ShGKme August 29, 2023 15:29
Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine by me with change id to data-nav-id.

But I'm still not a fun of mixing vanilla DOM manipulation with Vue.

Not tested.

@Antreesy Antreesy merged commit 0ba3105 into master Aug 30, 2023
22 checks passed
@Antreesy Antreesy deleted the fix/noid/arrow-navigation-events branch August 30, 2023 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants