Skip to content

Focus, shadow DOM, submenus, buttons and input elements #55

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

Closed
tim-janik opened this issue Feb 29, 2024 · 2 comments
Closed

Focus, shadow DOM, submenus, buttons and input elements #55

tim-janik opened this issue Feb 29, 2024 · 2 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@tim-janik
Copy link
Owner

Currently we have a number of focus related problems:

  • BUG: Clicking on menu button and then outside a popup menu causes the menu button to get stuck in pressed state. Also a pointer grab stays active so the UI becomes unusable. Pressing ESCAPE resolves the state. But from then on, the popup menu stays hidden and cannot never be shown again.
  • BLOCKER: The focus handling complexity is one major reason we do not support submenus yet, this important for plugin selection and some other cases.
  • MINOR: Arrow keys currently do not cycle through menus across start/end.
  • MINOR: Using tab inside a focus trap like a modal dialog escapes into the browser chrome.

Fixing focus/keyboard handling in the web platform is notoriously hard:
Managing focus in the shadow DOM
Dialogs and shadow DOM
We have a lot of complex code in place for focus handling, but don't always catch all cases. The problems with a stuck grab may be related to the move to <dialog/> instead of our own focus/modal shield element, but this can also be caused by the menu items containing a focusable <button/> element.

Issus related to shadow DOM are manyfold.

  • Elements inside shaodow DOM cause issues in case they are focusable. Normally, shadow DOM is not an issue when <slot/> re-exposes child elements of a custom component to the light DOM tree. Instead of stashing focusable child elements into the shadowDOM, the component should simply derive from the focusable HTMLElement subclass to be focusable/editable (it just cannot be LitElement that way).
  • Handling element traversal (querySelector*) does not go into shadow DOMs, making any logic (like controlling the focus chain) that needs deep DOM inspection much more complicated.
  • CSS styling and visibility logic based on :focus, :focus-visible, :focus-within will hardly work as intended with focusable elements stashed away in shadow DOMs.
  • Controlling the focus order (like menu & submenu navigation via arrow keys need it) is hard to get right when considering (open) shadow DOM, inert, hidden elements, tabindex, custom components where focusable may depend on derivation from a specific HTMLElement subclass and potential future elements for which we don't know if they are focusable.
  • For closed shadow DOMs, it is impossible to control the focus chain.

Here is what we can do:

  • Keep all focusable elements out of shadow DOMs. Our LitComponent class could enforce this in __DEV__ mode by peeking inside the shadow DOMs created and ignoring external children that are assigned via <slot> elements. That means keeping all focusable elements in the light DOM, and in some cases where we have custom focusable components, these need to be implemented without Lit (since LitElement always derives from HTMLElement instead of any of its focusable subclasses).
  • Analysis; currently we have shadow DOMs for:
    • contextmenu.js - OK, has dialog in shadow DOM
    • menutitle.js - OK, but could be simplified to not use shadow DOM
    • editable.js - BAD, uses focussable input
    • menuitem.js - BAD, the major offender with <button/> inside the shadow DOM.
  • Port editable to derive from HTMLInputElement directly, getting rid of its LitElement heritage. (Good start, should be easier than menuitem)
  • Port menuitem to derive from HTMLButtonElement directly, getting rid of its LitElement heritage.
  • Leave visibility handling to CSS, i.e. we could display (via CSS) menus and submenus only if they have a focus child somewhere in the ancestry.
  • Implementing context menus can be a bit tricky via CSS visibility handling, as the context menu may need to be moved between multiple elements that can show the same context menu.
  • Just implement arrow key focussing, based on reducing our current focus logic to just the two functions suggested in the above article series: element.getNextTabbableElement() element.getPreviousTabbableElement()
@tim-janik tim-janik self-assigned this Feb 29, 2024
@tim-janik tim-janik added the bug Something isn't working label Feb 29, 2024
@tim-janik tim-janik added this to the v0.3.0 milestone Feb 29, 2024
tim-janik added a commit that referenced this issue Mar 4, 2024
This solves the menu-button-stuck-on-outside-click issue
described in #55.

Signed-off-by: Tim Janik <[email protected]>
tim-janik added a commit that referenced this issue Mar 4, 2024
* Branch commit log:
  x11test/play-notes.json: fixup icon reference
  ui/theme.scss: fix duplicate CSS
  ui/postcss.js: minor fix
  ui/dom.js: add text_content()
  ui/b/treeselector.vue: allow get_uri() in <template/>
  ui/b/treeselector-item.vue: allow get_uri() in <template/>
  ui/b/textinput.js: fix missing prop init
  ui/b/contextmenu.js, menurow.js: fix padding
  ui/types.d.ts: add document.startViewTransition
  package.json: update all packages (Electron-29)
	This updates to Vue-3.4, Electron-29 with Chrome-122,
	Node-20 and V8-12.
  ui/Makefile.mk: run stylelint when processing CSS files
  ui/stylelintrc.cjs: add postcss-scss to stylelint, ignore whitespace diags
  package.json: add stylelint-config-standard-scss
  package.json: fix dependency order
  ui/b/contextmenu.js: treat normal <button/> elements like menu items
  ui/b/trackview.js: use button as menu item
  ui/b/menurow.js: add some horizontal spacing around rows
  ui/b/pianoroll.js: use button as menu item
  ui/b/menubar.js: use button as menu item
  ui/b/deviceeditor.js: use button as menu item
  ui/b/choiceinput.js: use button as menu item
  ui/util.js: remove uinused handler
  ui/kbd.js: remvoe useless prefix
  ui/kbd.js: leave focus activation via Enter to the browser
  ui/kbd.js: allow cycling when arrow keys change focus
  ui/dom.js: add get_uri, valid_uri, has_uri
  ui/b/treeselector.vue: use get_uri()
  ui/b/treeselector-item.vue: use get_uri()
  ui/b/trackview.js: use get_uri()
  ui/b/pianoroll.js: use get_uri()
  ui/b/filedialog.vue: use get_uri()
  ui/b/choiceinput.js: use get_uri()
  ui/eslintrc.cjs: permit custom class names next to tailwind
  ui/b/aboutdialog.js: fix class comment to apply to class
  ui/b/folderview.vue: remove useless flexbox packing around each file entry
	Significant CR speed up, flexbox seems slow in Chrome.
  ui/b/editable.js: fix background color
  ui/Makefile.mk: remove stale build file $>/ui/global.scss
  ui/tailwind.scss: split off button-down-within and button-active
  ui/b/menubar.js: adjust icon layout
  ui/b/folderview.vue: adjust icon layout
  ui/b/icon.js: simplify and remove intermediate HTMLElement
  ui/b/menubar.js: apply button-dim
  ui/b/playcontrols.js: apply button-down
  ui/tailwind.scss: split .button-dim .button-down
  ui/b/playcontrols.js: add .button-dim to push-buttons
  ui/b/filedialog.vue, ui/b/preferencesdialog.vue: use .button-xl
  ui/b/editable.js: only cancel valid edits on Escape
  ui/Makefile.mk: invoke ui/postcss.js directly
  ui/colors.js: import zcam-js as module
  ui/Makefile.mk: allow recovery from build errors during live updates
  ui/Makefile.mk: reassign ui-build and ui-reload rules
  ui/b/trackview.js: use tailwind for b-editable
  ui/b/positionview.js: use tailwind for b-editable
  ui/b/editable.js: simplify, remove shadowRoot, use tailwind
  ui/tailwind.scss: add text-inherit and :focus-visible styling
  ui/eslintrc.cjs: remove unsupported start_view_transition global
  ui/b/aboutdialog.js: use startViewTransition() around closing
  ui/global.scss: get rid of (now) unused grid helpers
  ui/b/filedialog.vue: use col-start-* row-start-*
  ui/b/pianoroll.js: use col-start-* row-start-*
  ui/b/shell.vue: properly use document.startViewTransition()
  ui/index.html: sync document.startViewTransition() with Vue for CR and FF
	Always sync document.startViewTransition() with pending Vue updates, and install
	a simple handler caller if the browser does not support document.startViewTransition()
	(like FireFox-123).
  ui/b/aboutdialog.js: use tailwind styling
  ui/tailwind.scss: add dialog-header, dialog-footer
  ui/tailwind.scss: add button-dim, button-xl
  ui/global.scss: leave buttons to tailwind
  ui/tailwind.scss: add border-1
  ui/tailwind.scss: add hflex, vflex and dialog helpers
  ui/tailwind.scss: add dflt border color, disable text selection in base layer
  ui/postcss.js: add "dim" color scheme, a smoothed zinc like color
  ui/postcss.js: add --tw-border-default-color
  ui/ch-component.md: add section on "Guidelines for Web Components"
  ui/util.js: remove unneeded visibility:hidden toggeling
	This solves the menu-button-stuck-on-outside-click issue
	described in #55.

