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

Fix/remove innerhtml #7337

Merged
merged 8 commits into from
Jul 28, 2021
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
10 changes: 8 additions & 2 deletions src/js/button.js
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
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
23 changes: 11 additions & 12 deletions src/js/control-bar/audio-track-controls/audio-track-menu-item.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
*/
import MenuItem from '../../menu/menu-item.js';
import Component from '../../component.js';
import {assign} from '../../utils/obj';

/**
* An {@link AudioTrack} {@link MenuItem}
Expand Down Expand Up @@ -46,21 +45,21 @@ 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);
const parentSpan = el.querySelector('.vjs-menu-item-text');

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

innerHTML += '</span>';

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

return el;
}

Expand Down
10 changes: 8 additions & 2 deletions src/js/control-bar/live-display.js
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
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
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
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
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
26 changes: 14 additions & 12 deletions src/js/control-bar/text-track-controls/subs-caps-menu-item.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
*/
import TextTrackMenuItem from './text-track-menu-item.js';
import Component from '../../component.js';
import {assign} from '../../utils/obj';
import {createEl} from '../../utils/dom.js';

/**
* SubsCapsMenuItem has an [cc] icon to distinguish captions from subtitles
Expand All @@ -14,21 +14,23 @@ 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);
const parentSpan = el.querySelector('.vjs-menu-item-text');

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

innerHTML += '</span>';

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

return el;
}
}
Expand Down
11 changes: 9 additions & 2 deletions src/js/control-bar/time-controls/time-display.js
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
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
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
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
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
12 changes: 10 additions & 2 deletions src/js/menu/menu-item.js
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,18 @@ 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);

// swap icon with menu item text.
el.replaceChild(createEl('span', {
className: 'vjs-menu-item-text',
textContent: this.localize(this.options_.label)
}), el.querySelector('.vjs-icon-placeholder'));

return el;
}

/**
Expand Down