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

VTT Caption Positioning Fixes #3523

Merged
merged 5 commits into from Nov 8, 2019

Conversation

@jnatalzia
Copy link
Collaborator

jnatalzia commented Nov 7, 2019

This PR will...

Fix positioning in the following ways:

  • Ensure caption width only extends as far as the words, not the full caption container
  • Fix the position finding algorithm such that it correct calculates the best place for the caption (previously was selecting the worst)
  • Fix the position finding algorithm by ensuring it combines both horizontal and vertical axis to position in the true best position

Why is this Pull Request needed?

Using line attribute screws with all the above issues and surfaced quite a few bugs in our caption rendering

Are there any points in the code the reviewer needs to double check?

All of it - this took a lot of research and trial and error to get right. want to be sure i'm not missing something

Are there any Pull Requests open in other repos which need to be merged with this?

Nope

Addresses Issue(s):

JW8-10682

Checklist

  • Jenkins builds and unit tests are passing
  • I have reviewed the automated results
jnatalzia added 2 commits Nov 6, 2019
Do intersect math based on true width of captions
Update pct checker to be accurate
@jnatalzia jnatalzia requested a review from robwalch as a code owner Nov 7, 2019
@jnatalzia jnatalzia added this to the 8.11.8 milestone Nov 7, 2019
let finalPos = bestPosition || specifiedPosition;
// If we detect a best position based on one axis, oftentimes combining with the other axis
// results in a much better position.
// TODO: Make more performant by checking axis permutations above

This comment has been minimized.

Copy link
@jnatalzia

jnatalzia Nov 7, 2019

Author Collaborator

there is certainly a way to refactor this to be O(n) however the complexity mixed with the amount of time already sunk into this make me believe this option is the best way forward until we want to spend more time on this rendering

Remove margin specific calculations
@jwplayer-robot

This comment has been minimized.

Copy link

jwplayer-robot commented Nov 7, 2019

❗️ MULTI Build for commit 1d50fca did not complete (FAILURE).
🏗 jwplayer build FAILURE
🏗 jwplayer unit tests NOT_STARTED
🏗 jwplayer-commercial build NOT_STARTED
🏗 jwplayer-commercial unit tests NOT_STARTED
🥒.js Allure Report NOT_STARTED
🥒 Allure Report NOT_STARTED
🍆 Manual Tests
📺 Views

@jnatalzia jnatalzia force-pushed the bugfix/vtt_line_alignment branch from 1d50fca to 5f9ccd1 Nov 7, 2019
width: obj.width || width
};
return ret;
};

