vladmandic / automatic

SD.Next: Advanced Implementation of Stable Diffusion and other Diffusion-based generative image models
https://github.com/vladmandic/automatic
GNU Affero General Public License v3.0
5.56k stars 409 forks source link

[Issue]: Image viewer - show current image #1765

Closed fbellgr closed 1 year ago

fbellgr commented 1 year ago

Feature description

~I am assuming here that the new behaviour is intentional.~

After generating a batch of image in txt2img, clicking on the image viewer always displays the next image. ~Would it be possible to be able to restore the original behaviour, i.e. displaying the current image, perhaps via a config switch?~

~In case this was unintentional, I include the platform description.~

Works as expected in chromium-based browser. Seems to be a Firefox-specific issue.

Version Platform Description

Version: e002652a Wed Jul 19 15:09:01 2023 -0400 Linux Mint 21.2 Firefox gradio/default light

vladmandic commented 1 year ago

not sure i understand. clicking on image in gallery should show that image. if it doesn't, it's a bug. and then there are previous/next buttons inside image viewer.

fbellgr commented 1 year ago

Here is a short video of what I am experiencing with the revision mentioned in the original post as well as on a different machine with a revision from yesterday evening.

https://github.com/vladmandic/automatic/assets/27892962/d8929807-2c4a-4f2a-a04e-75c2fabbf5db

fbellgr commented 1 year ago

Works as expected in Chromium. Should have tried that before even posting.

So this is an issue with Firefox.

vladmandic commented 1 year ago

Even weirder...

Synitare commented 1 year ago

Having this same issue, also exclusively with Firefox. Tested with --safe and a firefox window with addons disabled. It's definitely an odd one. Something that may be related; in Chrome the extra buttons for zoom/save/etc. are extended across the display width, while in firefox they're contained to a corner of the screen, which can be seen in the video provided of the issue.

I'm not super familiar with css or javascript, but tinkering a bit I disabled the event listener for clicking on the image, and clicking just caused it to cycle through the images. For some reason firefox seems to be calling whatever cycles the image prior to bringing up the lightbox. Hopefully that helps a bit to narrow it down.

vladmandic commented 1 year ago

That does help, just not sure when I'll have the time to drill into it.

fbellgr commented 1 year ago

It is very weird, because by the time this click event listener is entered, the image has already switched.

image

There was a bug with this last October in automatic1111. https://github.com/AUTOMATIC1111/stable-diffusion-webui/issues/3951

vladmandic commented 1 year ago

that a1111 bug says "fixed", but no reference to how/where. and sdnext does include all of fixes from that timeframe, so i don't think this was ever properly fixed.

anyhow, i took a closer look and root cause is difference how chrome vs firefox handle order of added events. both app and gradio add handler for click. in chrome, that handler triggers first, shows viewer and removes further handling. in firefox, gradio registered even triggers first, so by the time app handler is triggered, image already changed.

