Dark Mode Toggle Shortcut on Safari

lorenzolewis commented 1 year ago

Describe the bug

The shortcut for dark mode toggle of meta + shift + d has a conflict in Safari on macOS (which maps to Bookmarks > Add to reading list). This makes the keybinding unusable since the browser doesn't pass it on.

This is using version 0.11.6 of histoire.

I'd recommend using a keybinding that isn't using any modifier keys (such as just d) since browsers all like to have their own different combinations of keyboard shortcuts.

Reproduction

N/A, just need to use any project on a Mac with Safari.

System Info

System:
    OS: macOS 13.0
    CPU: (8) arm64 Apple M1 Pro
    Memory: 64.13 MB / 16.00 GB
    Shell: 5.8.1 - /bin/zsh
  Binaries:
    Node: 18.11.0 - /opt/homebrew/bin/node
    Yarn: 1.22.19 - /opt/homebrew/bin/yarn
    npm: 8.19.2 - /opt/homebrew/bin/npm
  Browsers:
    Safari: 16.1

Used Package Manager

pnpm

Validations

[X] Follow our Code of Conduct
[X] Read the Contributing Guidelines.
[X] Read the docs.
[X] Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
[X] Check that this is a concrete bug. For Q&A open a GitHub Discussion.
[X] The provided reproduction is a minimal reproducible example of the bug.

lorenzolewis commented 1 year ago

I can't reproduce this 100% of the time, sometimes the shortcut works, sometimes it doesn't and Safari steals the show.

Akryum commented 1 year ago

I'm using event.preventDefault() though :( https://github.com/histoire-dev/histoire/blob/a326a6a9522fd8437f0abfb6280590b0d404a05e/packages/histoire-app/src/app/components/app/AppHeader.vue#L19

Akryum commented 1 year ago

So I've just tried on my Macbook on Safari 16.0 and I can't reproduce. Did you make sure to press [Shift] too?

lorenzolewis commented 1 year ago

I can't get it to reproduce 100% of the time, but it seems like it happens when the page loads and the browser is out of focus, then when I focus again it's like the preventDefault didn't take affect. I think in general sites try to avoid any shortcuts using meta keys and treat them as reserved by the browser. In the code there's a shortcut also defined that doesn't use the meta key https://github.com/histoire-dev/histoire/blob/c85126efea5744cc9bb830b5059e2d436cc991b1/packages/histoire-app/src/app/components/app/AppHeader.vue#L18

So maybe just adapting the tooltip on the button to show the ctrl+shift+d shortcut instead of meta+shift+d (for webkit browsers at least) could avoid hitting up against the edge case?

Akryum commented 1 year ago

I'm not sure ^+Shift works on Mac

50rayn commented 1 year ago

I played around a little with this scenario is found:

meta + shift + d this is the shortcut for Adding the current page to the Reading List, or in Firefox - Add to Bookmark
It is easily reproducible. Everything works fine till you focus on iframe. The onKeyboardShortcut doesn't work in iframe.
in function onIframeLoad i think it should be added useEventListener

histoire-dev / histoire