Set SVG dimensions if absent

[mike-000]

Fixes #14196
Fixes #14237
where the image is same origin or CORS enabled

If an SVG can be fetched as in #14841 it is possible to check that is has dimensions and if absent set them based on any partial dimensions, viewBox and browser defaults to provide consistent behaviour on all browsers and regardless of whether color is specified.

Additionally if viewBox is present preserveAspectRatio is explicitly disabled. This is necessary to scale icons and images loaded via ol/Image#load in Firefox even if dimensions are set, as observed in #15013 (comment).

[github-actions]

📦 Preview the website for this branch here: https://deploy-preview-14933--ol-site.netlify.app/.

[ahocevar]

I'm not going to pursue the effort in #14841 further. Instead, I'm working on an API addition to allow specifying loaders for image layers, and provide some predefined loaders. Your effort here could then become such a predefined loader. See #14945.

I'm not sure when I'll be able to finish that effort, but until then let's keep this on hold.

[ahocevar]

The new loaders were designed to be modular. If you want an SVG specific loader, create one. Don't pack that functionality into the base loader, please.

[ahocevar]

Instead of these changes, it would be more elegant to extend the current img property to also accept a Promise resolving to a loaded image. Then any of the new load functions can be used with icon styles as well.

[mike-000]

Would a user ever want to select a loading method for an icon? How would caching work with img? SVGs are routinely used as icons so if we already have svgLoad it is the ideal method and could be used within IconImage.load to simplify the code:

svgLoad(this.image_, this.src_).then(
  this.handleImageLoad_.bind(this),
  this.handleImageError_.bind(this)
);

[ahocevar]

Adding magic to handle a very specific corner case can also cause things that do not match that corner case to fail magically, and if so, things tend to be hard to debug. Not to mention the code complexity added by such magic.

Along these lines, I'd also recommend against naming the new loader svgLoad and advertising it as all-purpose loader. It does not need to handle anything else than SVGs with intrinsic sizing, and can be offered for the one corner case where users cannot control what kind of SVG they get.

The safest thing to do is to document that icons, if SVG, cannot have intrinsic sizing. It is easy to create an SVG without intrinsic sizing, and it is easy to document.

Allowing the Icon style to handle promises for images, on the other hand, adds flexibility for users that know what they are doing.

[mike-000]

It is easy to create an SVG without intrinsic sizing,

Many of the problem icons are from third-party sites, and as I discovered it is not just unsized icons
One of our own test icons on Chrome and Firefox https://codesandbox.io/s/modify-icon-forked-fk7lfc?file=/main.js

Using the img option seems overcomplicated. Why not add a load option to ol/style/Icon (as in the createLoader) and call that inside the IconImage.load function. The default would need to be load or decodeFallback to handle compliant SVGs which need to be scalable for pixel ratio or icon scale (the current code is equivalent to using load).

[ahocevar]

Before committing to code, it might make sense to have a clear understanding of what currently works without any special configuration, what works with the existing imgSize option, and what does not work at all. Is it just intrinsic sizing, or are some of the cases that due not work due to invalid SVG images? Can this be put in a table with columns for what's present in the SVG (width/height, viewBox)?

[mike-000]

One or both dimensions is missing or a percentage

Firefox

Loaded image size is [0, 0].

drawImage does nothing (overriding with imgSize has no effect).

Other browsers

Loaded image size is based on the default of [300, 150] and the aspect ratio of viewBox if present, e.g. viewBox 0,0,900,900 produces [150, 150] image.

If only one dimension is present the missing one will be based on that and the viewBox or default aspect ratio.

drawImage does not use the loaded image size (overriding with imgSize also has no effect) and instead draws the image based on the size of the canvas it is drawing to.

createImageBitmap throws an error.

SVG has both dimensions and a viewBox

Firefox

drawImage ignores scaling requests and draws the image at its original aspect ratio (unless preserveAspectRatio none is set in the SVG).

It is very slow - reprojecting a large SVG such as from ImageArcGISRest freezes the browser for 20+ seconds at each map move.

Other browsers

drawImage scales the image as it would any other image type.

Speed is acceptable (2 seconds to reproject a large SVG).

[mike-000]

I presume what you are trying to achieve when specifying a loader in the img option is something like

import {load} from 'ol/Image';

this.loader_ = options.img ? toPromise(options.img) : load;

in the constructor and then calling

  this.loader_(this.image_, this.src_).then(
    (image) => {
      this.image_ = image;
      this.handleImageLoad_();
    },
    this.handleImageError_
  );

