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

Respect lang attribute on player and inherit correctly #3426

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@mister-ben
Contributor

mister-ben commented Jul 11, 2016

Description

Currently, if language isn't set in the player options language will be determined by the lang attribute of the html element if set before falling back to the browser language preference.

lang should be respected if present on the video element. If not present, lang should be inherited from the closest element with it set up to and including html.

Specific Changes proposed

Checks for closest element with a lang attribute, not just html. The check is made in constructor rather than options prototype, as it needs to be relative to the specific player instance. A language in the player options still takes precedence.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Unit Tests updated or fixed
    • Docs/guides updated
  • Reviewed by Two Core Contributors
@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Jul 18, 2016

Member

LGTM.

Member

gkatsev commented Jul 18, 2016

LGTM.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Jul 20, 2016

Member

@mister-ben hey, looks like some of the tests are failing in IE, would you be able to take a look? See https://travis-ci.org/videojs/video.js/builds/145628625 (ignore the flash duration and logging tests)

Member

gkatsev commented Jul 20, 2016

@mister-ben hey, looks like some of the tests are failing in IE, would you be able to take a look? See https://travis-ci.org/videojs/video.js/builds/145628625 (ignore the flash duration and logging tests)

@mister-ben

This comment has been minimized.

Show comment
Hide comment
@mister-ben

mister-ben Jul 20, 2016

Contributor

Ack. Will check next week when I'm back from holiday.

Contributor

mister-ben commented Jul 20, 2016

Ack. Will check next week when I'm back from holiday.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Jul 20, 2016

Member

@mister-ben cool, I might end up fixing it myself. Would like to get a release out this week, if possible. I'll make sure to write here whether I managed to fix it or not.

Member

gkatsev commented Jul 20, 2016

@mister-ben cool, I might end up fixing it myself. Would like to get a release out this week, if possible. I'll make sure to write here whether I managed to fix it or not.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Jul 22, 2016

Member

Figured it out. A pretty silly issue that's super hard to find: #3455

Member

gkatsev commented Jul 22, 2016

Figured it out. A pretty silly issue that's super hard to find: #3455

}
} else {
let element = tag;
while (element && element.nodeType === 1) {

This comment has been minimized.

@OwenEdwards

OwenEdwards Jul 22, 2016

Member

Suggest you use ... && element.nodeType === Node.ELEMENT_NODE here for clarity (see https://developer.mozilla.org/en-US/docs/Web/API/Node/nodeType).

@OwenEdwards

OwenEdwards Jul 22, 2016

Member

Suggest you use ... && element.nodeType === Node.ELEMENT_NODE here for clarity (see https://developer.mozilla.org/en-US/docs/Web/API/Node/nodeType).

@mister-ben

This comment has been minimized.

Show comment
Hide comment
@mister-ben

mister-ben Jul 25, 2016

Contributor

@gkatsev sorry about that. Thanks for sorting it out.

Contributor

mister-ben commented Jul 25, 2016

@gkatsev sorry about that. Thanks for sorting it out.

@mister-ben mister-ben deleted the mister-ben:lang-attr branch Aug 10, 2016

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