// Helper function for visualizing box position
// Helpful when debugging
function drawBox(b, container, color) { // eslint-disable-line no-unused-vars

This comment has been minimized.

Copy link
@jnatalzia

jnatalzia Nov 7, 2019

Author Collaborator

this is not used by the code so would get shipped with the player however the utility of this function in debugging these captions box positions is worth the extra couple kb of delivery imo

This comment has been minimized.

Copy link
@robwalch

robwalch Nov 8, 2019

Member

How do you execute it? Why does it need to be part of the player/this code, vs a snippet in your browser or a global method in one of our test pages?

This comment has been minimized.

Copy link
@jnatalzia

jnatalzia Nov 8, 2019

Author Collaborator

fair questions - you would have to call it yourself below. i suppose it's more confusing than anything leaving it in - i just found it extremely helpful. might add as a window global

src/js/polyfills/webvtt.js Outdated Show resolved Hide resolved
@jwplayer-robot

This comment has been minimized.

Copy link

jwplayer-robot commented Nov 7, 2019

⚠️ MULTI Build for commit 5f9ccd1 is unstable (FAILURE).
🏗 jwplayer build SUCCESS
🏗 jwplayer unit tests SUCCESS
🏗 jwplayer-commercial build SUCCESS
🏗 jwplayer-commercial unit tests SUCCESS
🥒.js Allure Report FAILURE
🥒 Allure Report UNSTABLE
🍆 Manual Tests
📺 Views

@@ -454,19 +455,22 @@ function CueStyleBox(window, cue) {
// https://w3c.github.io/webvtt/#ref-for-enumdef-positionalignsetting-1
// The cue.align property is settable and other browsers use it as the offset from which the cue.position
// value is applied.
let transform = '';

This comment has been minimized.

Copy link
@jnatalzia

jnatalzia Nov 7, 2019

Author Collaborator

transform ensures that we respect the cue alignment when determining position. now that these are inline-block elements the size is not known at time of instantiation. this new code adds a transform to properly place the cue based on alignment and then removes it when the stylebox is finally moved to prevent it from being applied twice (once during exact px calculations and then after when we go to display)

@jnatalzia jnatalzia force-pushed the bugfix/vtt_line_alignment branch from 77cbe1e to f472575 Nov 7, 2019
@jwplayer-robot

This comment has been minimized.

Copy link

jwplayer-robot commented Nov 7, 2019

❗️ MULTI Build for commit f472575 did not complete (FAILURE).
🏗 jwplayer build FAILURE
🏗 jwplayer unit tests NOT_STARTED
🏗 jwplayer-commercial build NOT_STARTED
🏗 jwplayer-commercial unit tests NOT_STARTED
🥒.js Allure Report NOT_STARTED
🥒 Allure Report NOT_STARTED
🍆 Manual Tests
📺 Views

@jnatalzia jnatalzia force-pushed the bugfix/vtt_line_alignment branch from f472575 to e7e512f Nov 7, 2019
@jwplayer-robot

This comment has been minimized.

Copy link

jwplayer-robot commented Nov 7, 2019

⚠️ MULTI Build for commit e7e512f is unstable (FAILURE).
🏗 jwplayer build SUCCESS
🏗 jwplayer unit tests SUCCESS
🏗 jwplayer-commercial build SUCCESS
🏗 jwplayer-commercial unit tests SUCCESS
🥒.js Allure Report FAILURE
🥒 Allure Report UNSTABLE
🍆 Manual Tests
📺 Views

@jwplayer-robot

This comment has been minimized.

Copy link

jwplayer-robot commented Nov 8, 2019

❗️ MULTI Build for commit a64ae3e did not complete (FAILURE).
🏗 jwplayer build FAILURE

@jnatalzia

$ eslint './src/js'

/var/lib/jenkins-slave/workspace/jw7-opensource/src/js/polyfills/webvtt.js
  705:17  error    'drawBox' is not defined        no-undef
  726:79  warning  Infix operators must be spaced  space-infix-ops
  728:9   error    'drawBox' is not defined        no-undef

/var/lib/jenkins-slave/workspace/jw7-opensource/src/js/view/controls/controls.js
  113:17  warning  Expected an assignment or function call and instead saw an expression  no-unused-expressions

✖ 4 problems (2 errors, 2 warnings)
  0 errors and 1 warning potentially fixable with the `--fix` option.
@jnatalzia jnatalzia force-pushed the bugfix/vtt_line_alignment branch from a64ae3e to 3a68a92 Nov 8, 2019
@jwplayer-robot

This comment has been minimized.

Copy link

jwplayer-robot commented Nov 8, 2019

❗️ MULTI Build for commit 3a68a92 did not complete (FAILURE).
🏗 jwplayer build SUCCESS
🏗 jwplayer unit tests SUCCESS
🏗 jwplayer-commercial build FAILURE

There's a lint issue in the commercial test changes:

/var/lib/jenkins-slave/workspace/jw7-commercial/test/assets/harness.js
   463:16  warning  Identifier 'console_log' is not in camel case  camelcase
  1147:9   error    Unexpected let declaration                     es5/no-block-scoping
  1148:9   error    Unexpected let declaration                     es5/no-block-scoping

/var/lib/jenkins-slave/workspace/jw7-commercial/test/assets/capture_console_log.js
  3:12  warning  Identifier 'console_log' is not in camel case  camelcase

✖ 4 problems (2 errors, 2 warnings)
@jnatalzia

This comment has been minimized.

Copy link
Collaborator Author

jnatalzia commented Nov 8, 2019

test this please

@robwalch

This comment has been minimized.

Copy link
Member

robwalch commented Nov 8, 2019

test this please

Click ❗️to go to the job and then "Rebuild"

@jwplayer-robot

This comment has been minimized.

Copy link

jwplayer-robot commented Nov 8, 2019

⚠️ MULTI Build for commit 3a68a92 is unstable (FAILURE).
🏗 jwplayer build SUCCESS
🏗 jwplayer unit tests SUCCESS
🏗 jwplayer-commercial build SUCCESS
🏗 jwplayer-commercial unit tests SUCCESS
🥒.js Allure Report FAILURE
🥒 Allure Report UNSTABLE
🍆 Manual Tests
📺 Views

@jnatalzia jnatalzia merged commit 9ada232 into master Nov 8, 2019
2 of 3 checks passed
2 of 3 checks passed
jw7-pr-multi-opensource Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@jnatalzia jnatalzia deleted the bugfix/vtt_line_alignment branch Nov 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.