Signed-off-by: Tim Janik <[email protected]>
tim-janik added a commit that referenced this issue Mar 6, 2024
This addresses #55 by now keeping a button inside a shadowRoot.

Signed-off-by: Tim Janik <[email protected]>
tim-janik added a commit that referenced this issue Mar 6, 2024
* Branch commit log:
  ui/b/menurow.js: update menu item description
  ui/b/menuitem.js: remove, use <button/> instead now
	This addresses #55 by now keeping a button inside a shadowRoot.
  ui/b/contextmenu.js: handle .isactive, disabled state, focus assignment
  ui/b/aboutdialog.js: use startViewTransition() to show dialog with async values
  ui/eslintrc.cjs: allow & in class attributes of lit elements
  ui/b/dialog.vue: add floating-dialog styling
  ui/theme.scss: fix CSS
  ui/mixins.scss: fix CSS
  ui/global.scss: fix CSS
  ui/b/filedialog.vue: fix CSS
  ui/Makefile.mk: fix dir dependency
  ase/memory.cc: change TODO comment
  ase/clapplugin.cc: change TODO comment
  ui/b/trackview.js: use render_contextmenu() and fix menu flicker
  ui/b/treeselector-item.vue: fix CSS
  ui/b/statusbar.js: use tailwind, fix height
  ui/b/menurow.js: use tailwind
  ui/b/propgroup.js: fix CSS
  ui/b/menubar.js: fix CSS
  ui/b/knob.js: fix CSS
  ui/b/icon.js: fix CSS
  ui/b/dialog.vue: fix CSS
  ui/b/devicepanel.vue: fix CSS
  ui/b/contextmenu.js: add render_contextmenu()
  ui/b/clipview.js: fix CSS
  ui/b/choiceinput.js: fix CSS
  ui/b/contextmenu.js: properly update buttons on attribute changes
  ui/b/contextmenu.js: fix CSS
  ui/dom.js: fix comment
  misc/version.sh: truncate extra long commit hash characters

Signed-off-by: Tim Janik <[email protected]>
@tim-janik
Copy link
Owner Author

* **BUG:** Clicking on menu button and then _outside_ a popup menu causes the menu button to get stuck

FIXED.

* **BLOCKER:** The focus handling complexity is one major reason we do not support submenus yet, this important for plugin selection and some other cases.

WONTFIX, at least for now. We have proper support for trees with collapsible branches in menus now.

* **MINOR:** Arrow keys currently do not cycle through menus across start/end.

FIXED.

* **MINOR:** Using tab inside a focus trap like a modal dialog escapes into the browser chrome.

WONTFIX. We can reopen this in a separate issue if it turns out to be a real accessibility issue.

* Just implement arrow key focussing, based on reducing our current focus logic to just the two functions suggested in the above article series: `element.getNextTabbableElement()` `element.getPreviousTabbableElement()`

The code to figure the current (captured) focus chain (and thus the START/PREV/NEXT/END/LEFT/RIGHT/TOP/BOTTOM focus sibling) is still tedious and brittle, but covers our uses for now.
It be best to move this into a community maintained npm module and ideally browser would provide support for this one day...

@tim-janik
Copy link
Owner Author

Issus related to shadow DOM are manyfold.

* Elements inside shaodow DOM cause issues in case they are focusable. Normally, shadow DOM is **not** an issue when <slot/> re-exposes child elements of a custom component to the light DOM tree. Instead of stashing focusable child elements into the shadowDOM, the component should simply derive from the focusable HTMLElement subclass to be focusable/editable (it just cannot be LitElement that way).

Also, we have moved all focus handling out of shadowDom elements now. And we will keep it that way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant