Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Apply styles to SVG text elements directly as allowed by strict CSPs#7256

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
martian111 wants to merge3 commits intoplotly:master
base:master
Choose a base branch
Loading
frommartian111:svg-text-utils-with-strict-csp
Open
Show file tree
Hide file tree
Changes fromall 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: 2 additions & 0 deletionsdraftlogs/7256_fix.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
- Remove inline styles in SVG text elements generated from pseudo-HTML configurations, which break with strict CSP setups [[#7256](https://github.com/plotly/plotly.js/pull/7256)],
with thanks to @martian111 for the contribution!
83 changes: 64 additions & 19 deletionssrc/lib/svg_text_utils.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -328,15 +328,15 @@ var TAG_STYLES = {
// would like to use baseline-shift for sub/sup but FF doesn't support it
// so we need to use dy along with the uber hacky shift-back-to
// baseline below
sup: 'font-size:70%',
sub: 'font-size:70%',
s: 'text-decoration:line-through',
u: 'text-decoration:underline',
b: 'font-weight:bold',
i: 'font-style:italic',
a: 'cursor:pointer',
span:'',
em: 'font-style:italic;font-weight:bold'
sup:{'font-size':'70%'},
sub:{'font-size':'70%'},
s:{'text-decoration':'line-through'},
u:{'text-decoration':'underline'},
b:{'font-weight': 'bold'},
i:{'font-style':'italic'},
a:{'cursor':'pointer'},
span:{},
em:{'font-style':'italic','font-weight':'bold'}
};

// baseline shifts for sub and sup
Expand DownExpand Up@@ -383,9 +383,6 @@ exports.BR_TAG_ALL = /<br(\s+.*)?>/gi;
* convention and will not make a popup if this string is empty.
* per the spec, cannot contain whitespace.
*
* Because we hack in other attributes with style (sub & sup), drop any trailing
* semicolon in user-supplied styles so we can consistently append the tag-dependent style
*
* These are for tag attributes; Chrome anyway will convert entities in
* attribute values, but not in attribute names
* you can test this by for example:
Expand All@@ -394,7 +391,7 @@ exports.BR_TAG_ALL = /<br(\s+.*)?>/gi;
* > p.innerHTML
* <- '<span styl&#x65;="font-color:red;">Hi</span>'
*/
var STYLEMATCH = /(^|[\s"'])style\s*=\s*("([^"]*);?"|'([^']*);?')/i;
var STYLEMATCH = /(^|[\s"'])style\s*=\s*("([^"]*)"|'([^']*)')/i;
var HREFMATCH = /(^|[\s"'])href\s*=\s*("([^"]*)"|'([^']*)')/i;
var TARGETMATCH = /(^|[\s"'])target\s*=\s*("([^"\s]*)"|'([^'\s]*)')/i;
var POPUPMATCH = /(^|[\s"'])popup\s*=\s*("([\w=,]*)"|'([\w=,]*)')/i;
Expand DownExpand Up@@ -495,7 +492,8 @@ var entityToUnicode = {
nbsp: ' ',
times: '×',
plusmn: '±',
deg: '°'
deg: '°',
quot: "'",
};

// NOTE: in general entities can contain uppercase too (so [a-zA-Z]) but all the
Expand DownExpand Up@@ -537,6 +535,50 @@ function fromCodePoint(code) {
);
}

var SPLIT_STYLES = /([^;]+;|$)|&(#\d+|#x[\da-fA-F]+|[a-z]+);/;

var ONE_STYLE = /^\s*([^:]+)\s*:\s*(.+?)\s*;?$/i;

function applyStyles(node, styles) {
var parts = styles.split(SPLIT_STYLES);
var filteredParts = [];
for(var i = 0; i < parts.length; i++) {
if(parts[i] && typeof parts[i] === "string" && parts[i].length > 0) {
filteredParts.push(parts[i]);
}
}
parts = filteredParts;

for(var i = 0; i < parts.length; i++) {
var parti = parts[i];

// Recombine parts that was split due to HTML entity's semicolon
var partToSearch = parti;
do {
var matchEntity = partToSearch.match(ENTITY_MATCH);
if(matchEntity) {
var entity = matchEntity[0];
partToSearch = parts[i+1];
if(parti.endsWith(entity) && partToSearch) {
// Matched HTML entity is at the end, and thus, need to
// combine with next part to complete the style (when it ends
// with a semicolon that is not part of a HTML entity)
parti += partToSearch;
i++;
}
} else {
partToSearch = undefined;
}
} while (partToSearch);

var match = parti.match(ONE_STYLE);
if(match) {
var decodedStyle = convertEntities(match[2]);
d3.select(node).style(match[1], decodedStyle);
}
}
}

/*
* buildSVGText: convert our pseudo-html into SVG tspan elements, and attach these
* to containerNode
Expand DownExpand Up@@ -613,8 +655,6 @@ function buildSVGText(containerNode, str) {
}
} else nodeType = 'tspan';

if(nodeSpec.style) nodeAttrs.style = nodeSpec.style;

var newNode = document.createElementNS(xmlnsNamespaces.svg, nodeType);

if(type === 'sup' || type === 'sub') {
Expand All@@ -633,6 +673,10 @@ function buildSVGText(containerNode, str) {
}

d3.select(newNode).attr(nodeAttrs);
if(nodeSpec.style) applyStyles(newNode, nodeSpec.style)
if(nodeSpec.tagStyle) {
d3.select(newNode).style(nodeSpec.tagStyle);
}

currentNode = nodeSpec.node = newNode;
nodeStack.push(nodeSpec);
Expand DownExpand Up@@ -693,10 +737,10 @@ function buildSVGText(containerNode, str) {
var css = getQuotedMatch(extra, STYLEMATCH);
if(css) {
css = css.replace(COLORMATCH, '$1 fill:');
if(tagStyle) css += ';' + tagStyle;
} else if(tagStyle) css = tagStyle;
}

if(css) nodeSpec.style = css;
if(tagStyle) nodeSpec.tagStyle = tagStyle;

if(tagType === 'a') {
hasLink = true;
Expand DownExpand Up@@ -770,7 +814,7 @@ exports.sanitizeHTML = function sanitizeHTML(str) {
var extra = match[4];

var css = getQuotedMatch(extra, STYLEMATCH);
var nodeAttrs =css ? {style: css} :{};
var nodeAttrs = {};

if(tagType === 'a') {
var href = getQuotedMatch(extra, HREFMATCH);
Expand All@@ -790,6 +834,7 @@ exports.sanitizeHTML = function sanitizeHTML(str) {
var newNode = document.createElement(tagType);
currentNode.appendChild(newNode);
d3.select(newNode).attr(nodeAttrs);
if(css) applyStyles(newNode, css);

currentNode = newNode;
nodeStack.push(newNode);
Expand Down
2 changes: 1 addition & 1 deletiontest/image/mocks/pseudo_html.json
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -3,7 +3,7 @@
{
"x": ["<b>b</b> <i>i</i>","line <em>1</em><br>line <b>2</b>"],
"y": ["sub<sub>1</sub>","sup<sup>2</sup>"],
"name":"<b>test</b> <i>pseudo</i>HTML<br><sup>3</sup>H<sub>2</sub>O is <em>heavy!</em><br>and <span style=\"color:#f00;font-family:'Times New Roman', Times, serif\">Fonts,</span><br>oh my?"
"name":"<b>test</b> <i>pseudo</i>HTML<br><sup>3</sup>H<sub>2</sub>O is <em>heavy!</em><br>and <span style=\"color:#f00;font-family:'Times', serif\">Fonts,</span><br>oh my?"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this change necessary? Would the former family list be handled differently by the new code?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

If I remember correctly (it's been a while now), it caused errors with the test automation when run by CircleCI (don't have exact details at the moment). I assumed it was an environment/setup issue, for example, font not installed. Removing Times New Roman helped get the test case to run and pass without errors. I believe the spirit of the test case is intact to show that the font style did get applied as expected.

Copy link
Collaborator

Choose a reason for hiding this comment

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

hmph, the CI env hasn't changed though, so if the same input that used to work now causes errors, doesn't that mean there's something different now about how we're handling these attributes?

Overall this is a fantastic change! Just want to make sure we aren't creating a subtle bug along the way.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks! Yes, it makes sense to double check this change. I can try to reproduce when I get a chance (maybe in the next week or two). I vaguely remember it causing the CircleCI tests to fail, but no errors when testing the fonts via the browser on my laptop. I can try to verify with a CodeSandbox.

}
],
"layout": {
Expand Down
6 changes: 3 additions & 3 deletionstest/jasmine/tests/hover_label_test.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -748,7 +748,7 @@ describe('hover info', function() {

it('provides exponents correctly for z data', function(done) {
function expFmt(val, exp) {
return val + '×10\u200b<tspan style="font-size:70%" dy="-0.6em">' +
return val + '×10\u200b<tspandy="-0.6em"style="font-size:70%;">' +
(exp < 0 ? MINUS_SIGN + -exp : exp) +
'</tspan><tspan dy="0.42em">\u200b</tspan>';
}
Expand DownExpand Up@@ -2071,7 +2071,7 @@ describe('hover info', function() {
expect(hoverTrace.y).toEqual(1);

assertHoverLabelContent({
nums: '<tspan style="font-weight:bold">$1.00</tspan>\nPV learning curve.txt',
nums: '<tspan style="font-weight:bold;">$1.00</tspan>\nPV learning curve.txt',
name: '',
axis: '0.388'
});
Expand DownExpand Up@@ -2120,7 +2120,7 @@ describe('hover info', function() {
expect(hoverTrace.y).toEqual(1);

assertHoverLabelContent({
nums: 'Cost ($/W​<tspan style="font-size:70%" dy="0.3em">P</tspan><tspan dy="-0.21em">​</tspan>):$1.00\nCumulative Production (GW):0.3880',
nums: 'Cost ($/W​<tspandy="0.3em"style="font-size:70%;">P</tspan><tspan dy="-0.21em">​</tspan>):$1.00\nCumulative Production (GW):0.3880',
name: '',
axis: '0.388'
});
Expand Down
2 changes: 1 addition & 1 deletiontest/jasmine/tests/icicle_test.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -635,7 +635,7 @@ describe('Test icicle hover:', function() {
exp: {
label: {
nums: 'Abel :: 6.00',
name: '<tspan style="font-weight:bold">N.B.</tspan>'
name: '<tspan style="font-weight:bold;">N.B.</tspan>'
},
ptData: {
curveNumber: 0,
Expand Down
2 changes: 1 addition & 1 deletiontest/jasmine/tests/sunburst_test.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -648,7 +648,7 @@ describe('Test sunburst hover:', function() {
exp: {
label: {
nums: 'Abel :: 6.00',
name: '<tspan style="font-weight:bold">N.B.</tspan>'
name: '<tspan style="font-weight:bold;">N.B.</tspan>'
},
ptData: {
curveNumber: 0,
Expand Down
Loading
[8]ページ先頭
©2009-2025 Movatter.jp