Add customisation options #2

nigelmegitt · 2021-01-05T16:02:44Z

This pull request does the following:

Adds a customisation options parameter to imscHtml.render()

This allows the following presentation characteristics to be modified by multiplying by a scale factor:

Size
Opacity of every region
Opacity of the background on span
Opacity of the text colour

It also allows the following to be overridden

textOutline
fontFamily

Finally it allows colour values to be mapped to different values for each of:

text color
span backgroundColor
p backgroundColor
div backgroundColor
body backgroundColor
region backgroundColor

It therefore implements all of the IMSC-specific adjustments listed in CTA-CEB35 (note that there are some possible adjustments omitted from that document, for example fontStyle for italic/oblique and textDecoration for underline and line-through).

jlks

👍 Looks good, I just have the one bit of feedback on the mutation of options.

jlks · 2021-01-11T14:28:07Z

src/main/js/html.js

@@ -114,6 +129,20 @@
        rootcontainer.style.right = 0;
        rootcontainer.style.zIndex = 0;

+        if (options) { 


Can this section of code move to below the definition of options inside context (setting context.options.colorAdjust) to avoid mutating the options object?

The use case is that a library user has the same options they want to pass into the function on each call and after the first call the colorAdjust properties are mutated and can't be read by imscUtils.parseColor.

Addressed in 9116902.

jlks · 2021-01-11T15:59:06Z

src/main/js/html.js

@@ -133,9 +148,24 @@
            bpd: null, /* block progression direction (lr, rl, tb) */
            ruby: null, /* is ruby present in a <p> */
            textEmphasis: null, /* is textEmphasis present in a <p> */
-            rubyReserve: null /* is rubyReserve applicable to a <p> */
+            rubyReserve: null, /* is rubyReserve applicable to a <p> */
+            options: options || {}


9116902 still appears to mutate the existing options. I think this should do it:

Suggested change

options: options || {}

options: {}

};

if (options) {

if (options.colorAdjust)

context.options.colorAdjust = preprocessColorMapOptions(options.colorAdjust);

var bgcColorElements = ['region', 'body', 'div', 'p', 'span'];

var propName;

for (var bgcei in bgcColorElements)

{

propName = bgcColorElements[bgcei] + backgroundColorAdjustSuffix;

if (options[propName])

context.options[propName] = preprocessColorMapOptions(options[propName]);

}

}

Sigh. That suggestion would trash all the other options settings that aren't colour adjustments, so that wouldn't work either. We need to work on a copy that isn't a reference. Or I could make the keys for the preprocessed colour maps a different value to the passed-in ones instead of the same value?

bdd8530 should do it, but I don't like it!

I think I've got a compromise. This will copy the initial *colorAdjust references but then overwrite them when preprocessColorMapOptions happens. It also removes the need to copy across the other properties added in bdd8530.

Suggested change

options: options || {}

options: Object.assign({}, options) || {},

};

