odoo icon indicating copy to clipboard operation
odoo copied to clipboard

[FIX] website: restore focused element highlight in search suggestions

Open bso-odoo opened this issue 2 years ago • 6 comments

Since 1 the highlight of the focused element in the search auto-complete has disappeared. This makes the keyboard navigation barely usable.

This commit restores the highlight of the focused element.

Steps to reproduce:

  • Drop a "Search" snippet.
  • Save.
  • Type "a" in the search box.
  • Navigate the suggestions with the keyboard up and down arrows. => Currently focused element looked the same as the other ones.

task-3148871

bso-odoo avatar Feb 13 '23 16:02 bso-odoo

Pull request status dashboard

robodoo avatar Feb 13 '23 16:02 robodoo

@fw-bot up to 15.0

bso-odoo avatar Feb 14 '23 07:02 bso-odoo

Forward-port disabled.

fw-bot avatar Feb 14 '23 07:02 fw-bot

I am not sure to understand the fix that was merged 5ef7e8b for 16.0+:

  • Why can't we use standard :focus pseudo class?
  • But mainly, why do we need to restore specific CSS rules for that searchbar_form dropdown? Is not the bug on all website dropdowns? From what I see because OWL dropdown rules apply on non-owl dropdowns?

@qsm-odoo on the first PR #98688 for 16.0+, even with :focus style restored, the issue won't be solved, because (@bso-odoo you can correct me if I'm wrong...) after the migration to BS5 and with the replacement of .dropdown-menu by .o_dropdown_menu here, default bootstrap keyboard navigation was completely lost on the searchbar dropdown. so the possible fix for 16.0+ was to implement it manually...

The lost :focus style on 15.0 is a separate issue and Yes it was lost on all non-owl dropdowns! (I see that BSO updated the code on this PR with a general CSS rule...)

A SUGGESTION: maybe we can get rid of the @extend of .o_focus from https://github.com/odoo/odoo/commit/5ef7e8b7dcfff8a13fd014dca7aa65c7cc715af9 by using the .focus class added on the OWL refactoring. I think it has the same effect :thinking:?

xO-Tx avatar Feb 15 '23 13:02 xO-Tx

I am not sure to understand the fix that was merged 5ef7e8b for 16.0+:

  • Why can't we use standard :focus pseudo class?
  • But mainly, why do we need to restore specific CSS rules for that searchbar_form dropdown? Is not the bug on all website dropdowns? From what I see because OWL dropdown rules apply on non-owl dropdowns?

(Oops, I see there has just been another response by @xO-Tx)

@qsm-odoo Is this what you had in mind ? (I patched the forward-port) https://github.com/odoo/odoo/pull/112722/files

bso-odoo avatar Feb 15 '23 13:02 bso-odoo

@xO-Tx @bso-odoo

default bootstrap keyboard navigation was completely lost on the searchbar dropdown. so the possible fix for 16.0+ was to implement it manually...

I understand we removed the JS behavior for that component but we still used the full CSS of dropdown menus (thanks to the @extend command. So I don't see why :focus would not work.

A SUGGESTION: maybe we can get rid of the @extend of .o_focus from https://github.com/odoo/odoo/commit/5ef7e8b7dcfff8a13fd014dca7aa65c7cc715af9 by using the .focus class added on the OWL refactoring. I think it has the same effect thinking?

And at worst, yes you could use the focus class (which is a bootstrap class, not an owl class)... but better to use :focus if possible (just have to make the element focusable.

@qsm-odoo Is this what you had in mind ? (I patched the forward-port) https://github.com/odoo/odoo/pull/112722/files

I don't think so: what I have in mind for both cases (15.0 standard dropdown or 16.0 custom dropdown) is to not need extra CSS rules: bootstrap have rules for dropdowns, they should work without the need of re-forcing them. For me the dropdown.scss file is applied to the frontend while it should not (or at least, in stable, it should only impact the dropdowns he meant to impact: those whose are handle by OWL on the JS -> maybe they should be targeted via a more precise rule or something like that)?

qsm-odoo avatar Feb 16 '23 16:02 qsm-odoo

@qsm-odoo Ok :bulb: :facepalm: , you meant like this: https://github.com/odoo/odoo/pull/112569/commits/fdfffc3b7c421103df11b31626489c7074358342 Which gives this in the forward-port: https://github.com/odoo/odoo/pull/112722/commits/d3e80276835abffa62faee0e24a6b8d14c2f4551. If this is OK, I would then let this PR's change get forwarded up to master and remove it from #112722 - and backport the JS part of #112722 onto 16.0.

bso-odoo avatar Feb 17 '23 12:02 bso-odoo

Ok :bulb: :facepalm: , you meant like this: https://github.com/odoo/odoo/commit/fdfffc3b7c421103df11b31626489c7074358342

No sorry ^^ Doing that:

  • You risk breaing the backend by making all rules with a higher priority
  • You break any use of an OWL dropdown on the frontend side (OWL is available in the frontend, I am guessing the dropdown component too)
  • If this was the right solution... then the fix would rather be about moving the SCSS file in the backend-only assets instead of keeping it virtually disabled (genereting rules that cannot be matched) in the frontend

There are two parts in that problematic file:

  • the .o-dropdown rule at the end of the file: this targets dropdown which are OWL components (see <t t-name="web.Dropdown" owl="1">) -> those can stay untouched: they apply both in frontend/backend, on OWL components. Now I am surprised that there are CSS rules which should apply on OWL dropdowns only... but... no need to investigate that in the website team, especially in stable
  • the rules that target all dropdown menus (standard bootstrap or OWL+bootstrap) at the top of the file: I don't think they make any sense... especially since they lack comments. But they clearly seem to target a very specific OWL behavior: "I don't want the focus style to be controlled by the browser but only when I add the class myself", not only I don't see when that behavior can make sense (when would I not want a focused element have its focus style?) but mainly... this seems extremely linked to the OWL component behavior -> they should not target .dropdown-menu but .o-dropdown--menu.

Now there also are rule on item and toggle, I did not check what they are (but same logic to use). If they are rules that are fixing bootstrap behavior, they should not be there but in bootstrap_review.scss instead. But it does not seem the case:

.dropdown-item:not(.disabled):not(:disabled) {
  // Needed 'cause <DropdownItem> generate <span> html tags that for browsers are
  // normally not clickable.

=> The comment says "this fixes a problem with the OWL component"... but it targets all dropdown items whether or not they are OWL components :shrug:

qsm-odoo avatar Feb 17 '23 17:02 qsm-odoo

@qsm-odoo Actually all the styles in the first part were related to the owl dropdown component (as far as I can tell). I merged them into the second part. I could limit the change to moving only the .o-dropdown--menu part if we fear that some customization might rely on the other styles outside of owl dropdowns.

bso-odoo avatar Feb 20 '23 13:02 bso-odoo

@fw-bot up to master

bso-odoo avatar Feb 21 '23 07:02 bso-odoo

@bso-odoo forward-port limit can only be set before the PR is merged.

fw-bot avatar Feb 21 '23 07:02 fw-bot

Oops, did not see you blocked the forward-port originally :grimacing: Continuing on https://github.com/odoo/odoo/pull/113201

qsm-odoo avatar Feb 21 '23 13:02 qsm-odoo