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

FontFamily default name mapping missing #250

Closed
mattjuggins opened this issue Dec 8, 2023 · 7 comments · Fixed by #251
Closed

FontFamily default name mapping missing #250

mattjuggins opened this issue Dec 8, 2023 · 7 comments · Fixed by #251
Labels

Comments

@mattjuggins
Copy link
Contributor

The 'default' fontFamilyType isn't mapped correctly, resulting in no fontFamily styling being applied to subtitle elements.

As far as I understand:

  • There are several FontFamilyTypes, specified in EBU-TT-D (Section 4.4.), including default.
  • html.js interprets tts:fontfamily and converts these family types to generic font families, or to appropriate named fonts.
  • default is not handled as a specific case in html.js so it ends back unmapped into the value for fontFamily to be applied to an element.
  • The fontFamily style is not applied because default is a reserved word in CSS and hence not a valid font family name.

In the IMSC specification, there is a mention that default should be interpreted as monospaceSerif. As duplicate font family names are cleared up later in the code I think it would be fair to change the monospaceSerif case to the following in `html.js:

if (attr[i] === "monospaceSerif" || attr[i] === "default") { ...

This also seems to be considered in styles.js..

If this all checks out I'm happy to make the change and raise a PR.

@palemieux
Copy link
Contributor

When specified, tts:fontFamily="default" is parsed as "monospaceSerif" per

function (str) {
.

The initial value of tts:fontFamily is also aliased to "monospaceSerif" since initial values, as specified in TTML, are subjected to the same parsing.

Do you have a test case that demonstrates the issue you are encoutering?

@mattjuggins
Copy link
Contributor Author

My test case was using something along the lines of tts:fontFamily="myCustomFont, Arial, Helvetica, default". So default was used as a fallback rather than a single entry.

With the above in mind, I think there might be a problem similar to #245
default doesn't get converted to monospaceSerif correctly with the space character at the start when the array is split. It could be solved using trim() again?

@palemieux palemieux added the bug label Dec 11, 2023
@palemieux
Copy link
Contributor

@nigelmegitt Do you recall why trim() at #246 was applied during HTML mapping instead of TTML parsing?

https://github.com/nigelmegitt/imscJS/blob/13c449b22d570d8ec722f6ff11934de2f5ef4fa9/src/main/js/html.js#L1403

instead of:

if (ffs[i].charAt(0) !== "'" && ffs[i].charAt(0) !== '"') {

@nigelmegitt
Copy link
Contributor

nigelmegitt commented Dec 11, 2023

Hi @palemieux yes I do. The reason is that it is where we have it in the BBC fork, and the reason we put it there is because we apply styling options when HTML mapping, so that they happen whenever we render an ISD, not just on document parsing. The specific example here is at https://github.com/bbc/imscJS/blob/4b194d20e590f255e4168f9d3ddfd28430b35590/src/main/js/html.js#L1470-L1472

                    if (context.options.fontFamily) {
                        attr = context.options.fontFamily.split(",");
                    }

Then we process the split attr. That way if the customisation options change then we only have to generate new HTML for the current ISD, and the syntax of the fontFamily option is the same as the syntax in the TTML document, which can include spaces.

@palemieux
Copy link
Contributor

@mattjuggins It sounds like adding a trim() at time of parsing should work.

@nigelmegitt
Copy link
Contributor

I agree, that should work.

@mattjuggins
Copy link
Contributor Author

Excellent. Thanks both. I've opened up #251 for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants