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.

Sign up for GitHub

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

Fix/remove innerhtml #7337

Merged
merged 8 commits into from
Jul 28, 2021
Merged

Fix/remove innerhtml #7337

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c0fbfbf
fix: remove uses of innerHTML to prevent xss
brandonocasey Jul 22, 2021
8cebce9
slight refactor and fixes looking at diff
brandonocasey Jul 22, 2021
cf4b38e
fix role presentation
brandonocasey Jul 22, 2021
48267e1
code review comments
brandonocasey Jul 23, 2021
7931e9d
forgot ;
brandonocasey Jul 23, 2021
d06bc7f
correct spacer textContent
brandonocasey Jul 23, 2021
dac0cdd
remove children like innerHTML
brandonocasey Jul 23, 2021
e85c076
do less work
brandonocasey Jul 27, 2021
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
10 changes: 8 additions & 2 deletions src/js/button.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import Component from './component';
import log from './utils/log.js';
import {assign} from './utils/obj';
import keycode from 'keycode';
import {createEl} from './utils/dom.js';

/**
* Base class for all buttons.
Expand Down Expand Up @@ -34,7 +35,6 @@ class Button extends ClickableComponent {
tag = 'button';

props = assign({
innerHTML: '<span aria-hidden="true" class="vjs-icon-placeholder"></span>',
className: this.buildCSSClass()
}, props);

Expand All @@ -45,7 +45,13 @@ class Button extends ClickableComponent {
type: 'button'
}, attributes);

const el = Component.prototype.createEl.call(this, tag, props, attributes);
const el = createEl(tag, props, attributes);

el.appendChild(createEl('span', {
className: 'vjs-icon-placeholder'
}, {
'aria-hidden': true
}));

this.createControlTextEl(el);

Expand Down
9 changes: 7 additions & 2 deletions src/js/clickable-component.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ class ClickableComponent extends Component {
*/
createEl(tag = 'div', props = {}, attributes = {}) {
props = assign({
innerHTML: '<span aria-hidden="true" class="vjs-icon-placeholder"></span>',
className: this.buildCSSClass(),
tabIndex: 0
}, props);
Expand All @@ -73,7 +72,13 @@ class ClickableComponent extends Component {

this.tabIndex_ = props.tabIndex;

const el = super.createEl(tag, props, attributes);
const el = Dom.createEl(tag, props, attributes);

el.appendChild(Dom.createEl('span', {
className: 'vjs-icon-placeholder'
}, {
'aria-hidden': true
}));

this.createControlTextEl(el);

Expand Down
33 changes: 22 additions & 11 deletions src/js/control-bar/audio-track-controls/audio-track-menu-item.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
*/
import MenuItem from '../../menu/menu-item.js';
import Component from '../../component.js';
import {assign} from '../../utils/obj';
import document from 'global/document';

/**
* An {@link AudioTrack} {@link MenuItem}
Expand Down Expand Up @@ -46,20 +46,31 @@ class AudioTrackMenuItem extends MenuItem {
}

createEl(type, props, attrs) {
let innerHTML = `<span class="vjs-menu-item-text">${this.localize(this.options_.label)}`;
const el = super.createEl(type, props, attrs);

if (this.options_.track.kind === 'main-desc') {
innerHTML += `
<span aria-hidden="true" class="vjs-icon-placeholder"></span>
<span class="vjs-control-text"> ${this.localize('Descriptions')}</span>
`;
while (el.firstChild) {
el.removeChild(el.firstChild);
}

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

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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');

className: 'vjs-menu-item-text'
});

parentSpan.appendChild(document.createTextNode(this.localize(this.options_.label)));

if (this.options_.track.kind === 'main-desc') {
parentSpan.appendChild(super.createEl('span', {
className: 'vjs-icon-placeholder'
}, {
'aria-hidden': true
}));
parentSpan.appendChild(super.createEl('span', {
className: 'vjs-control-text',
textContent: this.localize('Descriptions')
}));
}

const el = super.createEl(type, assign({
innerHTML
}, props), attrs);
el.appendChild(parentSpan);

return el;
}
Expand Down
10 changes: 8 additions & 2 deletions src/js/control-bar/live-display.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
*/
import Component from '../component';
import * as Dom from '../utils/dom.js';
import document from 'global/document';

// TODO - Future make it click to snap to live

Expand Down Expand Up @@ -41,12 +42,17 @@ class LiveDisplay extends Component {
});

this.contentEl_ = Dom.createEl('div', {
className: 'vjs-live-display',
innerHTML: `<span class="vjs-control-text">${this.localize('Stream Type')}\u00a0</span>${this.localize('LIVE')}`
className: 'vjs-live-display'
}, {
'aria-live': 'off'
});

this.contentEl_.appendChild(Dom.createEl('span', {
className: 'vjs-control-text',
textContent: `${this.localize('Stream Type')}\u00a0`
}));
this.contentEl_.appendChild(document.createTextNode(this.localize('LIVE')));

el.appendChild(this.contentEl_);
return el;
}
Expand Down
4 changes: 2 additions & 2 deletions src/js/control-bar/playback-rate-menu/playback-rate-menu-button.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ class PlaybackRateMenuButton extends MenuButton {
this.labelEl_ = Dom.createEl('div', {
className: 'vjs-playback-rate-value',
id: this.labelElId_,
innerHTML: '1x'
textContent: '1x'
});

el.appendChild(this.labelEl_);
Expand Down Expand Up @@ -190,7 +190,7 @@ class PlaybackRateMenuButton extends MenuButton {
*/
updateLabel(event) {
if (this.playbackRateSupported()) {
this.labelEl_.innerHTML = this.player().playbackRate() + 'x';
this.labelEl_.textContent = this.player().playbackRate() + 'x';
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/js/control-bar/seek-to-live.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ class SeekToLive extends Button {

this.textEl_ = Dom.createEl('span', {
className: 'vjs-seek-to-live-text',
innerHTML: this.localize('LIVE')
textContent: this.localize('LIVE')
}, {
'aria-hidden': 'true'
});
Expand Down
12 changes: 5 additions & 7 deletions src/js/control-bar/spacer-controls/custom-control-spacer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,12 @@ class CustomControlSpacer extends Spacer {
* The element that was created.
*/
createEl() {
const el = super.createEl({
className: this.buildCSSClass()
return super.createEl('div', {
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'
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

});

// No-flex/table-cell mode requires there be some content
// in the cell to fill the remaining space of the table.
el.innerHTML = '\u00a0';
return el;
}
}

Expand Down
10 changes: 6 additions & 4 deletions src/js/control-bar/spacer-controls/spacer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,12 @@ class Spacer extends Component {
* @return {Element}
* The element that was created.
*/
createEl() {
return super.createEl('div', {
className: this.buildCSSClass()
});
createEl(tag = 'div', props = {}, attributes = {}) {
if (!props.className) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

pass props along

props.className = this.buildCSSClass();
}

return super.createEl(tag, props, attributes);
}
}

Expand Down
36 changes: 25 additions & 11 deletions src/js/control-bar/text-track-controls/subs-caps-menu-item.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
*/
import TextTrackMenuItem from './text-track-menu-item.js';
import Component from '../../component.js';
import {assign} from '../../utils/obj';
import document from 'global/document';
import {createEl} from '../../utils/dom.js';

/**
* SubsCapsMenuItem has an [cc] icon to distinguish captions from subtitles
Expand All @@ -14,20 +15,33 @@ import {assign} from '../../utils/obj';
class SubsCapsMenuItem extends TextTrackMenuItem {

createEl(type, props, attrs) {
let innerHTML = `<span class="vjs-menu-item-text">${this.localize(this.options_.label)}`;
const el = super.createEl(type, props, attrs);

if (this.options_.track.kind === 'captions') {
innerHTML += `
<span aria-hidden="true" class="vjs-icon-placeholder"></span>
<span class="vjs-control-text"> ${this.localize('Captions')}</span>
`;
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)));
Copy link
Member

Choose a reason for hiding this comment

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

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.


if (this.options_.track.kind === 'captions') {
parentSpan.appendChild(createEl('span', {
className: 'vjs-icon-placeholder'
}, {
'aria-hidden': true
}));
parentSpan.appendChild(createEl('span', {
className: 'vjs-control-text',
// space added as the text will visually flow with the
// label
textContent: ` ${this.localize('Captions')}`
}));
}

const el = super.createEl(type, assign({
innerHTML
}, props), attrs);
el.appendChild(parentSpan);

return el;
}
Expand Down
11 changes: 9 additions & 2 deletions src/js/control-bar/time-controls/time-display.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,16 @@ class TimeDisplay extends Component {
createEl() {
const className = this.buildCSSClass();
const el = super.createEl('div', {
className: `${className} vjs-time-control vjs-control`,
innerHTML: `<span class="vjs-control-text" role="presentation">${this.localize(this.labelText_)}\u00a0</span>`
className: `${className} vjs-time-control vjs-control`
});
const span = Dom.createEl('span', {
className: 'vjs-control-text',
textContent: `${this.localize(this.labelText_)}\u00a0`
}, {
role: 'presentation'
});

el.appendChild(span);

this.contentEl_ = Dom.createEl('span', {
className: `${className}-display`
Expand Down
15 changes: 12 additions & 3 deletions src/js/control-bar/time-controls/time-divider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,24 @@ 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>'
Copy link
Member

Choose a reason for hiding this comment

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

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

const el = super.createEl('div', {
className: 'vjs-time-control vjs-time-divider'
}, {
// 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', {
textContent: '/'
});

div.appendChild(span);
el.appendChild(div);

return el;
}

}
Expand Down
11 changes: 8 additions & 3 deletions src/js/control-bar/volume-control/volume-level.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,15 @@ class VolumeLevel extends Component {
* The element that was created.
*/
createEl() {
return super.createEl('div', {
className: 'vjs-volume-level',
innerHTML: '<span class="vjs-control-text"></span>'
const el = super.createEl('div', {
className: 'vjs-volume-level'
});

el.appendChild(super.createEl('span', {
className: 'vjs-control-text'
}));

return el;
}

}
Expand Down
2 changes: 1 addition & 1 deletion src/js/loading-spinner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ class LoadingSpinner extends Component {
const playerType = this.localize(isAudio ? 'Audio Player' : 'Video Player');
const controlText = dom.createEl('span', {
className: 'vjs-control-text',
innerHTML: this.localize('{1} is loading.', [playerType])
textContent: this.localize('{1} is loading.', [playerType])
});

const el = super.createEl('div', {
Expand Down
2 changes: 1 addition & 1 deletion src/js/menu/menu-button.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ class MenuButton extends Component {
if (this.options_.title) {
const titleEl = Dom.createEl('li', {
className: 'vjs-menu-title',
innerHTML: toTitleCase(this.options_.title),
textContent: toTitleCase(this.options_.title),
tabIndex: -1
});

Expand Down
15 changes: 13 additions & 2 deletions src/js/menu/menu-item.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import Component from '../component.js';
import {assign} from '../utils/obj';
import {MenuKeys} from './menu-keys.js';
import keycode from 'keycode';
import {createEl} from '../utils/dom.js';

/**
* The component for a menu item. `<li>`
Expand Down Expand Up @@ -63,11 +64,21 @@ class MenuItem extends ClickableComponent {
// The control is textual, not just an icon
this.nonIconControl = true;

return super.createEl('li', assign({
const el = super.createEl('li', assign({
className: 'vjs-menu-item',
innerHTML: `<span class="vjs-menu-item-text">${this.localize(this.options_.label)}</span>`,
tabIndex: -1
}, props), attrs);

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

el.appendChild(createEl('span', {
className: 'vjs-menu-item-text',
textContent: this.localize(this.options_.label)
}));
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.


return el;
}

/**
Expand Down