fyi, this is even in gradio: in Gallery.svelte -> `<img on:click={() => (selected_image = next)} ...> so its clear that gradio changes image to next and then viewer gets triggered.

i have no idea how to change order of events since gradio's event is dynamically assigned for each image, not just once. if anyone has an idea, please chime in...

Synitare commented 1 year ago

The relevant function from a1111:

function setupImageForLightbox(e) {
    if (e.dataset.modded) {
        return;
    }

    e.dataset.modded = true;
    e.style.cursor = 'pointer';
    e.style.userSelect = 'none';

    var isFirefox = navigator.userAgent.toLowerCase().indexOf('firefox') > -1;

    // For Firefox, listening on click first switched to next image then shows the lightbox.
    // If you know how to fix this without switching to mousedown event, please.
    // For other browsers the event is click to make it possiblr to drag picture.
    var event = isFirefox ? 'mousedown' : 'click';

    e.addEventListener(event, function(evt) {
        if (!opts.js_modal_lightbox || evt.button != 0) return;

        modalZoomSet(gradioApp().getElementById('modalImage'), opts.js_modal_lightbox_initially_zoomed);
        evt.preventDefault();
        showModal(evt);
    }, true);

}

Seems like Firefox really is just specifically a pain in this case for some reason. I haven't used it in a while, but anapnoe might also have some insight?

Razunter commented 1 year ago

Gallery.svelte has <img on:click={() => (selected_image = next)} and this seems to trigger first

vladmandic commented 1 year ago

@Razunter didn't i just say that couple of hours ago? @Synitare i've added that workaround in dev, will publish in few hours.

fbellgr commented 1 year ago

i've added that workaround in dev, will publish in few hours.

That is great news, thanks.

I notice that the event was changed from mousedown to click at f2a39ffdcf8c7b3c91e1e41b003614755b20967f.

Could this be handled differently or was it the "large js refactor"? Because I seem to recall that after the initial problem with Firefox was solved, right after the large js refactor, this was still working properly.

vladmandic commented 1 year ago

refactor did not change it, it was changed later because of specific issue (although, a different one), again firefox related.

fbellgr commented 1 year ago

Just out of curiosity, I tried putting mousedown back and it stops switching before opening. But with mousedown the image would not stay open with firefox after the previous changes.

Anyway, not trying to drag this on. Just not used to have these issues in this browser.

Synitare commented 1 year ago

For lack of a more elegant solution, this seems to do the trick, though it still causes the gallery to scroll to the next image, at least it opens the intended one in the lightbox and keeps it open.

function setupImageForLightbox(e) {
  if (e.dataset.modded) return;
  e.dataset.modded = true;
  e.style.cursor = 'pointer';
  e.style.userSelect = 'none';
  var isFirefox = navigator.userAgent.toLowerCase().indexOf('firefox') > -1;

  // Function to handle the common actions
  function showLightbox(evt) {
    const initialZoom = (localStorage.getItem('modalZoom') || true) === 'yes';
    modalZoomSet(gradioApp().getElementById('modalImage'), initialZoom);
    evt.preventDefault();
    showModal(evt);
  }

  if (isFirefox) {
    // For Firefox, add both mousedown and mouseup event listeners
    e.addEventListener('mousedown', (evt) => {
      if (evt.button !== 0) return;
      e.addEventListener('mouseup', showLightbox, { once: true });
    });
  } else {
    // For other browsers, add the click event listener
    e.addEventListener('click', showLightbox, true);
  }
}

Still not sure why this behaves the way it does in Firefox alone, but this at least seems to be a working solution that doesn't break other browsers.

fbellgr commented 1 year ago

567faeb751e4657d7caa7643439247188566a432 partially fixed the issue.

Now the image will not stay open when you release the mouse button unless you drag slightly before releasing. This is not a big irritant.

And contrary to the last time this happened, holding and dragging does not move the image upon opening it, which is good. You have to click again and hold to move the image, and this is the way it should be.

I would say it's better.

vladmandic commented 1 year ago

i'm not sure if its better or worse. but i'm starting to hate firefox more and more, there are sooo many things that either don't work or require special handling in firefox.

fbellgr commented 1 year ago

Let's say that it is less annoying.

Strange, because this the only time I have seen so many issues specific to Firefox. And to be fair, this is all centred around one implementation of a specific feature.

vladmandic commented 1 year ago

oh, i've done a lot of dev work not related to webui and trust me, firefox is annoying as hell. firefox was amazing in the first few years (i used to love it), now its bloated, slow and very much behind. i'll do what i can to support it.

fbellgr commented 1 year ago

Interesting. I have not had this experience. Entirely different dev stack, though. When viewing media in a browser, I always prefer firefox. Its fullscreen mode is way better than the others.

EDIT: not to mention Firefox multi-account containers, which are very handy when working with multiple Azure or Salesforce accounts at the same time.

Razunter commented 1 year ago

Now clicking an image in FF opens and closes Image Viewer instantly

fbellgr commented 1 year ago

Now clicking an image in FF opens and closes Image Viewer instantly

@Razunter

Try dragging slightly the mouse before releasing. It remains open if you do that. I'd rather have to do that than always having it switch to the next image.

Razunter commented 1 year ago

Yeah, but that's horrible. Trying to figure out a better solution.

fbellgr commented 1 year ago

I'm grateful for any step in a less annoying direction. There is a lot to do, and I'm sure the coming release of SDXL 1.0 will bring a lot of work.

Razunter commented 1 year ago

We can't control Gradio and can't control order of events, but we can stop propagation if we detect Click on the parent element:

function setupImageForLightbox(e) {
  if (e.dataset.modded) return;
  e.dataset.modded = true;
  e.style.cursor = 'pointer';
  e.style.userSelect = 'none';
  e.parentNode.addEventListener('click', (evt) => {
    if (evt.button !== 0) return;
    if (evt.target.nodeName === 'IMG' && !evt.target.parentNode.classList.contains('thumbnail-item')) {
      const initialZoom = (localStorage.getItem('modalZoom') || true) === 'yes';
      modalZoomSet(gradioApp().getElementById('modalImage'), initialZoom);
      evt.preventDefault();
      showModal(evt);
    }
  }, true);
}

This probably should be refactored to add the event listener once, since its target now is the same element for all images.

fbellgr commented 1 year ago

Looks good, @Razunter.

@vladmandic The code supplied by @Razunter does appear to work perfectly in both Firefox and Chromium.

vladmandic commented 1 year ago

ok, i'm reopening. @Razunter might as well submit a pr, i'll review and approve it it works tomorrow.