-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Changes from 4 commits
98adfae
ebc1a10
12aeb9d
4aa1a45
61ade94
2105930
0d555b4
66baf51
fe12729
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,7 +13,7 @@ var bundleTestGlob = path.join(constants.pathToJasmineBundleTests, '**/*.js'); | |
|
||
// main | ||
assertJasmineSuites(); | ||
assertHeaders(); | ||
assertSrcContents(); | ||
assertFileNames(); | ||
assertCircularDeps(); | ||
|
||
|
@@ -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 = []; | ||
|
@@ -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)'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great idea! |
||
} | ||
} | ||
}); | ||
|
||
var header = comments[0]; | ||
|
||
|
@@ -70,7 +82,7 @@ function assertHeaders() { | |
} | ||
}); | ||
|
||
log('correct headers in lib/ and src/', logs); | ||
log('correct headers and contents in lib/ and src/', logs); | ||
}); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
<script type="text/javascript" src="./main.js"></script> | ||
</body> | ||
</html> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
/* | ||
* strict-d3: wrap selection.style to prohibit specific incorrect style values | ||
* that are known to cause problems in IE (at least IE9) | ||
*/ | ||
|
||
/* global Plotly:false */ | ||
(function() { | ||
'use strict'; | ||
|
||
var selProto = Plotly.d3.selection.prototype; | ||
|
||
var originalSelStyle = selProto.style; | ||
|
||
selProto.style = function() { | ||
var sel = this, | ||
obj = arguments[0]; | ||
|
||
if(sel.size()) { | ||
if(typeof obj === 'string') { | ||
checkVal(obj, arguments[1]); | ||
} | ||
else { | ||
Object.keys(obj).forEach(function(key) { checkVal(key, obj[key]); }); | ||
} | ||
} | ||
|
||
return originalSelStyle.apply(sel, arguments); | ||
}; | ||
|
||
function checkVal(key, val) { | ||
if(typeof val === 'string') { | ||
// in case of multipart styles (stroke-dasharray, margins, etc) | ||
// test each part separately | ||
val.split(/[, ]/g).forEach(function(valPart) { | ||
var pxSplit = valPart.length - 2; | ||
if(valPart.substr(pxSplit) === 'px' && !isNumeric(valPart.substr(0, pxSplit))) { | ||
throw new Error('d3 selection.style called with value: ' + val); | ||
} | ||
}); | ||
} | ||
|
||
} | ||
|
||
// below ripped from fast-isnumeric so I don't need to build this file | ||
|
||
function allBlankCharCodes(str) { | ||
var l = str.length, | ||
a; | ||
for(var i = 0; i < l; i++) { | ||
a = str.charCodeAt(i); | ||
if((a < 9 || a > 13) && (a !== 32) && (a !== 133) && (a !== 160) && | ||
(a !== 5760) && (a !== 6158) && (a < 8192 || a > 8205) && | ||
(a !== 8232) && (a !== 8233) && (a !== 8239) && (a !== 8287) && | ||
(a !== 8288) && (a !== 12288) && (a !== 65279)) { | ||
return false; | ||
} | ||
} | ||
return true; | ||
} | ||
|
||
function isNumeric(n) { | ||
var type = typeof n; | ||
if(type === 'string') { | ||
var original = n; | ||
n = +n; | ||
// whitespace strings cast to zero - filter them out | ||
if(n === 0 && allBlankCharCodes(original)) return false; | ||
} | ||
else if(type !== 'number') return false; | ||
|
||
return n - n < 1; | ||
} | ||
|
||
})(); |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe
There was a problem hiding this comment.
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
orundefinedpx
errors, at least as long as they go through d3.Will investigate the other two tomorrow, they may be related.