IE9 fixes - to make SVG maps compatible #1332

alexcjohnson · 2017-01-25T03:31:00Z

A bunch of random fixes for IE9 behavior, only one was actually in maps, oddly enough (so some edge cases may have been broken in other plot types too), but all of which are accessed by maps.

~~classList~~
~~instanceof d3.selection~~ -> seems to work for built-in types and some user-defined types, but for some reason d3.selection broke it, replaced with duck typing
~~console[fn].apply~~ - I guess these aren't regular functions in IE9, so if there's no apply just call the function once for each argument.
When an invalid attribute value is provided, IE9 throws an error rather than just using a default value. Choropleth with no explicit line width was trying to set undefinedpx via drawing.dashLine, which in all other browsers silently defaulted to 1px. I made this explicit at the supplyDefaults level, while at the same time defaulting to 0px in dashLine so this can't cause further problems.

cc @chriddyp @etpinard

Given the scattered nature of these fixes, it's probably worthwhile to have someone spend some time manually trying out more plots & behaviors, ideally on a real local copy of IE9 rather than Browserstack. I saw some sporadic glitches with drag interactions, including one where it seemed like the hover effects were pointing to the old point locations... but couldn't reproduce so I'm concerned it was just an artifact of tunneling through Browserstack.

alexcjohnson · 2017-01-25T05:26:37Z

tasks/test_syntax.js

