Skip to content
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

Toimage bug #604

Merged
merged 2 commits into from
Jun 6, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/snapshot/svgtoimg.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ function svgToImg(opts) {
imgData = canvas.toDataURL('image/webp');
break;
case 'svg':
imgData = svg;
imgData = url;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The imgData must be an actual URL to work as a download via a link.

Copy link
Contributor

Choose a reason for hiding this comment

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

good to know!

break;
default:
reject(new Error('Image format is not jpeg, png or svg'));
Expand Down
23 changes: 7 additions & 16 deletions src/snapshot/tosvg.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,24 +78,12 @@ module.exports = function toSVG(gd, format) {
return;
}

// I've seen font-family styles with non-escaped double quotes in them - breaks the
// serialized svg because the style attribute itself is double-quoted!
// Is this an IE thing? Any other attributes or style elements that can have quotes in them?
// TODO: this looks like a noop right now - what happened to it?

/*
* Font-family styles with double quotes in them breaks the to-image
* step in FF42 because the style attribute itself is wrapped in
* double quotes. See:
*
* - http://codepen.io/etpinard/pen/bEdQWK
* - https://github.com/plotly/plotly.js/pull/104
*
* for more info.
*/
// Font family styles break things because of quotation marks,
// so we must remove them *after* the SVG DOM has been serialized
// to a string (browsers convert singles back)
var ff = txt.style('font-family');
if(ff && ff.indexOf('"') !== -1) {
txt.style('font-family', ff.replace(/"/g, '\\\''));
txt.style('font-family', ff.replace(/"/g, 'TOBESTRIPPED'));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can switch this to something even less likely to have collisions if we think it's necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ok with this. The most important part now is that all our supported image types are tested.

Down the road, we might want to put these plotly-specific constants in a separate well-documented file (along with e.g. Axed.BADNUM). But, there's no need to do this now.

👍

}
});

Expand All @@ -115,5 +103,8 @@ module.exports = function toSVG(gd, format) {
s = svgTextUtils.html_entity_decode(s);
s = svgTextUtils.xml_entity_encode(s);

// Fix quotations around font strings
s = s.replace(/("TOBESTRIPPED)|(TOBESTRIPPED")/g, '\'');

return s;
};
115 changes: 77 additions & 38 deletions test/jasmine/tests/download_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,45 +44,84 @@ describe('Plotly.downloadImage', function() {
});

it('should create link, remove link, accept options', function(done) {
//use MutationObserver to monitor the DOM
//for changes
//code modeled after
//https://developer.mozilla.org/en-US/docs/Web/API/MutationObserver
// select the target node
var target = document.body;
var domchanges = [];

// create an observer instance
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(mutation) {
domchanges.push(mutation);
});
});
downloadTest(gd, 'jpeg', done);
});

Plotly.plot(gd, textchartMock.data, textchartMock.layout).then(function(gd) {
// start observing dom
// configuration of the observer:
var config = { childList: true };

// pass in the target node and observer options
observer.observe(target, config);

return Plotly.downloadImage(gd, {format: 'jpeg', height: 300, width: 300, filename: 'plotly_download'});
}).then(function(filename) {
// stop observing
observer.disconnect();
// look for an added and removed link
var linkadded = domchanges[domchanges.length - 2].addedNodes[0].outerHTML;
var linkdeleted = domchanges[domchanges.length - 1].removedNodes[0].outerHTML;

// check for a <a element and proper file type
expect(linkadded.split('href="')[1].split('jpeg;')[0]).toEqual('data:image/');
// check that filename option handled properly
expect(filename).toBe('plotly_download.jpeg');

// check that link removed
expect(linkadded).toBe(linkdeleted);
done();
it('should create link, remove link, accept options', function(done) {
downloadTest(gd, 'png', done);
});

it('should create link, remove link, accept options', function(done) {
checkWebp(function(supported) {
if(supported) {
downloadTest(gd, 'webp', done);
} else {
done();
}
});

});

it('should create link, remove link, accept options', function(done) {
downloadTest(gd, 'svg', done);
});
});


function downloadTest(gd, format, done) {
//use MutationObserver to monitor the DOM
//for changes
//code modeled after
//https://developer.mozilla.org/en-US/docs/Web/API/MutationObserver
// select the target node
var target = document.body;
var domchanges = [];

// create an observer instance
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(mutation) {
domchanges.push(mutation);
});
});

Plotly.plot(gd, textchartMock.data, textchartMock.layout).then(function(gd) {
// start observing dom
// configuration of the observer:
var config = { childList: true };

// pass in the target node and observer options
observer.observe(target, config);

return Plotly.downloadImage(gd, {format: format, height: 300, width: 300, filename: 'plotly_download'});
}).then(function(filename) {
// stop observing
observer.disconnect();
// look for an added and removed link
var linkadded = domchanges[domchanges.length - 2].addedNodes[0];
var linkdeleted = domchanges[domchanges.length - 1].removedNodes[0];

// check for a <a element and proper file type
expect(linkadded.getAttribute('href').split(format)[0]).toEqual('data:image/');
// check that filename option handled properly
expect(filename).toEqual('plotly_download.' + format);

// check that link removed
expect(linkadded).toBe(linkdeleted);
done();
});
}


// Only chrome supports webp at the time of writing
function checkWebp(cb) {
var img = new Image();
img.onload = function() {
cb(true);
};

img.onerror = function() {
cb(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

smooth.

};

img.src = 'data:image/webp;base64,UklGRjoAAABXRUJQVlA4IC4AAACyAgCdASoCAAIALmk0mk0iIiIiIgBoSygABc6WWgAA/veff/0PP8bA//LwYAAA';
}
2 changes: 1 addition & 1 deletion test/jasmine/tests/toimage_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ describe('Plotly.toImage', function() {
// now do svg
return Plotly.toImage(gd, {format: 'svg'});
}).then(function(url) {
expect(url.substr(1, 3)).toBe('svg');
expect(url.split('svg')[0]).toBe('data:image/');
done();
});
});
Expand Down