[FIX] website: Restore arrow key navigation in search bar #4922
Conversation
* = website_sale, website_forum, website_blog,... Co-authored-by: Divyesh Vyas <[email protected]>
|
This PR targets the un-managed branch odoo-dev/odoo:master-split-results-dtda, it needs to be retargeted before it can be merged. |
divy-odoo
force-pushed
the
master-split-results-dtda
branch
from
6ae593b to
ee7dcef
Compare
shsa-odoo
force-pushed
the
master-split-search-keydown-issue-shsa
branch
from
be0f77f to
8b6213a
Compare
divy-odoo
force-pushed
the
master-split-results-dtda
branch
2 times, most recently
from
f4b55e0 to
eac9afc
Compare
shsa-odoo
force-pushed
the
master-split-search-keydown-issue-shsa
branch
from
8b6213a to
7ec22d0
Compare
divy-odoo
force-pushed
the
master-split-results-dtda
branch
from
eac9afc to
a57de63
Compare
Issue: Arrow key navigation for search result items regressed with the new search bar layout. The previous logic in onKeydown was no longer triggered due to the updated DOM structure. Fix: Introduce a dedicated onSearchResultKeydown handler and wire it to search result items. The new method restores ArrowUp and ArrowDown focus management while keeping the existing input keydown behavior unchanged. Added a complete test case to cover arrow key navigation in search results. task-5424392
shsa-odoo
force-pushed
the
master-split-search-keydown-issue-shsa
branch
from
7ec22d0 to
653d3be
Compare
dtda-odoo
force-pushed
the
master-split-results-dtda
branch
from
a57de63 to
d9a91c8
Compare
divy-odoo
left a comment
divy-odoo
left a comment
There was a problem hiding this comment.
Hello @shsa-odoo, Here is my first round of review.
| color: inherit; | ||
| } | ||
| } | ||
| a { |
There was a problem hiding this comment.
May be you want to select direct child only.
| a { | |
| > a { |
| <template id="one_hybrid" name="Single any Search Results"> | ||
| <a t-att-href="result.get('website_url')" class="dropdown-item p-2 text-wrap"> | ||
| <div class="d-flex align-items-center flex-wrap o_search_result_item o_cc o_cc1 text-decoration-none"> | ||
| <div class="d-flex align-items-center flex-wrap o_search_result_item o_cc o_cc1 text-decoration-none" tabindex="0"> |
There was a problem hiding this comment.
Why do we need this change? Btw it's dead template, will be remove in following merge. 😄
There was a problem hiding this comment.
hmmmm it should have been deleted upon rebase. 🤔
| case "ArrowUp": | ||
| ev.preventDefault(); | ||
| if (this.menuEl) { | ||
| const allResults = [...this.menuEl.querySelectorAll(".o_search_result_item a")]; |
There was a problem hiding this comment.
| const allResults = [...this.menuEl.querySelectorAll(".o_search_result_item a")]; | |
| const resultEls = this.menuEl.querySelectorAll(".o_search_result_item > a"); |
🙁 Els suffix for var holding element value + querySelectorAll itself should return list + we should target direct anchor child only.
| "t-on-keydown": this.onKeydown, | ||
| "t-on-search": this.onSearch, | ||
| }, | ||
| ".o_search_result_item a": { |
There was a problem hiding this comment.
| ".o_search_result_item a": { | |
| ".o_search_result_item > a": { |
| * @param {KeyboardEvent} ev | ||
| */ | ||
| onSearchResultKeydown(ev) { | ||
| const allResults = [...this.menuEl.querySelectorAll(".o_search_result_item a")]; |
There was a problem hiding this comment.
| const allResults = [...this.menuEl.querySelectorAll(".o_search_result_item a")]; | |
| const allResults = this.menuEl.querySelectorAll(".o_search_result_item a"); |
Also, this will be called everytime you press the click, So numberOf(DOM Query) === NumberOf(key press).
To reduce this repetitive click, just cache this result in this.allresults or somewhere else.
| distance = Math.sqrt( | ||
| Math.pow(centerY - currentCenterY, 2) + | ||
| Math.pow(Math.abs(centerX - currentCenterX) / 2, 2) | ||
| ); |
There was a problem hiding this comment.
🙂 Aren't these 4 lines repeated in all cases? It's just a jumble of the positions of variables (which doesn't matter when you're using pow)."
Math.pow(-4, 2) === Math.pow(4, 2) & calculation of the absolute distance will always be same for each time AFAIK.
This should be enough for all the cases. ( if divide by 2 is not that much important, I didn't get the logic for that)
const distance = Math.sqrt(
Math.pow(centerY - currentCenterY, 2) + Math.pow(centerX - currentCenterX, 2)
);
| if (isFirstRow) { | ||
| // Focus back to input when in first row | ||
| this.inputEl.focus(); |
There was a problem hiding this comment.
Can't we just prepare a global list of allresult = [this.inputEl, ..searchResults] & follow the same process instead manual redirection? (Just like it was done previously - I'm not sure!!)
Issue:
Arrow key navigation for search result items regressed with the new search bar layout. The previous logic in onKeydown was no longer triggered due to the updated DOM structure.
Fix:
Introduce a dedicated onSearchResultKeydown handler and wire it to search result items. The new method restores ArrowUp and ArrowDown focus management while keeping the existing input keydown behavior unchanged. Added a complete test case to cover arrow key navigation in
search results.
task-5424392