inside the existing the IconImage load method?

Using the img option would not be a breaking change, but for a new version it could perhaps be renamed to reflect its new purpose. imgSize would not be mandatory as it was with IE since valid loaded images (plus canvases, etc.) have a size.

[mike-000]

Dropping this in favour of a simpler SVG targeted loader which does not need CORS access but is sufficient to fix the browser-specific scaling and performance issues in #15013.

[ahocevar]

@mike-000 Why don't you pursue these ideas to fix the browser-specific scaling and performance issues further as a separate library? I think this could be useful outside of OpenLayers as well.

[mike-000]

There are examples of injecting dimensions or preserveAspectRatio="none" into SVGs to make them browser compatible in Stack Overflow but, as here, they require CORS access to fetch the source. That could easily be made into a utility function, e.g. takes a url and returns a promise of an object or data url for a more compliant SVG, or as with OpenLayers takes an image and src and asynchronously sets a more more compliant src. But that would not fix the performance problem with Firefox. I investigated that further and found it only happens when drawing an SVG to a context which has rotation or skew (so it only affects reprojection) - very high CPU usage which in the example gets worse as you zoom in and will eventually freeze the system and force a reboot.

The scaling issue is not specific to reprojection (but scaling will always be needed for reprojection). It does not affect the example as the SVG used does not have viewBox.

The fallback I added to #15013 uses an intermediate canvas which avoids both the performance and scaling issues, but there is a limit to the size of an image which Firefox will draw, which therefore limits the size of the intermediate canvas. Elsewhere as in #14099 #14403 #14947 polyfills will fix inconsistent or problem browser behaviour but I do not think that is an option in this case. While a polyfill would know if it was drawing an HTMLImageElement it could only rely on the src to know if that was an SVG (either checking the extension or looking for WMS or ArcGISRest parameters) and creating a large intermediate canvas for each drawImage call would be very inefficient.

Reprojecting a single image directly gives much better results than using a stitchContext (there is some improvement even for scaled non-SVG images) but having an example which could freeze systems would not be acceptable. I think putting a simple fallback into a loader where an SVG is expected as in #15013 is both efficient and failsafe.

[ahocevar]

@mike-000 I'm talking about the svgLoad() function only. Wouldn't that make a nice library on npmjs.com? With a configurable upscaled size even?

[mike-000]

Configurable upscale size so it could be more appropriate for icons? With a large number of icons there is a noticeable performance hit when using SVGs directly even with Chrome https://codesandbox.io/s/yfhh32?file=/main.js - uncomment the 'icon-color': 'transparent', and it is much more responsive. Clearly a loader returning an imageBitmap would be better but to avoid blurring issues it would need to upscale sufficiently to accommodate a reasonable likely maximum display size and pixel ratio, then return the scale as well as the bitmap so the image could be rendered based on the original scale. I think that would be a separate loader which could be used unconditionally for icons regardless of content type or browser (plus some changes to rendering to handle it), while svgLoad in #15013 is specific to problems with SVGs in a few browsers.

[ahocevar]

Again, I'm not talking about a loader for OpenLayers, but a separate library that can help users in fixing SVG images on the fly.

For a well designed OpenLayers application, I'd never use SVG icons, but PNG sprite sheets with conditional selection of different sizes depending on device pixel ratio.

[mike-000]

Again, I'm not talking about a loader for OpenLayers, but a separate library that can help users in fixing SVG images on the fly.

I would consider the svgLoad in this pull request to be general purpose but it has the limitation that it needs CORS access to work as intended. It fixes an unsized SVG to the size which Chrome would return and draw correctly to a canvas of the same size (as it would with an icon with color in Openlayers) so it works consistently with drawImage on all browsers regardless of canvas size, but the loader does not upscale. It would be well suited to problematic SVG icons in OpenLayers but could be used outside OpenLayers, either as a loader or simply to generate a browser compatible data url or object url. In OpenLayers image reprojection it did (with CORS) fix scaling issues in Firefox, but not the performance issue, so it was unusable on its own.

The separate svgLoad suggested in #15013 is a better alternative to checking the user agent in ReprojImage and falling back to using stitchContext with Firefox (the quality was better and performance acceptable) I did not intend any use for it beyond that, although the canScaleSVG() method which it uses might be useful elsewhere as it is not dependent on user agent. To work with unsized SVGs the decodeFallback method which it uses would need be to replaced with the original method used here.