Fix/remove innerhtml #7337

brandonocasey · 2021-07-22T21:08:09Z

Description

Remove our usage of innerHTML across the board.

TODO: text-track-settings-menu which requires more changes than all of the rest of the code together. I think that will need it's own pull request.

codecov · 2021-07-22T21:37:05Z

Codecov Report

Merging #7337 (e85c076) into main (b483a76) will increase coverage by 0.02%.
The diff coverage is 85.29%.

@@            Coverage Diff             @@
##             main    #7337      +/-   ##
==========================================
+ Coverage   79.59%   79.62%   +0.02%     
==========================================
  Files         115      115              
  Lines        7262     7277      +15     
  Branches     1745     1747       +2     
==========================================
+ Hits         5780     5794      +14     
- Misses       1482     1483       +1

Impacted Files	Coverage Δ
...-bar/audio-track-controls/audio-track-menu-item.js	`3.84% <0.00%> (ø)`
src/js/control-bar/seek-to-live.js	`92.85% <ø> (ø)`
src/js/loading-spinner.js	`100.00% <ø> (ø)`
src/js/menu/menu-button.js	`73.80% <ø> (ø)`
src/js/control-bar/spacer-controls/spacer.js	`85.71% <75.00%> (-14.29%)`	⬇️
src/js/button.js	`62.50% <100.00%> (+1.63%)`	⬆️
src/js/clickable-component.js	`89.70% <100.00%> (+0.15%)`	⬆️
src/js/control-bar/live-display.js	`100.00% <100.00%> (ø)`
...ar/playback-rate-menu/playback-rate-menu-button.js	`80.70% <100.00%> (ø)`
...ntrol-bar/spacer-controls/custom-control-spacer.js	`100.00% <100.00%> (ø)`
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b483a76...e85c076. Read the comment docs.

gkatsev · 2021-07-22T23:39:54Z

TODO: text-track-settings-menu which requires more changes than all of the rest of the code together. I think that will need it's own pull request.

also, that one doesn't use any user-generated content, so, there aren't any issues related to that.

gkatsev

Reviewed most of these, still have audio track and subs caps menu items to review.

gkatsev · 2021-07-22T23:48:06Z

src/js/control-bar/live-display.js

+      textContent: `${this.localize('Stream Type')}\u00a0`
    }, {
      'aria-live': 'off'
    });

+    this.contentEl_.appendChild(Dom.createEl('span', {
+      className: 'vjs-control-text'
+    }));


In here, we want the textContent Stream Type to be in the control text span. We should be careful to keep the same DOM structure before and after.

Suggested change

textContent: `${this.localize('Stream Type')}\u00a0`

}, {

'aria-live': 'off'

});

this.contentEl_.appendChild(Dom.createEl('span', {

className: 'vjs-control-text'

}));

}, {

'aria-live': 'off'

});

this.contentEl_.appendChild(Dom.createEl('span', {

className: 'vjs-control-text',

textContent: `${this.localize('Stream Type')}\u00a0`

}));

gkatsev · 2021-07-22T23:53:02Z

src/js/control-bar/spacer-controls/custom-control-spacer.js

+      className: this.buildCSSClass(),
+      // No-flex/table-cell mode requires there be some content
+      // in the cell to fill the remaining space of the table.
+      textContent: '\u00a0'


Looks like this isn't coming through here. Comparing videojs-preview to this branch's preview, I see   in videojs-preview but not here. We can probably switch to &nbsp here.

Ok I found the issue here, the Spacer class that this inherits from did not pass along props or attributes. After correcting that this works. textContent says that it recommends not using html escape codes and instead using hex/js character codes.

gkatsev · 2021-07-22T23:58:24Z

src/js/control-bar/time-controls/time-divider.js

