Allow to suppress onImageTap for linked images.

[jhass]

I have user content, namely I render parsed markdown content. Users sometimes embed images which I'd like to provide a lightbox for on tap. But sometimes users also wrap the image embed with a link for example for a link banner or a link to their own gallery page etc. In this case I want to open just the link and not show the lightbox.

Originally posted by @jhass in #121 (comment)

[ryan-berger]

@jhass Sorry for the issue getting closed.

I'm not sure that I follow the use case, but would you mind explaining it again with maybe some screenshots of how it is currently, and what you would like to happen? This seems doable, we just need to be careful and make sure there is a well defined use case and we don't introduce too many breaking changes.

[jhass]

It's very simple. I have user provided content that I render.

Sometimes the user content is like this:

<img src="http://placekitten.com/g/200/300" />

In this case, when the user taps on the element, I want to open a lightbox.

But other times the user content is like this:

<a href="http://placekitten.com"><img src="http://placekitten.com/g/200/300" /></a>

In this case, I don't want to open the lightbox, just the link! Currently, in this case both handlers are called, so both actions get executed. I guess I could try to manually sync them into a common method that waits for a timeout and ignores subsequent requests in the meantime, but it would be messy and racy.

[ryan-berger]

Oh, I see. You are proposing that if there is an image that is nested within an anchor tag, you suppress the one. This makes sense, and I think that it should be supported. I'll look into what this would entail.

Thank you for the detailed explanation!

[devhammed]

I have a temporary fix which is using a custom renderer for img tags and check if the parent is A tag in the gesture detector onTap handler but that also brings another issue, the image will not display if I use the child passed to the render function.

