feat: text displayer config overrides for text color/background color

[david-hm-morgan]

Following on and extending the style of config of text displayer introduced with #8242 .

FYI @matvp91

[matvp91]

Hi @david-hm-morgan,

Thank you for the PR!

This is initially how I had this in mind, but I started doubting adding defaults for several reasons. Text displayers are a visual representation of a shaka.text.Cue, theoretically if you'd influence a cue to your liking, there's no real use to alter styles at the text displayer level separately.

Using modifyCueCallback would allow you to do that. I noticed it wasn't called in src= mode and fixed it in #8254. The callback is a generic way to alter a cue, no matter where they come from (either from shaka.text.TextEngine or HTMLMediaElement.textTracks).

The idea to add defaulting when there's an explicit way to modify a cue (and thus its visual representation) doesn't sit well with me anymore. I like the idea to change its appearance unrelated to what text displayer is used.

Another thing is that config.textDisplayer is generic, meaning it should apply to all displayers. I see this getting filled up with all sorts of defaults future wise, some would apply to UiTextDisplayer, others would only serve purpose for SimpleTextDisplayer. I'm not too sure yet this is the right approach, thus that's why I initially didn't continue with a PR similar to yours.

First, I'd like to figure out if there's a case where we wouldn't want to alter the cue through modifyCueCallback and whether defaulting has a purpose.

Happy to discuss this further, let me know your thoughts.

[avelad]

Hi @david-hm-morgan,

Thank you for the PR!

This is initially how I had this in mind, but I started doubting adding defaults for several reasons. Text displayers are a visual representation of a shaka.text.Cue, theoretically if you'd influence a cue to your liking, there's no real use to alter styles at the text displayer level separately.

Using modifyCueCallback would allow you to do that. I noticed it wasn't called in src= mode and fixed it in #8254. The callback is a generic way to alter a cue, no matter where they come from (either from shaka.text.TextEngine or HTMLMediaElement.textTracks).

The idea to add defaulting when there's an explicit way to modify a cue (and thus its visual representation) doesn't sit well with me anymore. I like the idea to change its appearance unrelated to what text displayer is used.

Another thing is that config.textDisplayer is generic, meaning it should apply to all displayers. I see this getting filled up with all sorts of defaults future wise, some would apply to UiTextDisplayer, others would only serve purpose for SimpleTextDisplayer. I'm not too sure yet this is the right approach, thus that's why I initially didn't continue with a PR similar to yours.

First, I'd like to figure out if there's a case where we wouldn't want to alter the cue through modifyCueCallback and whether defaulting has a purpose.

Happy to discuss this further, let me know your thoughts.

I see an improvement over your solution, and that is that you can change this on the fly.

@david-hm-morgan Do you want to use this to change cues dynamically even while they are on screen?

[matvp91]

Right, I think it's fair to assume people want to change defaults on the fly (eg; a default style menu à la Prime).

Would it make sense to add textDisplayer.defaultStyles of type CSSStyleDeclaration instead of explicitly defining each possible default?

[shaka-bot]

Incremental code coverage: 100.00%

[david-hm-morgan]

yes @avelad the intention was to see instant change. Locally I have a little demo page that the user could tinker with the styles to get instant feedback.

and @matvp91 I am open to the CSSStyleDeclaration concept, especially since before too long I want to add edge/textShadow settings too.

[avelad]

@david-hm-morgan @matvp91 Then I'd prefer CSSStyleDeclaration instead this PR

[gkatsev]

This may be tricky enough to do that it may not be worth it, but it's worth considering having an option on whether this should or shouldn't override the settings of the captions files themselves. That is, if a caption file includes a setting for text color etc, should this option override cue setting or not? This is something that Apple provides natively on macs (see screenshot below). I think it would be nice to do if possible and a reasonable amount of work.

[matvp91]

@gkatsev wouldn't modifyCueCallback suffice if you'd want to force cue styles?

[gkatsev]

@matvp91 the key here is to choose what you want to override or not. I.e., I want to make sure that background color is set to my color in all cases, but everything else can be what the caption says.

[gkatsev]

And as I mention, it's mostly a "nice-to-have". Having the overrides is more important in the first place. The "allow video to override" type setting can come later.

Also, I mention it for shaka because due to how shaka parses cues, it may have an easier time around that setting compared to other player projects.

[matvp91]

I agree, it's reasonable to add something like:

player.configure({
  textDisplayer: {
    defaultStyles: /* @type CSSStyleDeclaration */ {},
    forcedStyles: /* @type CSSStyleDeclaration */ {},
  },
});

WDYT?

[matvp91]

Suggestion to drop a separate function for each style property. You can add a default styles map to player_configuration.js. This would make it a one liner:

style.color = cue.color || this.config_.textColor

We can maybe introduce a helper function later on to resolve the correct value by key name (as the same logic would apply for all style properties). Especially if we're going to distinguish default vs. forced.

[david-hm-morgan]

I agree, it's reasonable to add something like:

player.configure({
  textDisplayer: {
    defaultStyles: /* @type CSSStyleDeclaration */ {},
    forcedStyles: /* @type CSSStyleDeclaration */ {},
  },
});

WDYT?

@matvp91 FYI I've other tasks in hand currently, it might be a little while before I can circle back to this topic