Reword API doc

[JayPanoz]

Resolves #78

@mickael-menu Given you opened the related issue, may I ask you for a quick look/review?

I’ve tried replacing “default” with “preset” + reworked the sections intro but maybe something else would make more sense.

[mickael-menu]

@JayPanoz Thanks, I think it's much clearer.

Just to be sure, in the Swift toolkit, we use for example readium-scroll-off to unset --scroll-view. But if I understand correctly, this value doesn't exist in ReadiumCSS? It's just a convention in the Swift toolkit, but we could as well put an empty string?

[JayPanoz]

@mickael-menu Thanks, and you’re right, that’s correct.

So I just checked User Settings’ Flags section and the convention exists here, although I’m not sure it was applied from the docs, or the docs caught up on what people had been doing.

Maybe I should update this doc too, and replace the [flag-value-off] pattern with something like “Any other value will disable the flag”?

[mickael-menu]

Maybe I should update this doc too, and replace the [flag-value-off] pattern with something like “Any other value will disable the flag”?

Yes I think it would be good. And to be coherent, maybe we should use an empty string for the off value, or maybe not put the attribute at all if possible, in the apps.

[JayPanoz]

Ah I did a quick test after checking the CSS custom props spec, especially valid/invalid values.

So empty string being an invalid value, this has the following side effect in JS:

root.style.setProperty("--foo", "") = root.style.removeProperty("--foo")

So an empty string removes the prop entirely, except when the var exists in the markup (say streamer put it here) and its value is invalid itself (no space between : and ;) – which is a weird quirk, although it doesn’t have any impact since the value is invalid so it will be ignored.

That said I’m not sure this is a pro, or a con, and whether it won’t create comprehension/debugging issues due to the unexpectedness. All I can say myself is that it would perhaps be preferable to have a discussion about that with known implementers weighing in? I can make a quick demo on codepen if needed.

[Edit] Note setProperty with empty value invoking removeProperty is specced

[mickael-menu]

Sounds like a pro to me, since that's actually what we want, not specifying a value?

But yeah we can raise the issue on Slack or/and the dev call.

If empty strings are not suitable, I think that we should then document a proper enum of values (including the -off values), and specify which value is the fallback in case of invalid or missing value – so allegedly the -off ones.

[JayPanoz]

Yeah to me too since it’s consistent with the JS syntax for non custom CSS properties:

root.style.prop = ""

And it’s actually sweet in the sense you don’t have to bother about which method to use, only care about the value.

But I’d just like to make sure it doesn’t create an issue in an existing implementation – because it is relying on the custom property being there or an “off” value. That said, the only one I can think of is the desktop test app so that would be about making sure @danielweck doesn’t raise any issue about this change.

[danielweck]

The r2-navigator-js Electron/Chromium implementation defines its own CSS styles using high-specificity selectors in order to override the cascade defined by ReadiumCSS, for example: :root[style*="readium-sepia-on"] MY_OWN_LOCAL_CSS_SELECTOR { ... }

So, I checked: only readium-sepia-on and readium-night-on are referenced explicitly. Sometimes, navigator styles use a more generic :root[style] ... as well, in order to win the CSS selector specificity "game".

[danielweck]

Also, note that r2-navigator-js does not set "things" (because I am not sure what this is) such as --scroll-view, but instead always sets / unsets --USER__xxx properties directly on the root document element, e.g.:

window.document.documentElement.style.setProperty("--USER__view", "readium-scroll-on");

or:

window.document.documentElement.style.removeProperty("--USER__fontSize");

Is that the correct way of doing it?

[danielweck]

As previously mentioned, r2-navigator-js depends on readium-sepia-on and readium-night-on in order to create CSS selectors with high specificity. Additionally, we also set the following "off" values, but we don't actually "depend" on them (i.e. we don't define style selectors that expect the presence of "off" values)

• --USER__advancedSettings (readium-advanced-on / readium-advanced-off)
• --USER__darkenFilter (readium-darken-on / readium-darken-off)
• --USER__invertFilter (readium-invert-on / readium-invert-off)
• --USER__a11yNormalize (readium-a11y-on / readium-a11y-off)
• --USER__fontOverride (readium-font-on / readium-font-off)

[JayPanoz]

Is that the correct way of doing it?

Yes. And TIL

window.document.documentElement.style.setProperty("--USER__view", "");

will invoke

window.document.documentElement.style.removeProperty("--USER__view");

So it seems reasonable we only keep the values which are used in ReadiumCSS, and replace the ones that aren’t with empty string or remove property.

Thanks!

[JayPanoz]

Additionally, we also set the following "off" values, but we don't actually "depend" on them (i.e. we don't define style selectors that expect the presence of "off" values)

Thanks for the confirmation. I will change the CSS-12 User Settings docs as well then, before merging.

[JayPanoz]

OK double-checked the entire codebase to make sure the readium-*-off values aren’t used anywhere – in case I missed hardcoded ones at some point in dev. I’m not expecting breaking changes by switching to the empty string in the docs. I’ll merge in a second.

Thanks for the reviews, feedbacks and insights everyone. 🙏