I thought of that too @jlks but it breaks on build I think because the grunt script is set to refuse ES6-only features, which includes Object.assign :-(

Oh! I don't get any grunt:build errors. I see Object.entries() being used inside preprocessColorMapOptions does that need to change too? I see it gets added as a polyfill in the minified distribution just the same as using Object.assign.

Huh, okay, maybe I didn't actually try it, but just assumed it would fail. I'll make this fix (tomorrow) - it will definitely clean things up.

OK done in 13dc9a5

robertbryer · 2021-01-12T12:45:36Z

Should this not be merged into the branch smp-master and not master?

robertbryer · 2021-01-12T12:56:49Z

Actually, I have no idea why I set it up as smp-master and not master in the first place... maybe just go ahead and I'll change the default branch...

nigelmegitt · 2021-01-12T13:29:44Z

Oh yeah, I don't know either - maybe you want a special SMP branch for SMP use and everyone else can choose master or make their own special main branch?

jlks · 2021-01-14T13:30:00Z

Is this PR ready to merge?

nigelmegitt · 2021-01-14T14:21:30Z

It has no "approve" reviews, so I'd argue no! I think only one would be required. I think the approach to the mutable options object should be changed though, to put the canonical values into a separate child object rather than breaking the one sent in.

nigelmegitt · 2021-01-15T09:35:05Z

In 13dc9a5 I attempted (again) to fix the mutating options object problem - hopefully this time it'll work?!

I also added a multiplier adjustment for tts:lineHeight, and tweaked fontFamily and fontSize to apply to p elements. This has the effect that, for our subtitle files, which specify those attributes above the span level in the TTML tree, they are specified as CSS styles on the HTML p element.

At least in my experiments this results in the line spacing being computed to the expected value from the TTML. Further detail in https://codepen.io/nigelmegitt/pen/qBaJZOv and also the related issues w3c/ttml2#1215 and sandflow#201.

domjacks · 2021-01-27T17:08:31Z

Could we update the BBC-CHANGELOG in this PR?

Allows rendered text size to be modified by some scale value

If we want to add other config options that can be done without an API change. Also made it so that if the options are omitted as per previous API, everything still works.

* color mapping adjustment * fontFamily override * region opacity multipler * text outline

Addresses review feedback

Should stop the options from mutating. There must be a better way though, this is horrible.

…les to spans

nigelmegitt requested review from jlks and robertbryer January 5, 2021 16:02

jlks reviewed Jan 11, 2021

View reviewed changes

jlks approved these changes Jan 26, 2021

View reviewed changes

domjacks requested a review from a team January 26, 2021 16:36

nigelmegitt changed the base branch from master to smp-master January 28, 2021 10:49

nigelmegitt and others added 13 commits January 28, 2021 11:31

Add sizeAdjust parameter

316f036

Allows rendered text size to be modified by some scale value

Refactor config options

5fdc579

If we want to add other config options that can be done without an API change. Also made it so that if the options are omitted as per previous API, everything still works.

Update dist builds

077452c

Add more options

d1001e0

* color mapping adjustment * fontFamily override * region opacity multipler * text outline

Add option for mapping span backgroundColor

ebb423a

backgroundColor for span, p, div, body and region

f43a4f0

Don't mutate options

afb986f

Addresses review feedback

Copy options attributes

f4f2fe9

Should stop the options from mutating. There must be a better way though, this is horrible.

options copy, lineHeight adjust and fontSize and fontFamily on p

f0e6015

Need to declare as variables; they're going to global

c7a655f

Add default background colour; fillgaps to default

8bdab64

Reset background color to transparent, push down background color sty…

fd9cbd3

…les to spans

Add changes to BBC-CHANGELOG.md

2864dbd

nigelmegitt force-pushed the nm-size-adjust branch from af659e3 to 2864dbd Compare January 28, 2021 11:31

domjacks changed the title ~~Update to upstream and add customisation options~~ Add customisation options Jan 28, 2021

Add in more missing vars

413f5f6

robertbryer approved these changes Jan 28, 2021

View reviewed changes

robertbryer merged commit 9201ec9 into smp-master Jan 28, 2021

robertbryer deleted the nm-size-adjust branch January 28, 2021 17:33

nigelmegitt mentioned this pull request May 2, 2024

Add presentation customisation options sandflow/imscJS#256

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add customisation options #2

Add customisation options #2

nigelmegitt commented Jan 5, 2021 •

edited by domjacks

jlks left a comment

jlks Jan 11, 2021

nigelmegitt Jan 11, 2021

jlks Jan 11, 2021

nigelmegitt Jan 11, 2021

nigelmegitt Jan 11, 2021

jlks Jan 14, 2021

nigelmegitt Jan 14, 2021

jlks Jan 14, 2021

nigelmegitt Jan 14, 2021

nigelmegitt Jan 15, 2021

robertbryer commented Jan 12, 2021

robertbryer commented Jan 12, 2021

nigelmegitt commented Jan 12, 2021

jlks commented Jan 14, 2021

nigelmegitt commented Jan 14, 2021

nigelmegitt commented Jan 15, 2021

domjacks commented Jan 27, 2021

-            options: options || {}
+            options: {}
+        };
+        if (options) {
+            if (options.colorAdjust)
+            context.options.colorAdjust = preprocessColorMapOptions(options.colorAdjust);
+            var bgcColorElements = ['region', 'body', 'div', 'p', 'span'];
+            var propName;
+            for (var bgcei in bgcColorElements)
+            {
+                propName = bgcColorElements[bgcei] + backgroundColorAdjustSuffix;
+                if (options[propName])
+                context.options[propName] = preprocessColorMapOptions(options[propName]);
+            }
+        }

	options: options \|\| {}
	options: Object.assign({}, options) \|\| {},
	};

Add customisation options #2

Add customisation options #2

Conversation

nigelmegitt commented Jan 5, 2021 • edited by domjacks

jlks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertbryer commented Jan 12, 2021

robertbryer commented Jan 12, 2021

nigelmegitt commented Jan 12, 2021

jlks commented Jan 14, 2021

nigelmegitt commented Jan 14, 2021

nigelmegitt commented Jan 15, 2021

domjacks commented Jan 27, 2021

nigelmegitt commented Jan 5, 2021 •

edited by domjacks