@@ -18,15 +18,22 @@ class TimeDivider extends Component {
   *         The element that was created.
   */
  createEl() {
-    return super.createEl('div', {
-      className: 'vjs-time-control vjs-time-divider',
-      innerHTML: '<div><span>/</span></div>'


the / here went missing in the new span. It's easy to miss.

gkatsev · 2021-07-22T23:58:58Z

src/js/control-bar/time-controls/time-divider.js

    }, {
      // this element and its contents can be hidden from assistive techs since
      // it is made extraneous by the announcement of the control text
      // for the current time and duration displays
      'aria-hidden': true
    });
+
+    const div = super.createEl('div');
+    const span = super.createEl('span');


Add / textContent

Suggested change

const span = super.createEl('span');

const span = super.createEl('span', {

textContent: '/'

});

brandonocasey · 2021-07-23T16:47:11Z

src/js/control-bar/spacer-controls/spacer.js

-      className: this.buildCSSClass()
-    });
+  createEl(tag = 'div', props = {}, attributes = {}) {
+    if (!props.className) {


pass props along

gkatsev · 2021-07-27T16:57:06Z

src/js/control-bar/audio-track-controls/audio-track-menu-item.js

    }

-    innerHTML += '</span>';
+    const parentSpan = super.createEl('span', {


I think here we want to use Dom.createEl otherwise, we get a filled out li element and end up with the label inserted into the DOM twice.

Actually, looking at el, maybe what we need to do here is to get the span that gets created and append to it?
So, not removing stuff from el on line 51/52, then parentSpan = el.querySelector('.vjs-menu-item-text');

gkatsev · 2021-07-27T17:28:33Z

src/js/control-bar/text-track-controls/subs-caps-menu-item.js

+    while (el.firstChild) {
+      el.removeChild(el.firstChild);
    }

-    innerHTML += '</span>';
+    const parentSpan = createEl('span', {
+      className: 'vjs-menu-item-text'
+    });
+
+    parentSpan.appendChild(document.createTextNode(this.localize(this.options_.label)));


Seems like there we can do the same as for the audio track menu item. Instead of removing all children, we grab the span that's created and append to it, since it seems like it gets created as we want.

gkatsev · 2021-07-27T17:44:59Z

src/js/menu/menu-item.js

+    while (el.firstChild) {
+      el.removeChild(el.firstChild);
+    }
+
+    el.appendChild(createEl('span', {
+      className: 'vjs-menu-item-text',
+      textContent: this.localize(this.options_.label)
+    }));


to do less work, we potentially can replace the first child with this new span and potentially do less work

el.replaceChild(createEl('span', { className: 'vjs-menu-item-text', textContent: this.localize(this.options_.label) }, el.firstChild)

Based on the output of ClickableComponent and the expected output of MenuItem, that's the only change.

Alternatively, we leave as is, but we should create an issue for this to do a follow-up refactor where we look and see where we can reduce the amount of work we're doing when creating components.

brandonocasey · 2021-07-27T17:48:50Z

going to do some testing with your suggestions.

brandonocasey · 2021-07-28T16:10:13Z

OK @gkatsev I updated this pull request with element replacements where possible.

gkatsev · 2021-07-28T20:41:56Z

Thanks, seems good to me!

In #7337, a lot of code was updated to no longer user innerHTML, but we accidentally caused an issue with Audio Description (AD) tracks where the track title was included twice. Once before and once after the AD icon. This is because we were calling `super.createEl()` but MenuItem created a specific element and didn't just pass things the arguments along. Instead, we should use `Dom.createEl()` directly. Fixes #7556

In videojs#7337, a lot of code was updated to no longer user innerHTML, but we accidentally caused an issue with Audio Description (AD) tracks where the track title was included twice. Once before and once after the AD icon. This is because we were calling `super.createEl()` but MenuItem created a specific element and didn't just pass things the arguments along. Instead, we should use `Dom.createEl()` directly. Fixes videojs#7556

brandonocasey added 3 commits July 22, 2021 16:52

fix: remove uses of innerHTML to prevent xss

c0fbfbf

slight refactor and fixes looking at diff

8cebce9

fix role presentation

cf4b38e

gkatsev reviewed Jul 23, 2021

View reviewed changes

brandonocasey added 3 commits July 23, 2021 11:15

code review comments

48267e1

forgot ;

7931e9d

correct spacer textContent

d06bc7f

brandonocasey commented Jul 23, 2021

View reviewed changes

remove children like innerHTML

dac0cdd

gkatsev reviewed Jul 27, 2021

View reviewed changes

do less work

e85c076

gkatsev approved these changes Jul 28, 2021

View reviewed changes

gkatsev merged commit eb8f802 into main Jul 28, 2021

gkatsev deleted the fix/remove-innerhtml branch July 28, 2021 20:42

gkatsev mentioned this pull request Dec 6, 2021

Audio Described displayed next the AD icon #7556

Closed

gkatsev mentioned this pull request Dec 7, 2021

fix: regression with AD audio track menu items #7559

Merged

edirub pushed a commit to edirub/video.js that referenced this pull request Jun 8, 2023

refactor: remove most usage of innerHTML (videojs#7337)

9066512

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/remove innerhtml #7337

Fix/remove innerhtml #7337

brandonocasey commented Jul 22, 2021

codecov bot commented Jul 22, 2021 •

edited

Loading

gkatsev commented Jul 22, 2021

gkatsev left a comment

gkatsev Jul 22, 2021

gkatsev Jul 22, 2021

brandonocasey Jul 23, 2021

gkatsev Jul 22, 2021

gkatsev Jul 22, 2021

brandonocasey Jul 23, 2021

gkatsev Jul 27, 2021

gkatsev Jul 27, 2021

gkatsev Jul 27, 2021

gkatsev Jul 27, 2021

gkatsev Jul 27, 2021

brandonocasey commented Jul 27, 2021

brandonocasey commented Jul 28, 2021

gkatsev commented Jul 28, 2021

-    const span = super.createEl('span');
+    const span = super.createEl('span', {
+      textContent: '/'
+    });

Fix/remove innerhtml #7337

Fix/remove innerhtml #7337

Conversation

brandonocasey commented Jul 22, 2021

Description

codecov bot commented Jul 22, 2021 • edited Loading

Codecov Report

gkatsev commented Jul 22, 2021

gkatsev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonocasey commented Jul 27, 2021

brandonocasey commented Jul 28, 2021

gkatsev commented Jul 28, 2021

codecov bot commented Jul 22, 2021 •

edited

Loading