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

Jump to bottom

Using hard-coded namespaces in TTML parser #2756

Closed
joeyparrish opened this issue Jul 27, 2020 · 5 comments
Closed

Using hard-coded namespaces in TTML parser #2756

joeyparrish opened this issue Jul 27, 2020 · 5 comments
Assignees
@michellezhuogg
Labels
component: captions/subtitles The issue involves captions or subtitles flag: good first issue This might be a relatively easy issue; good for new contributors status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Milestone
v3.1

Comments

@joeyparrish
Copy link
Member

Have you read the FAQ and checked for duplicate open issues?
Yes

What version of Shaka Player are you using?
master

Can you reproduce the issue with our latest release version?
Yes

Can you reproduce the issue with the latest code from master?
Yes

Are you using the demo app or your own custom app?
Demo

What browser and OS are you using?
ChromeOS stable 84

What are the manifest and license server URIs?
Not available (pointed out by a partner, confirmed by reading source)

What did you do?
The TTML parser uses hard-coded namespaces in some places. For example:

  • tts:extent
  • xml:space
  • xml:id
  • smpte:backgroundImage

This prevents someone from using different namespace names in their XML. For example, this would be typical:

<tt xmlns="http://www.w3.org/ns/ttml"
    xmlns:tts="http://www.w3.org/ns/ttml#styling">
  <head>
    <styling>
      <style xml:id="backgroundStyle"
          tts:fontFamily="proportionalSansSerif"
          tts:fontSize="1.5c"
          tts:textAlign="center"
          tts:origin="5% 70%"
          tts:extent="95% 28%"/>

But this should also be valid:

<tt xmlns="http://www.w3.org/ns/ttml"
    xmlns:s="http://www.w3.org/ns/ttml#styling">
  <head>
    <styling>
      <style xml:id="backgroundStyle"
          s:fontFamily="proportionalSansSerif"
          s:fontSize="1.5c"
          s:textAlign="center"
          s:origin="5% 70%"
          s:extent="95% 28%"/>

In some cases, we use getAttribute directly with a hard-coded namespace name in the attribute name:

extent = tt.getAttribute('tts:extent');

Instead, we should match by namespace URL, as we do in most other cases.

const ttsNs = TtmlTextParser.styleNs_;  // "http://www.w3.org/ns/ttml#styling"
extent = XmlUtils.getAttributeNS(tt, ttsNs, 'extent');

It's possible that we could write a compiler conformance rule or linter rule to ban the direct use of getAttribute() with a hard-coded namespace name.
@joeyparrish joeyparrish added type: bug Something isn't working correctly component: captions/subtitles The issue involves captions or subtitles labels Jul 27, 2020
@shaka-bot shaka-bot added this to the v3.1 milestone Jul 27, 2020
@ghost
Copy link

ghost commented Sep 11, 2020

Hi Joey,

Is this fix due to be backported to the 2.5.x branch as well?

Many thanks,

Alex

@joeyparrish
Copy link
Member Author

Yes, I believe the fix should and could be backported to v2.5.x and v3.0.x.

@avelad avelad mentioned this issue Sep 21, 2020
Merged
@michellezhuogg michellezhuogg self-assigned this Nov 3, 2020
@joeyparrish joeyparrish added the flag: good first issue This might be a relatively easy issue; good for new contributors label Nov 3, 2020
@michellezhuogg
Copy link
Contributor

Fixed with commit 7e4280f .

@joeyparrish joeyparrish reopened this Nov 4, 2020
@joeyparrish
Copy link
Member Author

There are still instances of hard-coded namespaces in the parser. For example, smpte:backgroundImage.

@michellezhuogg
Copy link
Contributor

Got it. Working on a fix.

joeyparrish pushed a commit that referenced this issue Nov 10, 2020
@TheModMaker @joeyparrish
fix(ttml): Fix another hard-coded namespace.
7958ea1 
This also changes the tests to fail if we use any hard-coded namespace
prefixes.

Fixes #2756

Backported to v2.5.x

Change-Id: I06cb7c2043cd7e1278f89a355e40d9d5d375d87d
joeyparrish pushed a commit that referenced this issue Nov 12, 2020
@TheModMaker @joeyparrish
fix(ttml): Fix another hard-coded namespace.
aa20f8c 
This also changes the tests to fail if we use any hard-coded namespace
prefixes.

Fixes #2756

Change-Id: I06cb7c2043cd7e1278f89a355e40d9d5d375d87d

Backported to v3.0.x

Change-Id: Ia48fc18722db672fcd4a1b0557ac80c66cebe78b
@shaka-project shaka-project locked and limited conversation to collaborators Jan 9, 2021
@shaka-bot shaka-bot added the status: archived Archived and locked; will not be updated label Apr 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@michellezhuogg michellezhuogg

Labels
component: captions/subtitles The issue involves captions or subtitles flag: good first issue This might be a relatively easy issue; good for new contributors status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Projects
None yet
Milestone
v3.1
Development

No branches or pull requests
3 participants
@joeyparrish @shaka-bot @michellezhuogg