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

SeekHandle content #902

Merged
merged 2 commits into from Feb 12, 2014

Conversation

Projects
None yet
3 participants
@jberns88
Contributor

jberns88 commented Dec 17, 2013

I wanted to use the SeekHandle to show the current time but as far as I could tell it only contained static content, I made this change to allow the content to be updated.

Is this its intended purpose or am I playing with something that has a different job?

jberns88 added some commits Dec 17, 2013

SeekHandle content
I wanted to use the SeekHandle to show the current time but as far as I could tell it only contained static content, I made this change to allow the content to be updated.

Is this its intended purpose or am I playing with something that has another a different job?
@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Feb 11, 2014

Member

Sorry for the slow response to this one. The seek handle used to support this and wasn't updated after the latest big rewrite. So the 00:00 is still there but it does need this update. It won't be shown in the default skin, but the hope is to make it easy for anyone to make a skin that shows the time above the seek handle, if that's what's desired.

I'll need to play around with this for a bit to make sure it doesn't conflict, and there were some changes that updated time cacheing.

Member

heff commented Feb 11, 2014

Sorry for the slow response to this one. The seek handle used to support this and wasn't updated after the latest big rewrite. So the 00:00 is still there but it does need this update. It won't be shown in the default skin, but the hope is to make it easy for anyone to make a skin that shows the time above the seek handle, if that's what's desired.

I'll need to play around with this for a bit to make sure it doesn't conflict, and there were some changes that updated time cacheing.

className: 'vjs-seek-handle'
});
vjs.SeekHandle.prototype.createEl = function() {
return this.content = vjs.SliderHandle.prototype.createEl.call(this, 'div', {

This comment has been minimized.

@heff

heff Feb 11, 2014

Member

I think you could use this.el() in the same way you're using this.content here. Let me know if there was a specific reason for creating a new property in addition to el().

@heff

heff Feb 11, 2014

Member

I think you could use this.el() in the same way you're using this.content here. Let me know if there was a specific reason for creating a new property in addition to el().

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Feb 11, 2014

Member

@gdkraus, after this update the slider handle's content will update with the current time. Do you know if it should it have an 'aria-live': 'off' attribute like the current time display? Seems like it should since its parent element (the slider) has the aria-valuetext attribute and the slider is the one in focus when using keyboard navigation, not the handle.

Member

heff commented Feb 11, 2014

@gdkraus, after this update the slider handle's content will update with the current time. Do you know if it should it have an 'aria-live': 'off' attribute like the current time display? Seems like it should since its parent element (the slider) has the aria-valuetext attribute and the slider is the one in focus when using keyboard navigation, not the handle.

@heff heff merged commit ec1bea4 into videojs:master Feb 12, 2014

1 check passed

default The Travis CI build passed
Details
@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Feb 12, 2014

Member

Thanks for the addition @MrAvenger!

Member

heff commented Feb 12, 2014

Thanks for the addition @MrAvenger!

@gdkraus

This comment has been minimized.

Show comment
Hide comment
@gdkraus

gdkraus Feb 14, 2014

@heff, I don't remember dealing with the text field updating when I made the accessibility changes about a year ago. Is that a new field?

Nonetheless, it should be set to aria-live="off" because the <div class="vjs-progress-holder vjs-slider" role="slider" aria-valuenow="55.91" aria-valuemin="0" aria-valuemax="100" tabindex="0" aria-label="video progress bar" aria-valuetext="0:26"> already has that information in a way that can be conveyed to screen reader users.

gdkraus commented Feb 14, 2014

@heff, I don't remember dealing with the text field updating when I made the accessibility changes about a year ago. Is that a new field?

Nonetheless, it should be set to aria-live="off" because the <div class="vjs-progress-holder vjs-slider" role="slider" aria-valuenow="55.91" aria-valuemin="0" aria-valuemax="100" tabindex="0" aria-label="video progress bar" aria-valuetext="0:26"> already has that information in a way that can be conveyed to screen reader users.

@heff

This comment has been minimized.

Show comment
Hide comment
Member

heff commented Feb 14, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment