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

Fix for #4153 #4253

Merged
merged 6 commits into from
Feb 15, 2025
Merged

Fix for #4153 #4253

merged 6 commits into from
Feb 15, 2025

Conversation

maxi4329
Copy link
Contributor

@maxi4329 maxi4329 commented Nov 6, 2024

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 and mouseout events on devices without touchscreens,
while the touchstart and touchend events are loaded for devices with touchscreens.

@softhack007
Copy link
Collaborator

Thanks for your contribution 👍

To make it easier for us poor reviewers, could you undo all formatting corrections (i.e. indentation changes)?
It will help us to see the real differences more clearly.

}

function tooltip(cont = null) {
const isTouchDevice = 'ontouchstart' in window || navigator.maxTouchPoints > 0;
Copy link
Contributor

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 });

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@w00000dy w00000dy Nov 11, 2024

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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); });
		}
	});
};

Copy link
Contributor

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); });
		}
	});
};

Copy link
Contributor Author

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

Copy link
Contributor

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?

Copy link
Contributor Author

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

}

function tooltip(cont = null) {
const isTouchDevice = 'ontouchstart' in window || navigator.maxTouchPoints > 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

@netmindz netmindz changed the base branch from 0_15 to main December 16, 2024 13:28
@softhack007 softhack007 added the bug label Jan 6, 2025
@DedeHai
Copy link
Collaborator

DedeHai commented Jan 20, 2025

@w00000dy @maxi4329 what is the status on this PR?

@maxi4329
Copy link
Contributor Author

I took another lock at it i think this is a good solution

@netmindz netmindz added this to the 0.15.1 candidate milestone Jan 22, 2025
Copy link
Contributor

@w00000dy w00000dy left a 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 😆:

  1. Click and hold the update segment button with your mouse
  2. While holding down the left mouse button, click with your finger anywhere else on the touchscreen
  3. 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.

@Andro-Marian
Copy link

This still occurs for me on 0.16 alpha for some reason.

@w00000dy
Copy link
Contributor

@Andro-Marian Which exact version have you tried? Since this PR hasn't been merged yet, you can test it here: https://github.com/maxi4329/WLED/tree/bugfix

@Andro-Marian
Copy link

@Andro-Marian Which exact version have you tried? Since this PR hasn't been merged yet, you can test it here: https://github.com/maxi4329/WLED/tree/bugfix

Nightly Release 20250209 - It says it has the fix. Or is not the good one?

@w00000dy
Copy link
Contributor

@Andro-Marian The fix is not in this release. The changelog says it because the issue has been closed, but this PR has not been merged yet. Binaries containing the fix can be found here:
https://github.com/maxi4329/WLED/actions/runs/12875375188

@DedeHai
Copy link
Collaborator

DedeHai commented Feb 15, 2025

@w00000dy is there anything that speaks agains merging this (apart from the edge case you found)?

@w00000dy
Copy link
Contributor

@DedeHai I don't see any reason not to merge this.

@DedeHai DedeHai merged commit 7f24269 into wled:main Feb 15, 2025
20 checks passed
DedeHai pushed a commit that referenced this pull request Feb 15, 2025
* fix for #4153

* only load touch/mouse events for touch/mouse devices

* undid formating changes

* undid more formating changes

* undid all formating changes

* use pointerover and pointerout eventlisteners
@maxi4329 maxi4329 deleted the bugfix branch February 15, 2025 17:29
@Andro-Marian
Copy link

Andro-Marian commented Feb 15, 2025

I try it now. Now on windows when you move the mouse over it, the label flickers.
firefox_0ixTFmlpEN

Why not removing it completly?

@w00000dy
Copy link
Contributor

The flickering is another bug. This bug was there before.

@Andro-Marian
Copy link

Andro-Marian commented Feb 15, 2025

Other thing I noticed is why the attribute Title is removed and setting another attribute data-title.
Sometimes show null as tooltip.

And it's still duplicate when clicking on it.

@maxi4329
Copy link
Contributor Author

maxi4329 commented Feb 15, 2025

Other thing I noticed is why the attribute Title is removed and setting another attribute data-title

this is to prevent the default tooltip from appearing

Sometimes show null as tooltip.

I cant reproduce that could you find a way to reproduce

And it's still duplicate when clicking on it.

do you updated to https://github.com/maxi4329/WLED/actions/runs/12875375188 (it should say v0.15.0-b7 instead of v0.16.0-alpha) and cleared the browsers cache?

@Andro-Marian
Copy link

Andro-Marian commented Feb 15, 2025

Yea. Fixed the flicker by this:

.btn-icon {
  pointer-events: none;
}

@maxi4329
Copy link
Contributor Author

Yea. Fixed the flicker by this:

.btn-icon {
  pointer-events: none;
}

@Andro-Marian could you open a new pr for this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Segment "update" hint not disappearing
6 participants