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

Improved mobile support #2616

Merged

Conversation

pythongosssss
Copy link
Collaborator

By no means perfect, but better than it currently is.

  • Ability to hide menu
  • Responsive setting screen
  • Touch events for zooming (pinch), opening rightclick/context menu (touch hold, release)

I've tested on Firefox + Chrome on Android, and Safari on an old iPad. If someone could please test on a newer iOS device that would be great.

Close button menu, hides the menu and shows a hamburger menu icon:
Screen Shot 2024-01-22 at 19 00 50
image

Settings collapses into a single column with a close button at the top
image

Responsive setting screen
Touch events for zooming/context menu
@nntb
Copy link

nntb commented Jan 29, 2024

i am testing on a galaxy fold 4 in chrome brave and firefox.

i would like to add the mac 2 finger addon dosnt aid in interactivity with touch on android phones. and in dex mode the ctrl scroll wheel combo dosnt zoom.
https://github.com/subtleGradient/TinkerBot-tech-for-ComfyUI-Touchpad

@pythongosssss pythongosssss marked this pull request as ready for review January 31, 2024 19:28
@comfyanonymous comfyanonymous merged commit fd73b5e into comfyanonymous:master Feb 8, 2024
1 check passed
@unphased unphased mentioned this pull request Feb 10, 2024
@unphased
Copy link
Contributor

unphased commented Feb 10, 2024

Tested on M1 ipad pro

  • now two finger pinch can zoom, which is a huge improvement from before, this should be usable from a phone now, not something that could be said earlier (basically on a phone youre using remote desktop haha). good stuff. I will build a PR on top of this to flesh out the two finger gesture for full pan and zoom. Hope you don't mind, and it's a great starting point.
  • hard to spot what was different from before in the settings page. but it does have nice responsive scroll within a modal so something's being done right there.
  • menu hiding works great
  • context menu open via tap hold works. easy to mess up and do a small drag with it but nothing a few tweaks can't address. curious if we would like to also have a two finger tap gesture alternative to do the same. i think if we make zooming much easier then it becomes practical because even if you have a tiny node you can zoom to make it bigger so you can tap with two fingers on it.

@unphased
Copy link
Contributor

unphased commented Feb 10, 2024

@pythongosssss I noticed poking in the network tab, you have another plugin called touchEvents.js:

import { app } from "../../../scripts/app.js";

// Adds mapping of touch events to mouse events for mobile. This isnt great but it is somewhat usable

app.registerExtension({
	name: "pysssss.TouchEvents",
	setup() {

seems like something provided by your node suite? Has this been superceded?

@pythongosssss pythongosssss deleted the improved-mobile-support branch February 10, 2024 15:14
@unphased
Copy link
Contributor

unphased commented Feb 10, 2024

I would like to note two things at this time

  • I found Touchpad two-finger gesture support for macOS #2059 and the corresponding solution posted here https://github.com/subtleGradient/TinkerBot-tech-for-ComfyUI-Touchpad/. It TOTALLY WORKS. so this addresses one of the huge UX issues on macOS with trackpads. I am strongly encouraging more testing of this code under windows and PRing it into Comfy proper because it hugely improves macOS trackpad user experience.
  • There is some sort of weird freezing i'm experiencing ever since i built comfyui HEAD today to test this here PR. I have seen it happen in safari on macos and ipadOS. I need to spend a bit of time to see if I can find out what's causing it. feels almost like some interaction is causing an infinite loop of some sort. Sorry for lack of details. I don't have any right now.

@unphased
Copy link
Contributor

unphased commented Feb 10, 2024

a little more information on the problem i found:

some sort of weird freezing i'm experiencing ever since i built comfyui HEAD today to test this here PR. I have seen it happen in safari on macos and ipadOS.

  • Specific to Safari (not reproducing on Chrome on macOS, but can reproduce in safari on macOS)
  • Triggered by doubleclicking to open the menu. on macOS this drops framerate to 1/5fps or so and the tab is for all practical purposes nonresponsive. on iOS something similar happens but usually no repaint can ever be observed. i'll investigate this. it's worth noting Chrome is unaffected by whatever the hell this is

Separately I'd like to mention that on macOS somehow ComfyUI seems completely broken in Firefox as you cannot left click or drag. I use it all the time from Firefox on linux though.

@unphased
Copy link
Contributor

Confirmed this PR did not introduce the rendering issue for the double click menu! will post further findings in the new issue I made #2760.

@Gerschel
Copy link

If you are trying to get true mobile support, you might have to drill in the Litegraph.core.js and change some event handlers. Like first detect if its a mobile user agent and/or if navigator.maxTouchPoints > 0. Then change some event handlers around, example,

LiteGraph.pointerListenerAdd(dialog,"leave", function(e) {
                if (prevent_timeout){
                    return;
                }
                timeout_close = setTimeout(function() {
                    dialog.close();
                }, typeof options.hide_on_mouse_leave === "number" ? options.hide_on_mouse_leave : 500);
            });

This causes the search box to close if the mouse leaves, so in mobile, if you tried scrolling through the entries, as soon as your touch ends, it closes the box. Instead, maybe there should be a click event for when it occured outside of the box, if(!dialog.contains(e.target), it closes the box.

@unphased
Copy link
Contributor

You're probably right! For now the search menu still successfully works for picking an item, I have to do more testing to confirm how broken swiping to scroll and choosing from farther down is.

@Gerschel
Copy link

I noticed this boolean at the top of litegraph.core.js, it completely disables the mouse leave auto hide, but still the scrolling and finger leaving causes it to vanish.

        search_hide_on_mouse_leave: true, // [false on mobile] better true if not touch device, TODO add an helper/listener to close if false

When looking at litegraph.js repo, it appears these are meant to be settings. They are also modifiable directly in the conosole LiteGraph.search_hide_on_mouse_leave = false.

Which means litegraph.core.js might not need to be modified directly, app.js might be able to make the modifications.

I tested trying to use a touchstart and touchend using the method LiteGraph.pointerListenerAdd("touchstart"), the results were not as expected.

@Happ1ness-dev
Copy link

but still the scrolling and finger leaving causes it to vanish.

Can confirm, search menu is a bit clunky to use because of this.

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

Successfully merging this pull request may close these issues.

6 participants