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

[CLOSED] Allow QuickView image preview for arbitrary URLs missing image known extension #9491

Open
core-ai-bot opened this issue Aug 30, 2021 · 6 comments

Comments

@core-ai-bot
Copy link
Member

Issue by humphd
Monday Mar 23, 2015 at 20:22 GMT
Originally opened as adobe/brackets#10786


Currently the code for QuickView's image preview depends on a URL having a known image extension:

            if (/^(data\:image)|(\.gif|\.png|\.jpg|\.jpeg|\.svg)$/i.test(tokenString)) {

This is fine in the general case, but falls down when you use an image from a web service. For example, my gravatar image is https://avatars3.githubusercontent.com/u/427398, which will fail this regex and shows no preview, despite the fact that the image can be loaded correctly.

I think this can be improved. Since the preview container is not shown until the preview img element's load event fires, it would be possible to simply try loading a given URL without bothering to check the extension at all. The current code is:

                  var showHandler = function () {
                        // Hide the preview container until the image is loaded.
                        $previewContainer.hide();

                        $previewContainer.find(".image-preview > img").on("load", function () {
                            $previewContent
                                .append("<div class='img-size'>" +
                                            this.naturalWidth + " &times; " + this.naturalHeight + " " + Strings.UNIT_PIXELS +
                                        "</div>"
                                    );
                            $previewContainer.show();
                            positionPreview(editor, popoverState.xpos, popoverState.ytop, popoverState.ybot);
                        });
                    };

After setting the img.src to the URL, there are two outcomes: a load or error event. In the load case, the preview would be shown as normal; in the error case, hidePreview() could be called to cancel the pending preview--this is possibly something that should be done anyway, for the case that an image can't be loaded via the URL (e.g., 404).

Would you consider a patch to do this?

@core-ai-bot
Copy link
Member Author

Comment by humphd
Monday Mar 23, 2015 at 23:55 GMT


Something like this: humphd/brackets@542cd1d

@core-ai-bot
Copy link
Member Author

Comment by MarcelGerber
Wednesday Mar 25, 2015 at 18:57 GMT


@humphd Great idea, but a little feedback from my side:
So, Quick View shows a preview for any kind of URL, no matter whether it's in a href or a src attribute, or in a CSS or HTML file (there are probably more variations, but yeah).
Thus, if a href points to 50MB .json file, with your suggestion implemented, Brackets would download it just to see it's not an image file to then throw it away. Same goes for a HTML file, or whatsoever. That's additional traffic on both the server side and the client side.

Thus, I'd suggest two things, maybe do both, maybe just of them:

  1. Only download extension-less files and those with known extensions -- How likely is it a .html file is actually a picture? Rather than downloading it just to make sure it isn't, just ignore it and focus on those that may be pictures, which are those with known image extensions (gif, jpg, jpeg, png, svg) and, to address your issue, those without an extension.
  2. HEAD request first -- To prevent Brackets from actually downloading a 50MB JSON file, it sends a HEAD request first so it can see whether the Content-Type is a known image format. That should suffice.

@core-ai-bot
Copy link
Member Author

Comment by humphd
Wednesday Mar 25, 2015 at 20:37 GMT


Browsers already do the optimization you suggest per the HTML spec. Specifically, if an img element's src is set, an image request is initiated on the network and image sniffing rules take over, which use the Content-Type headers, see https://html.spec.whatwg.org/multipage/embedded-content.html#img-determine-type.

Adding a black-list of extensions to ignore (html, json, css, ...), which are specifically known to not yield useful previews would probably be good. I'll add that to my patch.

@core-ai-bot
Copy link
Member Author

Comment by MarcelGerber
Wednesday Mar 25, 2015 at 20:55 GMT


I suggested to do a HEAD request first so the browser doesn't need to download the whole file.
So if it's a 50MB JSON file, the browser only downloads the small header portion (maybe 1-2KB) to see it's not a picture and throw it away.

@core-ai-bot
Copy link
Member Author

Comment by humphd
Friday Mar 27, 2015 at 16:30 GMT


My PR is updated.

Also, I confirmed that Firefox does in fact do what I said above, namely, if you try to use a 50M JSON file as the src to an img, it will not download the whole file: the imagelib decoder cancels the download early. Chrome, on the other hand, does download the whole thing. I'll file a bug on that with them.

@core-ai-bot
Copy link
Member Author

Comment by MarcelGerber
Sunday Mar 29, 2015 at 18:13 GMT


I wouldn't call Chrome's behavior a bug -- usually, HTML is used on normal webpages, where an img's src almost certainly links to an actual image. Thus, doing only one request instead of two is faster in most cases (every time the Content-Type is actually a supported image type).
That behavior also boosts performance, especially in cases where the TTFB is high.

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

No branches or pull requests

1 participant