Build HTML via DOM manipulation (vs string manipulation)

[ian-pvd]

Changes in this PR:

• Modifies the way the generateLegend() callback builds legend markup to utilize DOM node constructors instead of HTML strings.
• Updates unit tests to compare the expected HTML strings to the new DOM element method using outerHTML

Context:
These changes are in response to a request to use Chart.JS as part of a WordPress VIP theme, which necessitated a code review where three instances of HTML string constructors were flagged. These changes would bring the 2.8.0 version of Chart.JS into compliance with the WordPress VIP JavaScript coding standards, allowing it to be utilized on other VIP sites and themes.

[benmccann]

This seems like a good change, but isn't backwards compatible. It'd be better if we changed the html fragment construction as you're doing here, but then still returned a string and added a TODO to change the return type in v3

[simonbrunel]

Thanks @ian-pvd for your contribution. While I agree this way looks better, it's a breaking change that can't happen in v2. Furthermore in v3, I'm confident that will get rid of this HTML legend in favor of an external plugin to keep the core build as DOM independent as possible, thus the whole logic will be rewritten.

Since @benmccann suggestion will not help to fix your Wordpress VIP concerns, I'm going to close this PR.

[benmccann]

@simonbrunel I don't understand why my suggestion wouldn't fix the Wordpress VIP concern. I believe the concern there is using string concatenation to build HTML. But if we instead build it via createDocumentFragment and then convert to a string when returning that seems like it would address the issue

[simonbrunel]

... where three instances of HTML string constructors were flagged.

The current legendCallback implementation only manipulates strings, thus can't be the cause of any DOM warning / error. I guess the issue @ian-pvd is trying to workaround is the actual insertion of the legend HTML string inside his DOM tree (e.g. node.innerHTML = chart.generateLegend()).

[benmccann]

Ah, I wasn't sure because in the #general channel it referred to text.push. Maybe @ian-pvd can clarify?

[simonbrunel]

I may be wrong (I didn't see the Slack message) but I doubt WordPress VIP raises issues for using Array.prototype.push but instead for generating HTML from string (security issues similar to #5909). So I don't think it would change anything to generate a DOM fragment, convert it back to string and return it in generateLegend() because at the end it would still be needed to generate DOM elements from strings.

[ian-pvd]

Hi @simonbrunel & @benmccann , thanks for following up so quickly on this PR. To clarify, the specific issue that was flagged by VIP was the use of strings to construct HTML markup. The specific cases that were flagged were those lines inside the legendCallback functions (using text.push) that are adding markup as strings (to an array) before being concatenated. It's the use of HTML strings here, not the array method that was flagged.

I am trying to follow up with VIP after Ben's suggestion last night to see if we would be able to use createDocumentFragment to build the markup before returning it as a string. I haven't heard anything back yet but, I'm just curious @simonbrunel if this would be a compatible fix for the Chart.JS maintainers. So far, I have not heard anything from VIP about the use of node.innerHTML in their review.

[simonbrunel]

@ian-pvd is it possible to access the code review for your project so we can see the reported issues?

While reading the VIP guidelines, I found this: Inserting HTML directly into DOM with Javascript which is a blocker. That means you will not be able to use the generateLegend() API (with createDocumentFragment or not) since you will still need to call html() or innerHTML() anyway.

[ian-pvd]

HI @simonbrunel, unfortunately I'm unable to link directly to the VIP code review, however I can summarize the specific issues here:

/plugins/plugin.legend.js

• HTML string construction, use DOM node construction instead:

text.push('<li><span style="background-color:' + chart.data.datasets[i].backgroundColor + '"></span>');

/controllers/controller.polarArea.js
/controllers/controller.doughnut.js

• HTML string construction, use DOM node construction instead:

text.push('<ul class="' + chart.id + '-legend">');

        var data = chart.data;
        var datasets = data.datasets;
        var labels = data.labels;

        if (datasets.length) {
            for (var i = 0; i < datasets[0].data.length; ++i) {
                text.push('<li><span style="background-color:' + datasets[0].backgroundColor[i] + '"></span>');
                if (labels[i]) {
                    text.push(labels[i]);
                }
                text.push('</li>');
            }
        }

        text.push('</ul>');
        return text.join('');

Since this PR was closed, I've discussed @benmccann's suggestion of using DOM constructors to build the legend markup, but returning it as an HTML string to ensure backwards compatibility, and they have tentatively approved this suggestion.

I have made changes to my branch which can be previewed here:
https://github.com/ian-pvd/Chart.js/pull/1/files

Please let me know if this would be an acceptable resolution to the HTML string backwards compatibility issue.

[benmccann]

lgtm

[simonbrunel]

@ian-pvd Is this PR ready to be merged or is there anything else that needs to be done in order to get it accepted by the VIP code review? I'm fine to merge these changes even if it would solve your issue only as long as you don't need to call chart.generateLegend() in your code.

[ian-pvd]

Hi @simonbrunel

These changes should be sufficient to address the VIP concerns about using string construction and pending a quick re-review of the 2.8 => 2.9, changes Chart.js should be approved for use on WordPress VIP.

[ian-pvd]

@benmccann @nagix

I have made changes to the polar area and doughnut files, removing the || 0 value from the for loops and instead re-instating the conditional check for datasets values.

This is based on @nagix 's comments:

• Build HTML via DOM manipulation (vs string manipulation) #6179 (comment)
• Build HTML via DOM manipulation (vs string manipulation) #6179 (comment)

As for plugin.legend.js, there is a comment from @nagix to remove the || 0 condition, but I added it to this file specifically based on feedback from @benmccann here.

I apologize, that I haven't really been working with Charts.js the past week so I'm in a different headspace, but I just wanted some clarification around what changes, if any, should be made to plugin.legend.js

Again, sorry for being a little bit out of the loop on this, I'd like to help get this merged for 2.9 so let me know if there are additional changes.

[benmccann]

Sorry @ian-pvd. I'm not quite sure why I'd suggested that. Let's go ahead and remove it as suggested by @nagix. I just left you one other comment as well to help close this out. Thanks for your patience

[kurkle]

List should have the class even if it's empty.

[kurkle]

class is not set when datasets is empty. This line should be outside the if to mimic old behavior.

[etimberg]

agreed. if we could fix this during the rebase that'd be great

[kurkle]

Same here

[doubleqp]

Any idea when this issue will be solved + v2.9 will be released? Been trying to hack something together myself with different chart types per dataset but seems like this will solve my issue. Thanks a lot!

[benmccann]

@ian-pvd will you be able to respond to @kurkle's comments?

[benmccann]

@ian-pvd just a quick reminder about this outstanding PR. we'd love to get it merged if you're able to address the comments. Thanks!

[benmccann]

@ian-pvd will you be able to update this PR so we can get it merged?

[abbasahmed]

Looking forward to the release of 2.9, hoping this gets resolved soon!

[etimberg]

I think this is good to merge as soon as the last comment from @kurkle is addressed

[Darkecudoua]

A status on this?

[etimberg]

@Darkecudoua the last pending comment needs to be addressed