Legacy IE (7-9) Compatibility

[austinhyde]

These changes add compatibility for Internet Explorer 7-9 for most existing features of the DOM renderer, as well as some small improvements to the DOM renderer.

Namely:

• Adds naturalWidth and naturalHeight properties to images loaded by ImageTile, needed to render image based tile sources (41fa53b)
• Fixes issues with mouse events in IE 7-8 (e92d366)
• Fallback to non-rgba styles in older browsers (3013674)
• Use explicit DOM nodes for the zoom control instead of CSS :before and :after pseudo-classes (a4901e6)
• Fix issues with dragging behavior in IE 7-8, where IE would interpret click+move as either image drag or text selection (faef6a3)
• Switch between 2D/3D CSS transforms based on feature detection, not hardcoded booleans (7d1df4c)
• Add support for matrix-based transforms in IE 7-8 via CSS filter and in IE 9 via prefixed CSS property, needed for smooth zoom animations (7d1df4c, e5d0822)
• Add support for layer opacity in IE 7-8, via CSS filter (3f220c5)
• Fix issue with an IIFE calling Canvas API during script load - instead defer until needed (29bb04e)
• Fix issue with DOM renderer layers getting out of order after consecutive remove+inserts (9e9d9a0)

As a result, the following features work smoothly on Internet Explorer 7-9:

• Panning via mouse drag
• Zooming via scroll wheel
• Zooming via zoom control
• Zooming via zoom slider control
• Zooming via shift+drag (just the interaction, not the rendering)
• Zooming via double click
• Zoom control styling
• Zoom slider control styling
• Attribution control styling
• Zoom animations
• Pan animations (particularly kinetic panning)
• Tile layer rendering
• Image layer rendering
• HTML Overlays
• Multiple overlaid layers

Things that do not work in IE 7-9, and are not considered in this PR:

• Map rotation (that's quite the rabbit hole...)
• Vector layer rendering - in the works
• DrawFeature interaction
• FullScreen control styling
• ScaleLine control styling
• ZoomToExtent control styling
• Mobile styling and interactions
• DragZoom box rendering

Feel free to cherry-pick this or let me know if you think more needs done before accepting.

[twpayne]

Wow @austinhyde, this is absolutely fantastic!

[twpayne]

This is great work @austinhyde. I've made a couple of comments, but am +lots on this. I don't have access to Windows machine to test it, but the code looks very good.

[austinhyde]

Very minor, but recently we've been using

layerRender = this.getLayerRenderer(layer);
goog.asserts.assertInstanceof(layerRenderer, ol.renderer.dom.Layer);

instead of a typecast.

That makes sense, but I still need the typecast to make the Closure Compiler happy, because ol.renderer.Map#getLayerRenderer returns an ol.renderer.Layer. I'll add the assertion just for runtime safety though.

[twpayne]

The compiler can infer types from goog.asserts.assertInstanceof, so if you use assertInstanceof then you don't need a typecast.

[austinhyde]

Interesting... I tried with only the assertion and the compiler errored because "getTarget is not defined on ol.renderer.Layer"

[twpayne]

Interesting... I tried with only the assertion and the compiler errored because "getTarget is not defined on ol.renderer.Layer"

Are you sure you were asserting that layerRenderer is an instance of ol.renderer.dom.Layer and not just an ol.renderer.Layer?

It's possible that we need to give the compiler a couple more hints here. Currently layerRenderer is declared in a var statement without being given a type. It might need to be declared with a type (at around line 93):

/** @type {ol.renderer.dom.Layer} */
var layerRenderer;

[austinhyde]

/** @type {ol.renderer.dom.Layer} */
var layerRenderer;
...
  layerRenderer = this.getLayerRenderer(layer);
  goog.asserts.assertInstanceof(layerRenderer, ol.renderer.dom.Layer);

results in

found   : (null|ol.renderer.Layer)
required: (null|ol.renderer.dom.Layer)
    layerRenderer = this.getLayerRenderer(layer);

[twpayne]

Interesting. This approach works elsewhere (e.g. fd733ec) but obviously something else is going on here. So, please keep the typecast for the moment and we can debug this later.

[elemoine]

Vector layer rendering - in the works

What do you mean?

Impressive work @austinhyde! I think we'll discuss this work during our next hangout meeting on Thursday. And feel free to join the meeting if you're interested in attending.

[austinhyde]

Vector layer rendering - in the works

What do you mean?

I don't want to promise anything, but we're experimenting with implementing an ol.renderer.dom.VectorLayer to support vector drawing in older browsers (namely, IE 7-8) that don't support Canvas or WebGL. There was some interest, at least from @twpayne back in November, in an implementation of this.

[pagameba]

This is awesome, support for legacy IE will definitely make a big difference in uptake by the community and additional contributions by people that might not have considered ol3 if it lacked this.

One minor comment, not intending to derail this, but is there some way to extract this such that it could be loaded as a shim in some conditional comments targeting older browsers? Although there is very little code in here for the value added, it does add some bulk and complexity to the code and css that isn't needed for people targeting only newer browsers. Also, I assume potential vector support for legacy IE will add more code?

[elemoine]

getEventPixel and getEventCoordinate are user-API functions, and we do not expose goog types/objects to the user. We could add getEventPixelInternal and getEventCoordinateInternal functions to work around that.

[austinhyde]

That definitely makes sense. As @twpayne mentioned earlier, this commit is actually a subset of the changes from #1588. If you think that *Internal methods are a good intermediate solution, I can make that change.

[twpayne]

As it turned out, #1588 did not need this changes (thanks @elemoine). So, @austinhyde it would be great if you could add getEventPixelInternal and getEventCoordinateInternal functions, thanks.

[austinhyde]

One minor comment, not intending to derail this, but is there some way to extract this such that it could be loaded as a shim in some conditional comments targeting older browsers?

I can't think of anyway an additional shim could work well, because of how thoroughly the Closure Compiler optimizes and minifies the code. What you could do if this is accepted, is make a new branch on a local checkout, use git revert to get reverse the legacy IE support commits, then rebuild OL. This would create a "modern-only" build you can include alternately from the "legacy-compatible" default OL build. Aside from that, I'm fresh out of ideas.

Although there is very little code in here for the value added, it does add some bulk and complexity to the code and css that isn't needed for people targeting only newer browsers.

This PR only introduces a ~0.8% increase in minified code size (+3KB out of a total 263KB) - hardly anything to be worried about. Performance-wise, most of the JS changes only target the DOM render (and thus, only legacy browsers), and even then, there aren't any real performance hits.

Also, I assume potential vector support for legacy IE will add more code?

Yes, but I have no idea how much yet.

[twpayne]

A good way to solve this is to add an ol.OLD_IE_SUPPORT @define. These can then be used in if statements through the code. When the user builds ol3, he can specify true or false for this @define and the code will be automatically included or excluded as appropriate.

[twpayne]

Example:

/**
 * @define {boolean} Support old versions of IE.
 */
ol.OLD_IE_SUPPORT = true;

// ...

ol.someFunction = function() {
  if (ol.OLD_IE_SUPPORT) {
    // code in this block will only be included in the compiled output if ol.OLD_IE_SUPPORT is true
  }
  if (ol.OLD_IE_SUPPORT && goog.userAgent.IE && !goog.userAgent.isVersionOrHigher('9.0')) {
    // this whole if statement will be removed if ol.OLD_IE_SUPPORT is false
  }
};

[austinhyde]

I can add the @define's as well; that sounds like a very reasonable approach.

[twpayne]

If you're feeling really ambitious, you could make ol.OLD_IE_SUPPORT a number (e.g. 7 or 8) so that people can enable only IE7+ or IE8+.

[austinhyde]

I don't see too much value in that, as there's very little difference between 7 and 8, at least as far as these changes go.

[twpayne]

OK, in that case I agree that it makes more sense to keep it as a boolean.

[twpayne]

@austinhyde, please say if we can help with this PR. I've just been working with IE (in issue #1639) and would love to see this rebased on the latest master and merged.

[austinhyde]

This is all very stable, as far as I can tell, and hand-tested extensively on IE7 and 8, both native and emulated in IE11.

When you say "rebased", do you mean a literal git rebase master (which will almost certainly break history and comment references here), or do you just want me to git merge master to resolve conflicts before merging the PR through GitHub? I suspect there will be quite a few conflicts to work out, either way. Or did you mean something else?

[twpayne]

Please do a git rebase master. It will maintain the history of commits, allow any conflicts to be resolved, and the commit references here don't matter: they'll just be no-longer-interesting minutiae of history soon. (If you're like a philosophical debate about rebase or merge, then this blog post summarises my personal view well).

[austinhyde]

Alright, everything's rebased. Let me know if you need anything else!

[twpayne]

Thanks @austinhyde! It works great in IE7, but I'm getting just a blank map for the simple example in IE8, with no tiles displayed (I've set ol.LEGACY_IE_SUPPORT to true, but have not otherwise modified the code). There's nothing in IE8's dev console either. What information do you need to debug this? (I'm using IE8 in a VM from modern.ie.

It looks like resources/example-behaviour.js also needs a bit of TLC: IE8 doesn't have addEventListener (used at line 57).

[austinhyde]

It looks like the issue with the examples is two-fold: first, example-behavior.js is using document.querySelector() and addEventListener(). I solved this by including the already-present jQuery, and using $(selector) and $(element).change(), respectively. The second issue was a problem with my changes to dom/dom.js: if the map has a background (like the examples), the z-index fix that allows smooth panning and zooming actually puts the map layers behind the map. My last two commits fix these issues, at least for the simple example.

Looking through some of the other examples, there are a number of them that ought to work, but won't because of things like addEventListener(). Perhaps it would be best to get these changes merged in, and work through the examples in another pull request?

[twpayne]

Thanks for the fix - it looks good.

I'm +1 on merging this now. As you suggest, we can fix the other examples in a separate PR.

Any other reviews/comments before merging?

[austinhyde]

Not from me - merge whenever you want.

[twpayne]

We discussed this at the Hangout last Thursday, and everybody was for merging this. I was hoping that they would also post a comment here in the PR, but obviously that is not to be.

So, merging this.

[ahocevar]

+1 from me too. Great work!

[kaleb]

👍