Skip to content

IE9 fixes - to make SVG maps compatible #1332

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

Merged
merged 9 commits into from
Feb 1, 2017
1 change: 1 addition & 0 deletions src/components/drawing/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ drawing.lineGroupStyle = function(s, lw, lc, ld) {
};

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. Maybe this also fixes #166 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

var dlw = Math.max(lineWidth, 3);

if(dash === 'solid') dash = '';
Expand Down
2 changes: 1 addition & 1 deletion src/components/modebar/modebar.js
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ proto.createButton = function(config) {
}

button.setAttribute('data-toggle', config.toggle || false);
if(config.toggle) button.classList.add('active');
if(config.toggle) d3.select(button).classed('active', true);

button.appendChild(this.createIcon(config.icon || Icons.question));
button.setAttribute('data-gravity', config.gravity || 'n');
Expand Down
8 changes: 8 additions & 0 deletions src/lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -556,6 +556,14 @@ lib.isIE = function() {
return typeof window.navigator.msSaveBlob !== 'undefined';
};

/**
* Duck typing to recognize a d3 selection, mostly for IE9's benefit
* because it doesn't handle instanceof like modern browsers
*/
lib.isD3Selection = function(obj) {
return obj && (typeof obj.classed === 'function');
};


/**
* Converts a string path to an object.
Expand Down
29 changes: 18 additions & 11 deletions src/lib/loggers.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,7 @@ loggers.log = function() {
messages.push(arguments[i]);
}

if(console.trace) {
console.trace.apply(console, messages);
} else {
console.log.apply(console, messages);
}
apply(console.trace || console.log, messages);
}
};

Expand All @@ -44,11 +40,7 @@ loggers.warn = function() {
messages.push(arguments[i]);
}

if(console.trace) {
console.trace.apply(console, messages);
} else {
console.log.apply(console, messages);
}
apply(console.trace || console.log, messages);
}
};

Expand All @@ -60,6 +52,21 @@ loggers.error = function() {
messages.push(arguments[i]);
}

console.error.apply(console, arguments);
apply(console.error, messages);
}
};

/*
* Robust apply, for IE9 where console.log doesn't support
* apply like other functions do
*/
function apply(f, args) {
if(f.apply) {
f.apply(f, args);
}
else {
for(var i = 0; i < args.length; i++) {
f(args[i]);
}
}
}
4 changes: 3 additions & 1 deletion src/plots/cartesian/graph_interact.js
Original file line number Diff line number Diff line change
Expand Up @@ -792,7 +792,9 @@ fx.loneHover = function(hoverItem, opts) {
};

fx.loneUnhover = function(containerOrSelection) {
var selection = containerOrSelection instanceof d3.selection ?
// duck type whether the arg is a d3 selection because ie9 doesn't
// handle instanceof like modern browsers do.
var selection = Lib.isD3Selection(containerOrSelection) ?
containerOrSelection :
d3.select(containerOrSelection);

Expand Down
2 changes: 1 addition & 1 deletion src/traces/choropleth/attributes.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ module.exports = extendFlat({}, {
marker: {
line: {
color: ScatterGeoMarkerLineAttrs.color,
width: ScatterGeoMarkerLineAttrs.width
width: extendFlat({}, ScatterGeoMarkerLineAttrs.width, {dflt: 1})
}
},
hoverinfo: extendFlat({}, plotAttrs.hoverinfo, {
Expand Down
2 changes: 1 addition & 1 deletion src/traces/choropleth/plot.js
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ plotChoropleth.style = function(geo) {
d3.select(this)
.attr('fill', function(pt) { return sclFunc(pt.z); })
.call(Color.stroke, pt.mlc || markerLine.color)
.call(Drawing.dashLine, '', pt.mlw || markerLine.width);
.call(Drawing.dashLine, '', pt.mlw || markerLine.width || 0);
});
});
};
Expand Down
22 changes: 17 additions & 5 deletions tasks/test_syntax.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ var bundleTestGlob = path.join(constants.pathToJasmineBundleTests, '**/*.js');

// main
assertJasmineSuites();
assertHeaders();
assertSrcContents();
assertFileNames();
assertCircularDeps();

Expand Down Expand Up @@ -44,8 +44,12 @@ function assertJasmineSuites() {
});
}

// check for header in src and lib files
function assertHeaders() {
/*
* tests about the contents of source (and lib) files:
* - check for header comment
* - check that we don't have .classList
*/
function assertSrcContents() {
var licenseSrc = constants.licenseSrc;
var licenseStr = licenseSrc.substring(2, licenseSrc.length - 2);
var logs = [];
Expand All @@ -56,7 +60,15 @@ function assertHeaders() {

// parse through code string while keeping track of comments
var comments = [];
falafel(code, {onComment: comments, locations: true}, function() {});
falafel(code, {onComment: comments, locations: true}, function(node) {
// look for .classList
if(node.type === 'MemberExpression') {
var parts = node.source().split('.');
if(parts[parts.length - 1] === 'classList') {
logs.push(file + ' : contains .classList (IE failure)');
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea!

}
}
});

var header = comments[0];

Expand All @@ -70,7 +82,7 @@ function assertHeaders() {
}
});

log('correct headers in lib/ and src/', logs);
log('correct headers and contents in lib/ and src/', logs);
});
}

Expand Down
167 changes: 167 additions & 0 deletions test/jasmine/tests/lib_test.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
var Lib = require('@src/lib');
var setCursor = require('@src/lib/setcursor');
var overrideCursor = require('@src/lib/override_cursor');
var config = require('@src/plot_api/plot_config');

var d3 = require('d3');
var Plotly = require('@lib');
Expand Down Expand Up @@ -1599,6 +1600,172 @@ describe('Test lib.js:', function() {
expect(Lib.isPlotDiv({})).toBe(false);
});
});

describe('isD3Selection', function() {
var gd;

beforeEach(function() {
gd = createGraphDiv();
});

afterEach(function() {
destroyGraphDiv();
Plotly.setPlotConfig({ queueLength: 0 });
});

it('recognizes real and duck typed selections', function() {
var yesSelections = [
d3.select(gd),
// this is what got us into trouble actually - d3 selections can
// contain non-nodes - say for example d3 selections! then they
// don't work correctly. But it makes a convenient test!
d3.select(1),
// just showing what we actually do in this function: duck type
// using the `classed` method.
{classed: function(v) { return !!v; }}
];

yesSelections.forEach(function(v) {
expect(Lib.isD3Selection(v)).toBe(true, v);
});
});

it('rejects non-selections', function() {
var notSelections = [
1,
'path',
[1, 2],
[[1, 2]],
{classed: 1},
gd
];

notSelections.forEach(function(v) {
expect(Lib.isD3Selection(v)).toBe(false, v);
});
});
});

describe('loggers', function() {
var stashConsole,
stashLogLevel;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because toEqual doesn't think arguments is a regular array

messages.push([name, args]);
};

if(!hasApply) out.apply = undefined;

return out;
}

function mockConsole(hasApply, hasTrace) {
var out = {
MESSAGES: []
};
out.log = consoleFn('log', hasApply, out.MESSAGES);
out.error = consoleFn('error', hasApply, out.MESSAGES);

if(hasTrace) out.trace = consoleFn('trace', hasApply, out.MESSAGES);

return out;
}

beforeEach(function() {
stashConsole = window.console;
stashLogLevel = config.logging;
});

afterEach(function() {
window.console = stashConsole;
config.logging = stashLogLevel;
});

it('emits one console message if apply is available', function() {
var c = window.console = mockConsole(true, true);
config.logging = 2;

Lib.log('tick', 'tock', 'tick', 'tock', 1);
Lib.warn('I\'m', 'a', 'little', 'cuckoo', 'clock', [1, 2]);
Lib.error('cuckoo!', 'cuckoo!!!', {a: 1, b: 2});

expect(c.MESSAGES).toEqual([
['trace', ['LOG:', 'tick', 'tock', 'tick', 'tock', 1]],
['trace', ['WARN:', 'I\'m', 'a', 'little', 'cuckoo', 'clock', [1, 2]]],
['error', ['ERROR:', 'cuckoo!', 'cuckoo!!!', {a: 1, b: 2}]]
]);
});

it('falls back on console.log if no trace', function() {
var c = window.console = mockConsole(true, false);
config.logging = 2;

Lib.log('Hi');
Lib.warn(42);

expect(c.MESSAGES).toEqual([
['log', ['LOG:', 'Hi']],
['log', ['WARN:', 42]]
]);
});

it('falls back on separate calls if no apply', function() {
var c = window.console = mockConsole(false, false);
config.logging = 2;

Lib.log('tick', 'tock', 'tick', 'tock', 1);
Lib.warn('I\'m', 'a', 'little', 'cuckoo', 'clock', [1, 2]);
Lib.error('cuckoo!', 'cuckoo!!!', {a: 1, b: 2});

expect(c.MESSAGES).toEqual([
['log', ['LOG:']],
['log', ['tick']],
['log', ['tock']],
['log', ['tick']],
['log', ['tock']],
['log', [1]],
['log', ['WARN:']],
['log', ['I\'m']],
['log', ['a']],
['log', ['little']],
['log', ['cuckoo']],
['log', ['clock']],
['log', [[1, 2]]],
['error', ['ERROR:']],
['error', ['cuckoo!']],
['error', ['cuckoo!!!']],
['error', [{a: 1, b: 2}]]
]);
});

it('omits .log at log level 1', function() {
var c = window.console = mockConsole(true, true);
config.logging = 1;

Lib.log(1);
Lib.warn(2);
Lib.error(3);

expect(c.MESSAGES).toEqual([
['trace', ['WARN:', 2]],
['error', ['ERROR:', 3]]
]);
});

it('logs nothing at log level 0', function() {
var c = window.console = mockConsole(true, true);
config.logging = 0;

Lib.log(1);
Lib.warn(2);
Lib.error(3);

expect(c.MESSAGES).toEqual([]);
});
});
});

describe('Queue', function() {
Expand Down