Allow cover to be full-bleed image (BL-13271)

[andrew-polk]

This change is 

[hatton]

Reviewed 2 of 12 files at r1, all commit messages.
Reviewable status: 2 of 12 files reviewed, 1 unresolved discussion (waiting on @andrew-polk)

---

src/BloomBrowserUI/bookEdit/bookSettings/BookSettingsDialog.tsx line 202 at r1 (raw file):

    );
    const coverIsImageDescription = useL10n(
        "For paper books, this image will be full bleed.",

What can we say that will help people who don't know that term} I'm tempted to just not say anything. Best try:
"Using this option turns on the Print Bleed indicators on paper layouts".

[hatton]

Reviewable status: 2 of 12 files reviewed, 1 unresolved discussion (waiting on @andrew-polk)

---

src/BloomBrowserUI/bookEdit/bookSettings/BookSettingsDialog.tsx line 202 at r1 (raw file):

Previously, hatton (John Hatton) wrote…

What can we say that will help people who don't know that term} I'm tempted to just not say anything. Best try:
"Using this option turns on the Print Bleed indicators on paper layouts".

Yeah that works. Or "Enabling this option". Shall we also add a pointer to the inevitable docs?

"Using this option turns on the Print Bleed indicators on paper layouts. See Full Page Cover Images for information on sizing your image to fit.".

[andrew-polk]

src/BloomBrowserUI/bookEdit/bookSettings/BookSettingsDialog.tsx line 202 at r1 (raw file):

Previously, hatton (John Hatton) wrote…

Yeah that works. Or "Enabling this option". Shall we also add a pointer to the inevitable docs?

"Using this option turns on the Print Bleed indicators on paper layouts. See Full Page Cover Images for information on sizing your image to fit.".

Is "Print Bleed" a link to an external website?
More and more, I'm liking all our links going to our own docs where we can direct people to external sites.

[andrew-polk]

Reviewable status: 2 of 12 files reviewed, 1 unresolved discussion (waiting on @hatton)

---

src/BloomBrowserUI/bookEdit/bookSettings/BookSettingsDialog.tsx line 202 at r1 (raw file):

Previously, andrew-polk wrote…

Is "Print Bleed" a link to an external website?
More and more, I'm liking all our links going to our own docs where we can direct people to external sites.

I've made the change and put a TODO in the code for actual links.

[hatton]

Reviewed 7 of 12 files at r1, 2 of 3 files at r2, all commit messages.
Reviewable status: 11 of 12 files reviewed, 2 unresolved discussions (waiting on @andrew-polk)

---

src/BloomExe/Book/AppearanceSettings.cs line 112 at r2 (raw file):

        new StringPropertyDef("cssThemeName", "cssThemeName", "default"),
        // Cover page is just a full bleed image.
        new BooleanPropertyDef("coverIsImage", "coverIsImage", false),

Please add a comment explaining why this needs to be a BooleanPropertyDef and cannot be a CssPropertyDef, thx.

---

src/BloomExe/Book/AppearanceSettings.cs line 1035 at r2 (raw file):

}

public class BooleanPropertyDef : PropertyDef

Please add a comment explaining how this is different from CssPropertyDef

[hatton]

Reviewable status: 11 of 12 files reviewed, 3 unresolved discussions (waiting on @andrew-polk)

---

src/BloomExe/Book/AppearanceSettings.cs line 208 at r2 (raw file):

    public bool PendingChangeRequiresXmatterUpdate;

    private static readonly string[] s_propertiesWhichRequireXmatterUpdate = new string[]

Please add a comment explaining why this one property is different. Every existing property has defaults, many of them don't make sense in legacy, but we didn't have to introduce all this to support them.

[hatton]

Reviewable status: 11 of 12 files reviewed, 3 unresolved discussions (waiting on @andrew-polk)

---