+                if(node.type === 'MemberExpression') {
+                    var parts = node.source().split('.');
+                    if(parts[parts.length - 1] === 'classList') {
+                        logs.push(file + ' : contains .classList (IE failure)');


I was hoping eslint would have an option for IE compatibility (actually not even IE11 supports classList on SVG elements, apparently)... but I don't see one, so I added it in here. Confirmed that before the change I made to modebar.js it failed this test.

Great idea!

alexcjohnson · 2017-01-25T05:28:16Z

test/jasmine/tests/lib_test.js

+        function consoleFn(name, hasApply, messages) {
+            var out = function() {
+                var args = [];
+                for(var i = 0; i < arguments.length; i++) args.push(arguments[i]);


because toEqual doesn't think arguments is a regular array

etpinard · 2017-01-30T19:29:17Z

src/components/drawing/index.js

@@ -112,6 +112,7 @@ drawing.lineGroupStyle = function(s, lw, lc, ld) {
 };

 drawing.dashLine = function(s, dash, lineWidth) {
+    lineWidth = +lineWidth || 0;


Interesting. Maybe this also fixes #166 ?

Maybe - I'll try that tonight and see if it implies any further fixes. Any other IE issues I should include in my testing?

Maybe

[IE] Y-axis values overlap the title #1247

Malformed hovertext in IE11 #746

I think this does fix #166 - and 4aa1a45 should catch any of these nanpx or undefinedpx errors, at least as long as they go through d3.

Will investigate the other two tomorrow, they may be related.

etpinard · 2017-01-31T16:27:07Z

test/image/index.html

@@ -5,6 +5,7 @@
 <script type="text/javascript" src="../plotly.js/dist/extras/mathjax/MathJax.js?config=TeX-AMS-MML_SVG"></script>
 <script type="text/javascript" src="../plotly.js/build/plotly.js" charset="utf-8"></script>
 <script type="text/javascript" src="../plotly.js/dist/plotly-geo-assets.js" charset="utf-8"></script>
+<script type="text/javascript" src="../plotly.js/test/image/strict-d3.js" charset="utf-8"></script>


Great idea here!

First, I was thinking adding this to the image test index might lead to some very annoying debugging sessions when things go wrong. But, by adding it to the test-dashboard as well essentially make my concerns disappear (except for devs that don't use the test-dashboard cc @rreusser @monfera).

Alternatively, we could mock the d3.selection.prototype only in the ie9_test.js bundle test and test each ie9 falback one by one.

It comes down to choosing between a solution with great coverage (yours) and a less intrusive solution that might lead to less 🤕 down the road. I'm ok with both.

We've talked before about wanting a way to report errors out of the image tester - printing the stack trace to a blank image or something. My vote would be to leave this in and work toward that goal. Being strict here will help us even outside IE, as we really never want to implicitly rely on browser defaults.

IE puts a space between numbers in transforms, even if you set them with a comma

alexcjohnson · 2017-01-31T20:20:11Z

src/lib/index.js

@@ -451,8 +451,12 @@ lib.addStyleRule = function(selector, styleString) {
    else lib.warn('addStyleRule failed');
 };

-lib.getTranslate = function(element) {
+// TODO (alexcjohnson): the following translate/scale functions should
+// get moved to components/drawing rather than the generic lib.


I didn't want to move all of these (along with all their references and tests) without a discussion, but I did write a new one before finding this in here and discovering that it already tests the behavior I needed it to have for IE.

👍 for moving getTranslate and setTranslate out of Lib. Maybe putting them into Drawing would make the most sense?

Yeah, that's where I put mine before noticing these. Great, I'll move them in there.

done in 66baf51

Any final comments @etpinard ?

alexcjohnson · 2017-01-31T20:28:29Z

src/lib/svg_text_utils.js

+    // Chrome and FF display \n, \r, or \r\n as a space in this mode.
+    // I feel like at some point we turned these into <br> but currently we don't so
+    // I'm just going to cement what we do now in Chrome and FF
+    _str = _str.replace(NEWLINES, ' ');


As far as I can tell this is purely a bug in IE - though one that persists from 9 through 11... the affected text is invisible, even though getBoundingClientRect() on its node shows the correct position and shape, and it reappears when you take out the newline character.

🥇 debugging.

alexcjohnson · 2017-01-31T20:45:56Z

test/jasmine/tests/hover_label_test.js

@@ -123,7 +125,8 @@ describe('hover info', function() {

            expect(d3.selectAll('g.axistext').size()).toEqual(0);
            expect(d3.selectAll('g.hovertext').size()).toEqual(1);
-            expect(d3.selectAll('g.hovertext').select('text').html()).toEqual('hover text');
+            expect(d3.selectAll('g.hovertext').select('text').html())
+                .toEqual('hover text  with spaces  not newlines');


None of the image tests in 2105930 actually changed results with the fix there, but they would all have resulted in invisible text in IE before the fix. But this test confirms that we do sanitize newlines for IE compatibility.

Nicely done.

etpinard · 2017-02-01T14:54:20Z

💃

alexcjohnson added 3 commits January 24, 2017 17:42

IE9 fixes

98adfae

explicitly, rather than implicitly, default choropleth linewidth to 1

ebc1a10

tests for IE9 fixes

12aeb9d

alexcjohnson commented Jan 25, 2017

View reviewed changes

etpinard reviewed Jan 30, 2017

View reviewed changes

etpinard added status: reviewable bug something broken labels Jan 30, 2017

strict-d3: prevent stuff like 'nanpx' from selection.style

4aa1a45

etpinard reviewed Jan 31, 2017

View reviewed changes

alexcjohnson added 2 commits January 31, 2017 12:15

fix #1247 - titles avoid labels in IE

61ade94

IE puts a space between numbers in transforms, even if you set them with a comma

fix #746 issue with newline characters in text in IE

2105930

alexcjohnson commented Jan 31, 2017

View reviewed changes

test that svg text output has no newlines in it

0d555b4

alexcjohnson commented Jan 31, 2017

View reviewed changes

alexcjohnson added 2 commits January 31, 2017 16:42

move (set|get)(translate|scale|pointgroupscale) into components/drawing

66baf51

one more Lib->Drawing

fe12729

alexcjohnson merged commit e7f69c9 into master Feb 1, 2017

alexcjohnson deleted the ie9-maps branch February 1, 2017 15:12

alexcjohnson mentioned this pull request Feb 1, 2017

IE9 JS Plotly.redraw and Plotly.addTraces both throws Invalid Argument #166

Closed

Uh oh!

IE9 fixes - to make SVG maps compatible #1332

IE9 fixes - to make SVG maps compatible #1332

Uh oh!

Conversation

alexcjohnson commented Jan 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etpinard commented Feb 1, 2017

Uh oh!

Uh oh!