Skip to content
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

Firefox/Safari on Mac: can't change skin tone #14

Closed
danburzo opened this issue Jun 30, 2020 · 7 comments · Fixed by #15
Closed

Firefox/Safari on Mac: can't change skin tone #14

danburzo opened this issue Jun 30, 2020 · 7 comments · Fixed by #15
Labels
bug Something isn't working

Comments

@danburzo
Copy link

The skin tone selector does not work on my Firefox 77 / macOS. For some reason, there's no activeElement on the focusout event when I click on a skin tone button.

async function onSkinToneOptionsBlur () {
// On blur outside of the skintone options, collapse the skintone picker.
// Except if focus is just moving to another skintone option, e.g. pressing up/down to change focus
// I would use relatedTarget here, but iOS Safari seems to have a bug where it does not figure out
// the relatedTarget correctly, so I delay with rAF instead
await new Promise(resolve => requestAnimationFrame(resolve))
const { activeElement } = rootElement.getRootNode()
if (!activeElement || !activeElement.classList.contains('skintone-option')) {
skinTonePickerExpanded = false
}
}

@nolanlawson
Copy link
Owner

That blur event should only occur when you click _outside_of the skin tone picker. If you're trying to debug this, you may just comment out that entire thing, since it will probably be a bit annoying if, whenever you click inside of the DevTools, the skin tone picker disappears.

I can't repro in Firefox 77 on Ubuntu, so I wonder if this is an OS-specific thing? I'll try to find a Mac to check.

@nolanlawson nolanlawson added the bug Something isn't working label Jun 30, 2020
@danburzo
Copy link
Author

Huh, now that you mentioned... I checked and it only reproduces with an external Magic Trackpad. The Macbook's internal trackpad works fine. 🤦‍♂️

@nolanlawson
Copy link
Owner

Huh, well that's still a bug I'd like to fix... I wonder how that is possible?

@danburzo
Copy link
Author

danburzo commented Jun 30, 2020

Okay, it seems it's purely a timing issue and that I've Skinner-boxed myself into a pattern that is not there. I haven't exactly figured out everything yet, but: the problem reproduces in Firefox and Safari (but not Chrome) when you take longer between mousedown and mouseup. It just so happens that using the external trackpad I click it down (slower) while the internal trackpad I tap (faster). If you're quick enough to click the button until the next frame kicks it (as per the await rAF part) it works. Otherwise the list gets hidden from underneath your nose.

Here's me holding down the trackpad on Firefox (notice the list disappears):

ff

And here's Chrome, which on the mousedown already makes the button the activeElement, so I'm able to hold it down indefinitely:

chrome

Let me know if you're able to reproduce this on Linux!

@nolanlawson
Copy link
Owner

nolanlawson commented Jun 30, 2020

Thanks for the detailed explanation. On Linux, I cannot reproduce either with a separate mouse or with my built-in Dell XPS trackpad, in either Chrome, Firefox, or GNOME Web (WebKit). I will try to grab a Mac so I can test there! (My wife has one; I'll just bug her to borrow it when she's done. 😂)

If it's just a timing issue, then probably we can replace the requestAnimationFrame with something else (e.g. a setTimeout). It doesn't really matter, as long as it disappears within ~100ms after a blur.

It's a shame that I can't use a more simple relatedTarget, but it's not supported in Safari. 😞

@nolanlawson nolanlawson changed the title Firefox: can't change skin tone Firefox/Safari on Mac: can't change skin tone Jun 30, 2020
@nolanlawson
Copy link
Owner

Confirmed reproduce in Safari on MacOS. I'll write a fix.

@danburzo
Copy link
Author

Ah cool, let me know if I can help test it!

nolanlawson added a commit that referenced this issue Jun 30, 2020
nolanlawson added a commit that referenced this issue Jun 30, 2020
* fix: remove blur/focusout event for now

fixes #14

* test: disable test
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

Successfully merging a pull request may close this issue.

2 participants