src/BloomExe/Book/AppearanceSettings.cs line 215 at r2 (raw file):

        new Dictionary<string, object>
        {
            { "coverIsImage", false }

Please add a comment explaining why this one property is different. Every existing property has defaults, many of them don't make sense in legacy, but we didn't have to introduce all this to support them.

[hatton]

Reviewable status: 11 of 12 files reviewed, 4 unresolved discussions (waiting on @andrew-polk and @hatton)

---

src/BloomExe/Book/AppearanceSettings.cs line 208 at r2 (raw file):

    public bool PendingChangeRequiresXmatterUpdate;

    private static readonly string[] s_propertiesWhichRequireXmatterUpdate = new string[]

Instead of having a new list, would it be cleaner to add a new property to exiting definitions (CssStringVariableDef or BooleanPropertyDef) ?

[JohnThomson]

Reviewable status: 11 of 12 files reviewed, 12 unresolved discussions (waiting on @andrew-polk and @hatton)

a discussion (no related file):
Did you consider doing this entirely with CSS, so that we don't need to change the xmatter html? I'm not sure it's possible, but I would think pretty much all front cover pages have a marginBox containing a .bloom-imageContainer.data-title. If that's true, would something like this work?

.cover-is-image {
  * {display:none}
  .marginBox {
    display: block;
    height: 100%; // etc
    * {display: none}
    .imageContainer.data-title {
      display: block;
      height: 100% // etc
...

Apart from simplifying the code if we don't have to reload xmatter, I have a vague feeling that it might be a good thing not to break the expectation that books always have a title on the front cover (even if it's not visible). I don't know of any code that expects that, but it wouldn't greatly surprise me to find some.
Also, if these rules were in basePage.less, the new feature would just automatically not happen in legacy, without any special cases (except perhaps to disable the control).
Another possible benefit is that if the title is still there, even if hidden, a talking book might be able to read it. Don't know whether we want that...we still couldn't highlight it in the proper place inside the cover image...

---

---

src/BloomBrowserUI/bookEdit/bookSettings/BookSettingsDialog.tsx line 201 at r2 (raw file):

        "BookSettings.CoverIsImage"
    );
    //TODO real links (and change .xlf)

Is this ToDo still applicable?

---

src/BloomBrowserUI/react_components/requiresBloomEnterprise.tsx line 524 at r2 (raw file):

            {enterpriseAvailable ? (
                <Span l10nKey={"AvailableWithEnterprise"}>
                    Available with your Enterprise Subscription

To me this phrase is ambiguous between "You can do this because you have a subscription" and "you could do this if only you had a subscription", with the latter (wrong) interpretation being more likely. @hatton

---

src/BloomExe/Book/AppearanceSettings.cs line 184 at r2 (raw file):

    }

    private void EvaluateForXmatterChangeNeeded(KeyValuePair<string, object> property)

This would be much easier to read if it was clear that the "object" of the KVP is the value that we are about to change the property to.
You could simply give it two arguments, propertyName and newValue.
Or at least name the argument propertyNameAndNewValue.
Also consider making this responsible for doing the change. It's plausible for a method SetPropertyValue to first evaluate whether the property is actually changing, and if so, figure out whether an xmatter refresh is needed. Both callers immediately proceed to change the value.

---

src/BloomExe/Book/AppearanceSettings.cs line 212 at r2 (raw file):

        "coverIsImage"
    };
    private static readonly Dictionary<string, object> s_mapOfPropertyValuesRequiredByLegacyTheme =

Wants a comment explaining what the keys and values are (I think, property name -> value the property must have in legacy.

---

src/content/bookLayout/basePage.less line 709 at r2 (raw file):

    --page-background-color: @dark !important;

    background-color: @dark !important;

Why do we even want it? If the image covers the front cover we don't need it for areas outside the image, and if the image has transparent areas, isn't it more likely we want them white than dark?

---

src/content/templates/xMatter/bloom-xmatter-common.less line 114 at r2 (raw file):

            margin: 0mm;
            height: 100%;
        }

I'm not clear on the purpose of xmatter-common. Seems like stuff that is common to all x-matter could just be in basePage.css.
In this case, especially, I wonder if that would simplify things because this rule would just not be present in the legacy version of basePage.css, so it automatically wouldn't happen there.

---

src/content/templates/xMatter/CoverIsImage-XMatter/CoverIsImage-XMatter.pug line 9 at r2 (raw file):

	body
		+page-cover('Front Cover')(data-export='front-matter-cover', data-xmatter-page='frontCover').cover-is-image.no-margin-page.frontCover.outsideFrontCover#b5169df5-6a40-4c52-bd30-4cab45afe0ed
			.split-pane-component-inner

Isn't this an origami thing? I don't see it in the front cover of my current Bloom book.

[andrew-polk]

Reviewable status: 10 of 12 files reviewed, 12 unresolved discussions (waiting on @hatton and @JohnThomson)

---

src/BloomBrowserUI/bookEdit/bookSettings/BookSettingsDialog.tsx line 201 at r2 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Is this ToDo still applicable?

yes, right now they are just links to the main docs site.
I think the first is supposed to be an external link, and the second will be to a yet-created docs page.

---

src/content/bookLayout/basePage.less line 709 at r2 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Why do we even want it? If the image covers the front cover we don't need it for areas outside the image, and if the image has transparent areas, isn't it more likely we want them white than dark?

Good question.

Regarding the image filling the container:
The way I originally implemented this was with black bars if the image wasn't the same aspect ratio as the page (object-fit: contain;). But then I decided to do what Paper Comic pages do and fill the container (object-fit: cover;).
When I asked JH, he wasn't sure which was desirable.
@hatton, if you think object-fit: cover; is right, we might be able to remove this. But see below.

Regarding transparency:
I admit I hadn't thought of how transparent images would be affected.
If we didn't do this and left the image transparent, they won't get white, either, but rather the cover color.
So, for a transparent image, do we want cover color, dark gray, or white?
FWIW,

• Current digital and paper comic (when using the respective template pages):
  • cover: black (because that is the cover color)
  • content page: white

---

src/content/templates/xMatter/CoverIsImage-XMatter/CoverIsImage-XMatter.pug line 9 at r2 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Isn't this an origami thing? I don't see it in the front cover of my current Bloom book.

Good catch. I must have started with paper comic page instead of an existing cover.

[andrew-polk]

Reviewable status: 10 of 12 files reviewed, 12 unresolved discussions (waiting on @hatton and @JohnThomson)

a discussion (no related file):

Previously, JohnThomson (John Thomson) wrote…

Did you consider doing this entirely with CSS, so that we don't need to change the xmatter html? I'm not sure it's possible, but I would think pretty much all front cover pages have a marginBox containing a .bloom-imageContainer.data-title. If that's true, would something like this work?

.cover-is-image {
  * {display:none}
  .marginBox {
    display: block;
    height: 100%; // etc
    * {display: none}
    .imageContainer.data-title {
      display: block;
      height: 100% // etc
...

Apart from simplifying the code if we don't have to reload xmatter, I have a vague feeling that it might be a good thing not to break the expectation that books always have a title on the front cover (even if it's not visible). I don't know of any code that expects that, but it wouldn't greatly surprise me to find some.
Also, if these rules were in basePage.less, the new feature would just automatically not happen in legacy, without any special cases (except perhaps to disable the control).
Another possible benefit is that if the title is still there, even if hidden, a talking book might be able to read it. Don't know whether we want that...we still couldn't highlight it in the proper place inside the cover image...

I did make an attempt to do this via css rather than changing the DOM.
A few points came up which ultimately drove me the other way:

1. There were little corners to deal with in various places like qtips showing up for text boxes which were there but covered by the image.
   • JH pointed out that we may have lots of those things coming up like toolbox interactions with text boxes. So, my understanding was that it would undesirable for the Talking Book tool to try to interact with these text boxes. That was actually the final straw which pushed me fully to the DOM approach. Of course, if we decided we do want the user to be able record the title and have it play on the cover, that would be a strong argument for the css approach.
2. We can't have a pure css solution, anyway, since the appearance settings currently has no way set a class (and rules can't be turned on/off by the value of a css variable.
   • We talked briefly about how we would introduce a generic way for an appearance setting to turn a class on or off. We would go back to that approach if we go this direction.

I didn't try implementing it exactly the way you have it above. I'll give a shot again as I think it does simplify some of the things I was struggling with in my css approach. It may even be that if the divs are display:none, their qtips don't display (and perhaps even the Talking Book tool ignores them?

---

[andrew-polk]

Reviewable status: 10 of 12 files reviewed, 12 unresolved discussions (waiting on @hatton and @JohnThomson)

---

src/BloomExe/Book/AppearanceSettings.cs line 184 at r2 (raw file):

Previously, JohnThomson (John Thomson) wrote…

This would be much easier to read if it was clear that the "object" of the KVP is the value that we are about to change the property to.
You could simply give it two arguments, propertyName and newValue.
Or at least name the argument propertyNameAndNewValue.
Also consider making this responsible for doing the change. It's plausible for a method SetPropertyValue to first evaluate whether the property is actually changing, and if so, figure out whether an xmatter refresh is needed. Both callers immediately proceed to change the value.

I've attempted to improve things.

---

src/BloomExe/Book/AppearanceSettings.cs line 208 at r2 (raw file):

Previously, hatton (John Hatton) wrote…

Instead of having a new list, would it be cleaner to add a new property to exiting definitions (CssStringVariableDef or BooleanPropertyDef) ?

I wanted to do it that way, but at the time we are changing values, we no longer have access to our PropertyDefs. Or at least, I couldn't figure out how to access it.

---

src/BloomExe/Book/AppearanceSettings.cs line 212 at r2 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Wants a comment explaining what the keys and values are (I think, property name -> value the property must have in legacy.

I think this is clearer now, but maybe it still wants more commenting?

---

src/BloomExe/Book/AppearanceSettings.cs line 215 at r2 (raw file):

Previously, hatton (John Hatton) wrote…

Please add a comment explaining why this one property is different. Every existing property has defaults, many of them don't make sense in legacy, but we didn't have to introduce all this to support them.

Actually, I don't think this one is different. I think it was an oversight in the original implementation. I've added a review comment. Let me know what you think.

[andrew-polk]

src/BloomExe/Book/AppearanceSettings.cs line 208 at r2 (raw file):

Previously, andrew-polk wrote…

I wanted to do it that way, but at the time we are changing values, we no longer have access to our PropertyDefs. Or at least, I couldn't figure out how to access it.

Hm. I see now that I should just be able to get it by matching key to Name, so I'm not sure what my problem was before.
I'll pursue this after I verify we don't want go back to the css approach.

[hatton]

Reviewable status: 10 of 12 files reviewed, 12 unresolved discussions (waiting on @andrew-polk and @JohnThomson)

---

src/BloomBrowserUI/react_components/requiresBloomEnterprise.tsx line 524 at r2 (raw file):

Previously, JohnThomson (John Thomson) wrote…

To me this phrase is ambiguous between "You can do this because you have a subscription" and "you could do this if only you had a subscription", with the latter (wrong) interpretation being more likely. @hatton

Is it only ambiguous to you because you don't know if you have an Enterprise Subscription? If you had one, and it was enabled, what would be your confusion?

---

src/BloomExe/Book/AppearanceSettings.cs line 215 at r2 (raw file):

Previously, andrew-polk wrote…

Actually, I don't think this one is different. I think it was an oversight in the original implementation. I've added a review comment. Let me know what you think.

Sorry, I don't understand your reply or know what a "review comment" is, expect this conversation here. The heat mut be getting to me!

---

src/content/bookLayout/basePage.less line 709 at r2 (raw file):

Previously, andrew-polk wrote…

Good question.

Regarding the image filling the container:
The way I originally implemented this was with black bars if the image wasn't the same aspect ratio as the page (object-fit: contain;). But then I decided to do what Paper Comic pages do and fill the container (object-fit: cover;).
When I asked JH, he wasn't sure which was desirable.
@hatton, if you think object-fit: cover; is right, we might be able to remove this. But see below.

Regarding transparency:
I admit I hadn't thought of how transparent images would be affected.
If we didn't do this and left the image transparent, they won't get white, either, but rather the cover color.
So, for a transparent image, do we want cover color, dark gray, or white?
FWIW,

• Current digital and paper comic (when using the respective template pages):
  • cover: black (because that is the cover color)
  • content page: white

I'm not sure I'm following, but if the image doesn't cover the page in some dimension, the printer should not print anything in the extra areas (e.g. white).

[andrew-polk]

Reviewable status: 10 of 12 files reviewed, 12 unresolved discussions (waiting on @hatton and @JohnThomson)

---

src/BloomExe/Book/AppearanceSettings.cs line 215 at r2 (raw file):

Previously, hatton (John Hatton) wrote…

Sorry, I don't understand your reply or know what a "review comment" is, expect this conversation here. The heat mut be getting to me!

Sorry; I moved the code, so that probably confused things.
I added this in the code before this dictionary is defined:

            //REVIEW: I think that some of the other properties should also be here.
            // This concept of forcing a value based on the legacy theme was introduced at the time coverIsImage was added.
            // But I think the properties which existed before that and which get disabled by setting the theme
            // to legacy should also be here.
            // e.g. cover-topic-show
            // Without this, the user can set the theme to non-legacy, change the property to whatever he wants,
            // then change the theme back to legacy.

---

src/BloomExe/Book/AppearanceSettings.cs line 1035 at r2 (raw file):

Previously, hatton (John Hatton) wrote…

Please add a comment explaining how this is different from CssPropertyDef

Done.

[andrew-polk]

Reviewable status: 10 of 12 files reviewed, 12 unresolved discussions (waiting on @hatton and @JohnThomson)

---

src/content/templates/xMatter/CoverIsImage-XMatter/CoverIsImage-XMatter.pug line 9 at r2 (raw file):

Previously, andrew-polk wrote…

Good catch. I must have started with paper comic page instead of an existing cover.

I removed it.

[andrew-polk]

src/content/bookLayout/basePage.less line 709 at r2 (raw file):

if the image doesn't cover the page in some dimension, the printer should not print anything in the extra areas (e.g. white).

Apparently we made a very specific decision at some point in the past that the PDF for a full bleed book should include the cover color, not white as non-full-bleed books do.
aa581d1

I think that makes sense if you have a normal cover with stuff besides the image. You're sending the cover color you want printed to the printer. (And the user can alway set the cover color to white if desired.)
Are you saying we want to make an exception for this cover-is-image case?

I think its mostly a moot point currently. With the object-fit: cover;, the image will always cover the whole page, and only transparency will matter. And I can't imagine a real use case of cover-is-image with a transparent image.