Will not work:

 customRender: {
          'img': (renderContext, child, attributes, node) {
            var src = attributes['src'];
            var alt = attributes['alt'] ?? src;

            return GestureDetector(
              onTap: () {
+                // ignore click on images wrapped in "A" tag.
+                if (node.parent?.localName == 'a') {
+                  return;
+                }

                Navigator.of(context).push(
                  MaterialPageRoute<Null>(
                    builder: (BuildContext context) {
                      return Scaffold(
                        appBar: AppBar(title: Text(alt)),
                        body: Container(
                          child: PhotoView(imageProvider: NetworkImage(src)),
                        ),
                      );
                    },
                    fullscreenDialog: true,
                  ),
                );
              },
              child: Padding(
                padding: EdgeInsets.symmetric(vertical: 20),
+                child: child,
              ),
            );
          }

Will work:

 customRender: {
          'img': (renderContext, child, attributes, node) {
            var src = attributes['src'];
            var alt = attributes['alt'] ?? src;

            return GestureDetector(
              onTap: () {
                // ignore click on images wrapped in "A" tag.
                if (node.parent?.localName == 'a') {
                  return;
                }

                Navigator.of(context).push(
                  MaterialPageRoute<Null>(
                    builder: (BuildContext context) {
                      return Scaffold(
                        appBar: AppBar(title: Text(alt)),
                        body: Container(
                          child: PhotoView(imageProvider: NetworkImage(src)),
                        ),
                      );
                    },
                    fullscreenDialog: true,
                  ),
                );
              },
              child: Padding(
                padding: EdgeInsets.symmetric(vertical: 20),
-                child: child,
+                child: Image.network(src),
              ),
            );
          }

[ryan-berger]

@devhammed Thank you for the example. This should help me when I implement it.

@jhass @devhammed I think that I would prefer to have the click event suppress-able with any tree, not just the case where one is the child of the other. Something like the browser's suppress events API. I'll see what I can get implemented this weekend

[devhammed]

@ryan-berger How about the issue I reported, when using child it won't display.

[ryan-berger]

@devhammed This is likely because you are using a custom renderer. The child of the image is not an instance of Image and therefore, you need to instantiate the Image yourself.

Is there anything wrong or broken about that?

[devhammed]

@ryan-berger It is broken because according to the Wiki, the child argument is the widget that will be rendered initially and we can wrap it with any widget.

[ryan-berger]

@devhammed Yes, that would be correct for most every other element, except for an img tag as it is a ReplacedElement and therefore does not technically have any children that aren't the image. ReplacedElement was made for things like the img and video tags as they don't have children and must be "replaced" in the tree.

A custom renderer however is a StyledElement and doesn't anything outside of that the image tag does not have any children, and therefore renders nothing, and leaves you to do that work. If you think this should be changed, please open another issue where we can discuss it further

[devhammed]

It should be changed for sure, let's say I just want to wrap the element only without changing the internal logic or another alternative is to expose the renderer so we can use it.

[ryan-berger]

@devhammed Please open another issue with a clear problem statement and proposal to fix it so we can stop spamming this thread (sorry @jhass), and I can take a look.

[ryan-berger]

@jhass I have taken a look more in depth and messed around with the code, and it definitely looks like a sticky situation.

The issue is that both types of onTap are generic, and not tied to a given image or link tag, but a catchall for all instances.

I also don't see how this feature will solve this particular problem you are experiencing. For example, if I have a link that has an image as a child, and have another element that is just a link, from the onTap callback how would I be able to tell which link is which? I would not be given any context to determine whether or not there are children that are images and defeat the purpose of suppression. How do you know that you want to suppress the child's callback? What if you find a use case where you want to suppress the parent's callback?

I might see the argument that we should pass in some context about the element to the onTap such as its attributes so you can identify it that way, but at that point, I don't see why a customRenderer shouldn't be used. It seems much more elegant, and fits your use case better

The only thing that I can think of to do at the moment is to make it so that either the parent or the child element gets the event, and I don't know if I like that as a solution.

I'm going to keep this issue open because I really am not opposed to the idea, I just think that there needs to be a strong proposal for how we accomplish it.

[jhass]

I also don't see how this feature will solve this particular problem you are experiencing. For example, if I have a link that has an image as a child, and have another element that is just a link, from the onTap callback how would I be able to tell which link is which?

For my current usecase I don't need to. For me it would be logical that the link eats the onImageTap event for its childs.

A common theme in event handler APIs is returning true or false in handlers, true meaning "I handled this event, stop its propagation" and false meaning "please propagate this event further". You could argue both are actually the same event, a tap, so returning true should stop the propagation. But then systems doing that also usually start at the leaf of the tree and go up, so that scheme would only serve to suppress onTap but not onImageTap and thus not fit my usecase very well. It's a hard problem with the current API, yeah :/

A custom renderer seems verbose, why would I need to reimplement the styling this library does (and keep it consistent), just to handle events? Maybe I'm missing something.

I guess the ideal API would be having some DOM and then being able to pass the DOM element to the handler so you could query the parent in it. But that's probably a lot of work from where this is right now.

[ryan-berger]

For my current usecase I don't need to. For me it would be logical that the link eats the onImageTap event for its childs.

Right, I guess that is my worry. I'm sure that someone is in the exact opposite boat, just hasn't piped up and opened an issue.

A custom renderer seems verbose, why would I need to reimplement the styling this library does (and keep it consistent), just to handle events? Maybe I'm missing something.

You could pretty easily use a custom renderer and return an ImageElement and pass in your own RenderContext with onTaps and such (this may not be the name, not at my desk). There shouldn't be a reason to re-implement EVERYTHING.

But yes, it is a very tricky issue, and I do like the generic "event handler propagation" way of doing it, but it seems like it is a bit hard to do without a breaking API change (which isn't bad in the long run, just hard to get working in the short term). It also seems strange to me to provide methods such as onLinkTap and onImageTap and allow them to suppress each other, but not have an underlying event system to handle all tap events.

When I really started digging in to see what it would take to implement something like e.stopPropogation() in Javascript, or returning a true/false value from an the event handler to see if I would be propagating the event, I realized I would be hardcoding a search up the tree to find an <a/> so I could handle suppression. That isn't super elegant, and is super specific to one small part of the codebase.

I think that if the library had more like a javascript-esque system where you can register events on particular DOM elements it would be easier to say yes to this and just start working on it, but with the API it has currently, it seems really hard.

Maybe open a proposal for basic DOM event handlers? I'd have to dig in on whether or not it is an acceptable idea in the first place, because I don't want to be supporting way too many events, or become some sort of browser.

[marcjoha]

Just to inject some life into this issue, I'm working around the problem using a trimmed down version of what @devhammed posted:

customRender: {
  'img': (_, __, attributes, node) {
    return GestureDetector(
      onTap: () {
        // Show full screen when clicked, unless it's a link target
        if (node.parent?.localName != 'a') {
          Navigator.of(context).push(MaterialPageRoute(
            builder: (context) => PhotoView(
              imageProvider: NetworkImage(attributes['src']),
            ),
          ));
        }
      },
      child: Image.network(attributes['src']),
    );
  }
}

[devhammed]

Just to inject some life into this issue, I'm working around the problem using a trimmed down version of what @devhammed posted:

customRender: {
  'img': (_, __, attributes, node) {
    return GestureDetector(
      onTap: () {
        // Show full screen when clicked, unless it's a link target
        if (node.parent?.localName != 'a') {
          Navigator.of(context).push(MaterialPageRoute(
            builder: (context) => PhotoView(
              imageProvider: NetworkImage(attributes['src']),
            ),
          ));
        }
      },
      child: Image.network(attributes['src']),
    );
  }
}

Now that is trimming down 🙌 🙌 🙌