Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Allow QuickView image preview for arbitrary URLs missing image known extension #10786

Closed
humphd opened this issue Mar 23, 2015 · 6 comments
Closed

Comments

@humphd
Copy link
Contributor

humphd commented Mar 23, 2015

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?

@humphd
Copy link
Contributor Author

humphd commented Mar 23, 2015

Something like this: humphd@542cd1d

@marcelgerber
Copy link
Contributor

@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.

@humphd
Copy link
Contributor Author

humphd commented Mar 25, 2015

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.

@marcelgerber
Copy link
Contributor

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.

@humphd
Copy link
Contributor Author

humphd commented Mar 27, 2015

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.

@marcelgerber
Copy link
Contributor

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 subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants