-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Fix for #4153 #4253
base: main
Are you sure you want to change the base?
Fix for #4153 #4253
Conversation
Thanks for your contribution 👍 To make it easier for us poor reviewers, could you undo all formatting corrections (i.e. indentation changes)? |
wled00/data/common.js
Outdated
} | ||
|
||
function tooltip(cont = null) { | ||
const isTouchDevice = 'ontouchstart' in window || navigator.maxTouchPoints > 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will cause problems on laptops with a touch screen. You won't be able to see the tooltip when you move the mouse over the button.
Maybe do something like this:
var isTouchDevice = false;
window.addEventListener('touchstart', () => {
isTouchDevice = true;
}, { once: true });
window.addEventListener('mousemove', () => {
isTouchDevice = false;
}, { once: true });
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, but your solution doesn't work since the event listeners for touchstart
, touchend
, mouseover
, and mouseout
need to be removed/added.
I will get back to it when I have a little more time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And you have to consider that on laptops with touchscreens, you can switch between mouse and touch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My code snippet was only intended to replace your isTouchDevice
calculation.
Your add/remove event listener should still be where you put it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And you have to consider that on laptops with touchscreens, you can switch between mouse and touch.
I think this would not be a problem since touch input on a laptop with a touchscreen is interpreted as mouse input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My code snippet was only intended to replace your
isTouchDevice
calculation. Your add/remove event listener should still be where you put it.
Let me clarify what I mean:
by the time if (isTouchDevice)
... is executed, it is very unlikely that the touchstart
event has already been triggered.
As a result, only the mouse event listeners will be added, which in practice will lead to the same issue as before.
Here is how I implemented your code snippet:
function tooltip(cont = null) {
var isTouchDevice = false;
window.addEventListener('touchstart', () => {
isTouchDevice = true;
}, { once: true });
window.addEventListener('mousemove', () => {
isTouchDevice = false;
}, { once: true });
d.querySelectorAll((cont ? cont + " " : "") + "[title]").forEach((element) => {
// isTouchDevice is pretty much always false here (except the user somehow manages to trigger the tochstart event in that short time period)
if (isTouchDevice) {
element.addEventListener("touchstart", () => { showTooltip(element); });
element.addEventListener("touchend", () => { hideTooltip(element); });
} else {
element.addEventListener("mouseover", () => { showTooltip(element); });
element.addEventListener("mouseout", () => { hideTooltip(element); });
}
});
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of something like this:
var isTouchDevice = false;
window.addEventListener('touchstart', () => {
isTouchDevice = true;
}, { once: true });
window.addEventListener('mousemove', () => {
isTouchDevice = false;
}, { once: true });
...
function tooltip(cont = null) {
d.querySelectorAll((cont ? cont + " " : "") + "[title]").forEach((element) => {
if (isTouchDevice) {
element.addEventListener("touchstart", () => { showTooltip(element); });
element.addEventListener("touchend", () => { hideTooltip(element); });
} else {
element.addEventListener("mouseover", () => { showTooltip(element); });
element.addEventListener("mouseout", () => { hideTooltip(element); });
}
});
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tooltip
function is called on page load, therefore this doesn't make a huge difference
After a bit of testing I found out that the mousemove
event has the problem as mouseover
and gets triggert an touch devices
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean with touch devices? Devices with only a touchscreen or Laptops with touchscreen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
devices with only a touch screen
wled00/data/index.js
Outdated
} | ||
|
||
function tooltip(cont = null) { | ||
const isTouchDevice = 'ontouchstart' in window || navigator.maxTouchPoints > 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
I took another lock at it i think this is a good solution |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good idea to use the pointer
(over
/out
) event! 👏🏼 I wonder why I did not come up with this idea. Seems like a good and simple solution.
However, I did find a way to reproduce the behavior of #4153 on a laptop with a touchscreen 😆:
- Click and hold the update segment button with your mouse
- While holding down the left mouse button, click with your finger anywhere else on the touchscreen
- Release the mouse button
To be honest, I have no idea why the pointerout event is not triggered in this case. 🤔
But since this is such a rare edge case, I would still approve this PR.
Fixes #4153
The issue was caused by the
mouseover
event sometimes being triggered on button release for touch inputs. This resulted in the tooltip appearing but never closing.I resolved the issue by only adding the
mouseover
andmouseout
events on devices without touchscreens,while the
touchstart
andtouchend
events are loaded for devices with touchscreens.