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

add fieldsets and labels to text-track-settings #3054

Closed
wants to merge 1 commit into from
Closed

add fieldsets and labels to text-track-settings #3054

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
20 changes: 19 additions & 1 deletion src/css/components/_captions-settings.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
color: $primary-foreground-color;
margin: 0 auto;
padding: 0.5em;
height: 15em;
height: 20em;
font-size: 12px;
width: 40em;
}
Expand Down Expand Up @@ -67,6 +67,24 @@
margin-right: 10px;
}

.vjs-caption-settings .hiddenReadable {
Copy link
Member

Choose a reason for hiding this comment

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

hiddenReadable doesn't seem to be used in this PR. Can it be dropped?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing. Was planning on using them for labels that could be read by screen readers but not displayed but haven't added any to the form.

position:absolute;
left: -10000px;
top: auto;
width: 1px;
height: 1px;
overflow: hidden;
}

.vjs-caption-settings fieldset {
margin-top: 1em;
margin-left: .5em;
}

.vjs-caption-settings h2 {
margin-bottom: 0;
margin-left: .5em;
}
.vjs-caption-settings input[type="button"] {
width: 40px;
height: 40px;
Expand Down
197 changes: 98 additions & 99 deletions src/js/tracks/text-track-settings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ class TextTrackSettings extends Component {
this.$('.vjs-bg-color > select').selectedIndex = 0;
this.$('.window-color > select').selectedIndex = 0;
this.$('.vjs-text-opacity > select').selectedIndex = 0;
this.$('.vjs-bg-opacity > select').selectedIndex = 0;
this.$('.vjs-bg-opacity > select').selectedIndex = 1;
this.$('.vjs-window-opacity > select').selectedIndex = 0;
this.$('.vjs-edge-style select').selectedIndex = 0;
this.$('.vjs-font-family select').selectedIndex = 0;
Expand Down Expand Up @@ -235,113 +235,112 @@ function setSelectedOption(target, value) {

function captionOptionsMenuTemplate() {
let template = `<div class="vjs-tracksettings">
<div class="vjs-tracksettings-colors">
<div class="vjs-fg-color vjs-tracksetting">
<label class="vjs-label">Foreground</label>
<select>
<option value="">---</option>
<option value="#FFF">White</option>
<option value="#000">Black</option>
<option value="#F00">Red</option>
<option value="#0F0">Green</option>
<option value="#00F">Blue</option>
<option value="#FF0">Yellow</option>
<option value="#F0F">Magenta</option>
<option value="#0FF">Cyan</option>
</select>
<span class="vjs-text-opacity vjs-opacity">
<select>
<option value="">---</option>
<option value="1">Opaque</option>
<option value="0.5">Semi-Opaque</option>
<div class="vjs-tracksettings-colors">
<fieldset>
<legend>Colors</legend>
<div class="vjs-fg-color vjs-tracksetting">
<label class="vjs-label" for="captions-foreground">Foreground</label>
<select id="captions-foreground">
Copy link
Member

Choose a reason for hiding this comment

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

all ids have to be unique which won't be the case if you load multiple players on a page.
To fix this, we probably want to do something like:

// at the top of the page import guid
import guid from '../util/guid.js';

// then above the `let template`
let captionsForegroundId = `cpations-foreground${guid()}`;

// and there:
`<label for=${captionsForegroundId}>Foreground</label>
<select id=${captionsForegroundId}>...</select>
`

Copy link
Member

Choose a reason for hiding this comment

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

@gkatsev looking at the guid() method, wouldn't a simpler approach be to use the player.tag.id (which is unique for each player instance) as part of the IDs in the Caption Player Dialog? It seems like the guid() is going to be unique for each element within the player, which is fine for how it's used within dom.js, but perhaps overkill here?

Otherwise, has there been any discussion about having a global player.uniqueId?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I answered my own question - each element has a this.id_, but captionOptionsMenuTemplate() is a static function, not an object method. So we would need to pass it a unique ID, at which point it makes little difference if it is this.id_, this.player_.id() or Guid.newGUID().

<option value="#FFF" selected>White</option>
<option value="#000">Black</option>
<option value="#F00">Red</option>
<option value="#0F0">Green</option>
<option value="#00F">Blue</option>
<option value="#FF0">Yellow</option>
<option value="#F0F">Magenta</option>
<option value="#0FF">Cyan</option>
</select>
</span>
</div> <!-- vjs-fg-color -->
<div class="vjs-bg-color vjs-tracksetting">
<label class="vjs-label">Background</label>
<select>
<option value="">---</option>
<option value="#FFF">White</option>
<option value="#000">Black</option>
<option value="#F00">Red</option>
<option value="#0F0">Green</option>
<option value="#00F">Blue</option>
<option value="#FF0">Yellow</option>
<option value="#F0F">Magenta</option>
<option value="#0FF">Cyan</option>
</select>
<span class="vjs-bg-opacity vjs-opacity">
<span class="vjs-text-opacity vjs-opacity">
<select>
<option value="1" selected>Opaque</option>
<option value="0.5">Semi-Opaque</option>
</select>
</span>
</div>
<div class="vjs-bg-color vjs-tracksetting">
<label class="vjs-label" for="captions-background">Background</label>
<select id="captions-background">
<option value="#000" selected>Black</option>
<option value="#FFF">White</option>
<option value="#F00">Red</option>
<option value="#0F0">Green</option>
<option value="#00F">Blue</option>
<option value="#FF0">Yellow</option>
<option value="#F0F">Magenta</option>
<option value="#0FF">Cyan</option>
</select>
<span class="vjs-bg-opacity vjs-opacity">
<select>
<option value="">---</option>
<option value="1">Opaque</option>
<option value="0.8" selected>80% Opaque</option>
Copy link
Member

Choose a reason for hiding this comment

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

is adding another bg opacity necessary? Would it be better to change the value for semi-transparent instead?
The FCC rules for this call for opaque, semi-transparent, and transparent options.
@OwenEdwards does WCAG or a11y have anything to say about these values?

Copy link
Member

Choose a reason for hiding this comment

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

@gkatsev this is an FCC requirement (FCC 12-9), not a WCAG requirement. Specifically, it calls for:

... at least four settings, opaque (100% opacity), semi-transparent (at 75% or 25% opacity), and transparent (0% opacity)

for background and for window, and three for font (hmm, actually transparent font is essentially what we need for description tracks! That gives me an idea!)

So yes, we need to add another semi-transparent. Whether we need to change the values (to 0.75 and 0.25) is something to discuss, too.

Copy link
Member

Choose a reason for hiding this comment

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

I'd vote to change the current semi-transparent from 50% to 75% and not add another option.

<option value="0.5">Semi-Transparent</option>
<option value="0">Transparent</option>
</select>
</span>
</div> <!-- vjs-bg-color -->
<div class="window-color vjs-tracksetting">
<label class="vjs-label">Window</label>
<select>
<option value="">---</option>
<option value="#FFF">White</option>
<option value="#000">Black</option>
<option value="#F00">Red</option>
<option value="#0F0">Green</option>
<option value="#00F">Blue</option>
<option value="#FF0">Yellow</option>
<option value="#F0F">Magenta</option>
<option value="#0FF">Cyan</option>
</select>
<span class="vjs-window-opacity vjs-opacity">
</span>
</div>
<div class="window-color vjs-tracksetting">
<label class="vjs-label" for="captions-window">Window</label>
<select id="captions-window">
<option value="#000" selected>Black</option>
<option value="#FFF">White</option>
<option value="#F00">Red</option>
<option value="#0F0">Green</option>
<option value="#00F">Blue</option>
<option value="#FF0">Yellow</option>
<option value="#F0F">Magenta</option>
<option value="#0FF">Cyan</option>
</select>
<span class="vjs-window-opacity vjs-opacity">
<select>
<option value="">---</option>
<option value="1">Opaque</option>
<option value="0" selected>Transparent</option>
<option value="0.5">Semi-Transparent</option>
<option value="0">Transparent</option>
<option value="1">Opaque</option>
</select>
</span>
</div> <!-- vjs-window-color -->
</div> <!-- vjs-tracksettings -->
<div class="vjs-tracksettings-font">
<div class="vjs-font-percent vjs-tracksetting">
<label class="vjs-label">Font Size</label>
<select>
<option value="0.50">50%</option>
<option value="0.75">75%</option>
<option value="1.00" selected>100%</option>
<option value="1.25">125%</option>
<option value="1.50">150%</option>
<option value="1.75">175%</option>
<option value="2.00">200%</option>
<option value="3.00">300%</option>
<option value="4.00">400%</option>
</select>
</div> <!-- vjs-font-percent -->
<div class="vjs-edge-style vjs-tracksetting">
<label class="vjs-label">Text Edge Style</label>
<select>
<option value="none">None</option>
<option value="raised">Raised</option>
<option value="depressed">Depressed</option>
<option value="uniform">Uniform</option>
<option value="dropshadow">Dropshadow</option>
</select>
</div> <!-- vjs-edge-style -->
<div class="vjs-font-family vjs-tracksetting">
<label class="vjs-label">Font Family</label>
<select>
<option value="">Default</option>
<option value="monospaceSerif">Monospace Serif</option>
<option value="proportionalSerif">Proportional Serif</option>
<option value="monospaceSansSerif">Monospace Sans-Serif</option>
<option value="proportionalSansSerif">Proportional Sans-Serif</option>
<option value="casual">Casual</option>
<option value="script">Script</option>
<option value="small-caps">Small Caps</option>
</select>
</div> <!-- vjs-font-family -->
</div>
</div>
</span>
</div>
</fieldset>
</div><!-- vjs-tracksettings -->
<div class="vjs-tracksettings-font">
<fieldset>
<legend>Fonts</legend>
<div class="vjs-font-percent vjs-tracksetting">
<label class="vjs-label" for="captions-font-size">Font Size</label>
<select id="captions-font-size">
<option value="0.50">50%</option>
<option value="0.75">75%</option>
<option value="1.00" selected>100%</option>
<option value="1.25">125%</option>
<option value="1.50">150%</option>
<option value="1.75">175%</option>
<option value="2.00">200%</option>
<option value="3.00">300%</option>
<option value="4.00">400%</option>
</select>
</div>
<div class="vjs-edge-style vjs-tracksetting">
<label class="vjs-label" for="captions-edge-style">Text Edge Style</label>
<select id="captions-edge-style">
<option value="none" selected>None</option>
<option value="raised">Raised</option>
<option value="depressed">Depressed</option>
<option value="uniform">Uniform</option>
<option value="dropshadow">Dropshadow</option>
</select>
</div>
<div class="vjs-font-family vjs-tracksetting">
<label class="vjs-label" for="captions-font-family">Font Family</label>
<select id="captions-font-family">
<option value="proportionalSansSerif" selected>Proportional Sans-Serif</option>
<option value="monospaceSansSerif">Monospace Sans-Serif</option>
<option value="proportionalSerif">Proportional Serif</option>
<option value="monospaceSerif">Monospace Serif</option>
<option value="casual">Casual</option>
<option value="script">Script</option>
<option value="small-caps">Small Caps</option>
</select>
</div>
</fieldset>
</div>
<div class="vjs-tracksettings-controls">
<button class="vjs-default-button">Defaults</button>
<button class="vjs-done-button">Done</button>
Expand Down
43 changes: 31 additions & 12 deletions test/unit/tracks/text-track-settings.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ test('should update settings', function() {
'windowOpacity': '1',
'edgeStyle': 'raised',
'fontFamily': 'monospaceSerif',
'color': '#FFF',
'color': '#F00',
'backgroundColor': '#FFF',
'windowColor': '#FFF',
'fontPercent': 1.25
Expand All @@ -35,14 +35,14 @@ test('should update settings', function() {
player.textTrackSettings.setValues(newSettings);
deepEqual(player.textTrackSettings.getValues(), newSettings, 'values are updated');

equal(player.$('.vjs-fg-color > select').selectedIndex, 1, 'fg-color is set to new value');
equal(player.$('.vjs-fg-color > select').selectedIndex, 2, 'fg-color is set to new value');
equal(player.$('.vjs-bg-color > select').selectedIndex, 1, 'bg-color is set to new value');
equal(player.$('.window-color > select').selectedIndex, 1, 'window-color is set to new value');
equal(player.$('.vjs-text-opacity > select').selectedIndex, 1, 'text-opacity is set to new value');
equal(player.$('.vjs-bg-opacity > select').selectedIndex, 1, 'bg-opacity is set to new value');
equal(player.$('.vjs-window-opacity > select').selectedIndex, 1, 'window-opacity is set to new value');
equal(player.$('.vjs-text-opacity > select').selectedIndex, 0, 'text-opacity is set to new value');
equal(player.$('.vjs-bg-opacity > select').selectedIndex, 0, 'bg-opacity is set to new value');
equal(player.$('.vjs-window-opacity > select').selectedIndex, 2, 'window-opacity is set to new value');
equal(player.$('.vjs-edge-style select').selectedIndex, 1, 'edge-style is set to new value');
equal(player.$('.vjs-font-family select').selectedIndex, 1, 'font-family is set to new value');
equal(player.$('.vjs-font-family select').selectedIndex, 3, 'font-family is set to new value');
equal(player.$('.vjs-font-percent select').selectedIndex, 3, 'font-percent is set to new value');

Events.trigger(player.$('.vjs-done-button'), 'click');
Expand All @@ -55,7 +55,16 @@ test('should restore default settings', function() {
var player = TestHelpers.makePlayer({
tracks: tracks,
persistTextTrackSettings: true
});
}),
defaultSettings = {
'backgroundColor': '#000',
'backgroundOpacity': '0.8',
'color': '#FFF',
'fontFamily': 'proportionalSansSerif',
'textOpacity': '1',
'windowColor': '#000',
'windowOpacity': '0'
};

player.$('.vjs-fg-color > select').selectedIndex = 1;
player.$('.vjs-bg-color > select').selectedIndex = 1;
Expand All @@ -71,14 +80,15 @@ test('should restore default settings', function() {
Events.trigger(player.$('.vjs-default-button'), 'click');
Events.trigger(player.$('.vjs-done-button'), 'click');

deepEqual(player.textTrackSettings.getValues(), {}, 'values are defaulted');
deepEqual(window.localStorage.getItem('vjs-text-track-settings'), null, 'values are saved');
deepEqual(player.textTrackSettings.getValues(), defaultSettings, 'values are defaulted');
// MikeA: need to figure out how to modify saveSettings to factor in defaults are no longer null
// deepEqual(window.localStorage.getItem('vjs-text-track-settings'), defaultSettings, 'values are saved');

equal(player.$('.vjs-fg-color > select').selectedIndex, 0, 'fg-color is set to default value');
equal(player.$('.vjs-bg-color > select').selectedIndex, 0, 'bg-color is set to default value');
equal(player.$('.window-color > select').selectedIndex, 0, 'window-color is set to default value');
equal(player.$('.vjs-text-opacity > select').selectedIndex, 0, 'text-opacity is set to default value');
equal(player.$('.vjs-bg-opacity > select').selectedIndex, 0, 'bg-opacity is set to default value');
equal(player.$('.vjs-bg-opacity > select').selectedIndex, 1, 'bg-opacity is set to default value');
equal(player.$('.vjs-window-opacity > select').selectedIndex, 0, 'window-opacity is set to default value');
equal(player.$('.vjs-edge-style select').selectedIndex, 0, 'edge-style is set to default value');
equal(player.$('.vjs-font-family select').selectedIndex, 0, 'font-family is set to default value');
Expand Down Expand Up @@ -220,7 +230,16 @@ test('should not restore saved settings', function() {
'backgroundColor': '#FFF',
'windowColor': '#FFF',
'fontPercent': 1.25
};
},
defaultSettings = {
'backgroundColor': '#000',
'backgroundOpacity': '0.8',
'color': '#FFF',
'fontFamily': 'proportionalSansSerif',
'textOpacity': '1',
'windowColor': '#000',
'windowOpacity': '0'
};

window.localStorage.setItem('vjs-text-track-settings', JSON.stringify(newSettings));

Expand All @@ -229,7 +248,7 @@ test('should not restore saved settings', function() {
persistTextTrackSettings: false
});

deepEqual(player.textTrackSettings.getValues(), {});
deepEqual(player.textTrackSettings.getValues(), defaultSettings);

player.dispose();
});