matrix-react-sdk icon indicating copy to clipboard operation
matrix-react-sdk copied to clipboard

Fix TAC opening with keyboard

Open florianduros opened this issue 1 year ago • 4 comments

Checklist

  • [ ] Tests written for new code (and old code if feasible)
  • [x] Linter and other CI checks pass
  • [ ] Sign-off given on the changes (see CONTRIBUTING.md)

Closes https://github.com/element-hq/element-web/issues/27016

The compound Menu component is putting the keyboard listeners on the trigger through the props but the AccessibilityButton is already handling some keyboard events.

  • Replace the current implementation with the tooltip and icon button of compound components.
  • The compound tooltip visual is different than the EW Web

https://github.com/matrix-org/matrix-react-sdk/assets/2621378/edad5d8a-a3e9-4b77-9b41-a8e039240981

florianduros avatar Feb 26 '24 14:02 florianduros

What do we get from using AccessibilityButton without its accessible keyboard handling? Why not just switch to Compound button for usecases which need disableKeyboardOverrides

Because, it means add all the glue to between the EW tooltip and the compound button. The compound tooltip doesn't have the same style than the other tooltip in the spaces, so I'm using the old one of EW

florianduros avatar Feb 26 '24 16:02 florianduros

We have a 50/50-ish mix of Compound/local tooltips already, so please do not consider that a blocker, preferring Compound tooltips going forward

t3chguy avatar Feb 26 '24 16:02 t3chguy

We have a 50/50-ish mix of Compound/local tooltips already, so please do not consider that a blocker, preferring Compound tooltips going forward

Okay, I'll discard the use of the AccessibilityButton here and use the Tooltip and IconButton of compound instead

florianduros avatar Feb 26 '24 16:02 florianduros

We have a 50/50-ish mix of Compound/local tooltips already, so please do not consider that a blocker, preferring Compound tooltips going forward

Okay, I'll discard the use of the AccessibilityButton here and use the Tooltip and IconButton of compound instead

Compund ftw

t3chguy avatar Feb 26 '24 16:02 t3chguy