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.

[andrew-polk]

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

---

a discussion (no related file):

Previously, andrew-polk wrote…

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?

I went back to trying to do this with css and got stuck enough that JH said it wasn't worth it. We decided to go with the DOM swap approach I had implemented orginally.

---

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

Previously, hatton (John Hatton) wrote…

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

Done.

---

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

Previously, andrew-polk wrote…

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.

Done.

---

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

Previously, JohnThomson (John Thomson) wrote…

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.

I could certainly move this to basePage.less (and I don't think I have any principled reason for it to be in one place vs. the other).
But I don't see how it would simplify things.
I think we would still have to prevent the user from turning it on in legacy mode which is what all the code in AppearanceSettings.cs is for.

[JohnThomson]

I picked up a few more things to at least question. Maybe being too picky. It could merge as-is, though one of us will have to figure out how it combines with cover-image-is-background-overlay.

Reviewed 3 of 12 files at r1, 1 of 1 files at r3, 7 of 8 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @andrew-polk and @hatton)

---

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

Previously, andrew-polk wrote…

I've attempted to improve things.

Definitely improved. See my comment below, though.

---

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

Previously, andrew-polk wrote…

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

I still like JohnH's idea that RequiredValueForLegacyTheme could be a field in PropertyDef, assuming that we don't have properties whose value must be null, which at the moment we don't. Then the bulk of SetRequiredValuesIfLegacyTheme would just be a loop over Properties, calling SetProperty(pd.Name, pd.RequiredValueForLegacyTheme) for each of them. (Of course, that requires making SetProperty take the key and value as separate arguments. And either the caller or callee must handle RequiredValueForLegacyTheme being null.)

---

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

Previously, andrew-polk wrote…

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.

Ouch. My coming PR will mess this up, with the cover image becoming a background-overlay that is not using either content-fit or cover, but absolute size and position attributes set by code to make the size of the overlay simulate content-fit. If we want the "cover" effect it will have to be achieved with a special case in the code that sets those attributes.

And it's going to be a pain anyway, because the size adjusting normally only runs (in JS!) when the page is opened. But somehow the cover image will have to get adjusted to the right size and position every time we update the xmatter, even if the user never edits that page, at least before publication. I guess that's my problem, though.

One benefit is that if we do end up with crop controls on the cover, the user will have full control over what part of the image shows. Maybe that means we don't need a special case to make it simulate object-fit:cover?

Those things aside, I'm not seeing any case for new code to force a dark background. We want a special case for paper comic PDFs to show the cover color. We just possibly want a white background for partly-transparent images. Otherwise we want nothing printed where there isn't image.

---

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

Previously, andrew-polk wrote…

I could certainly move this to basePage.less (and I don't think I have any principled reason for it to be in one place vs. the other).
But I don't see how it would simplify things.
I think we would still have to prevent the user from turning it on in legacy mode which is what all the code in AppearanceSettings.cs is for.

Since you've thoroughly dealt with not allowing this to happen in legacy, I agree it doesn't matter much. And as long as we have an xmatter-common, that's quite possibly a good place to put rules that apply to all xmatter pages for all variations. I think we can leave it.

---

src/BloomExe/Book/XMatterHelper.cs line 322 at r7 (raw file):

            string metadataLangTag,
            List<string> oldIds = null,
            bool useCoverIsImage = false

This apparently means something like "use the front cover page from the special CoverIsImage template instead of the one from the book's own xmatter." That feels like exposing (and requiring understanding of) an implementation detail. How about something like "forceCoverToBeASingleImage"?
But even that isn't what this is heading towards. When my latest change lands, the cover won't be a simple image; it will be an image container with a single (background) image overlay. Someday we will probably allow other overlays on the cover, including (I hope) one identified as the book title. Not sure how that will be implemented; possibly there will be three options in appearance, something like Normal (Front Cover), Single Image, Single Image with moveable title? Or maybe some trick to identify a particular text overlay as the title?
How about coverImageShouldFillPage? Or maybe we get better consistency with other places by just calling it coverIsImage? Or mazimizeCoverImage? (I like that last one because it is clearer that it won't actually fill the page unless it is the right shape; content-fit still applies. But that's true everywhere we use coverIsImage.)

---

src/BloomExe/Book/XMatterHelper.cs line 387 at r7 (raw file):

                }
                else
                    newPageDiv = _bookDom.RawDom.ImportNode(xmatterPage, true) as SafeXmlElement;

Is it any improvement to start out with
var newPageSource = xmatterPage;
if (...) {
...
newPageSource = SelectSingleNode(...)
}
var newPageDiv = _bookDom.RawDom.ImportNode(newPageSource)...
?
Not shorter, but I think it makes it clearer that what's special about the new path is just finding a different source for the page to copy into the current book.

---

src/BloomExe/Book/XMatterHelper.cs line 536 at r7 (raw file):

            return pageDiv.SelectSingleNode("self::div[contains(@class, 'outsideFrontCover')]")
                != null;
        }

SafeXmlElement has a HasClass method that could be used here (or maybe make this method redundant)

---

src/BloomExe/Book/AppearanceSettings.cs line 872 at r7 (raw file):

        // and then for each property, copy into the Properties object.
        // The original comment here also said: For backwards capability, if the json we are reading has a null for a value, do not override the default value that we already have loaded.
        //  But the code didn't seem to implement that when I got here. (AP, Aug 2024)

As far as I can see, it still doesn't. You handle unknown properties and ones that haven't changed, but don't do anything special if the value is null. Did you intend to change this?

---

src/BloomExe/Book/AppearanceSettings.cs line 876 at r7 (raw file):

            SetProperty(property);

        SetRequiredValuesIfLegacyTheme();

I suppose it's OK that this (or the previous line) might set RequiresXmatterUpdate? Does every caller of this function follow it by saving the book, so that RequiresXmatterUpdate gets properly dealt with? Is it worth commenting on this method that it's important to follow it with a Save so that any required xmatter update happens?

---

src/BloomExe/Book/Book.cs line 4678 at r7 (raw file):

            if (BookInfo.AppearanceSettings.PendingChangeRequiresXmatterUpdate)
                EnsureUpToDateMemory(new NullProgress());

I'm not thrilled that we're doing this whole process just to get Xmatter updated, especially since the name EnsureUpToDateMemory implies that nothing will be done if we think it's already up to date. As it happens, this method doesn't currently check that, but I think only because it's a chunk of the main EnsureUpToDate which does. In fact, I think I left it as a separate method only to better preserve our Git change history; it's unfortunate for such an arbitrary chunk to be further established by giving it another caller.
Could we pull out the chunk of EnsureUpToDateMemory that actually updates xmatter and give it a less conditional-sounding name?

[andrew-polk]

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

Previously, andrew-polk wrote…

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.

I added a checkbox to the card so this doesn't get lost.

[andrew-polk]

Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @hatton and @JohnThomson)

---

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

Previously, JohnThomson (John Thomson) wrote…

I still like JohnH's idea that RequiredValueForLegacyTheme could be a field in PropertyDef, assuming that we don't have properties whose value must be null, which at the moment we don't. Then the bulk of SetRequiredValuesIfLegacyTheme would just be a loop over Properties, calling SetProperty(pd.Name, pd.RequiredValueForLegacyTheme) for each of them. (Of course, that requires making SetProperty take the key and value as separate arguments. And either the caller or callee must handle RequiredValueForLegacyTheme being null.)

Done. (I took his comment to mean adding the xmatter change as a field, which I had done. But it makes sense for both.)

---

src/BloomExe/Book/AppearanceSettings.cs line 872 at r7 (raw file):

Previously, JohnThomson (John Thomson) wrote…

As far as I can see, it still doesn't. You handle unknown properties and ones that haven't changed, but don't do anything special if the value is null. Did you intend to change this?

No; just stating that the comment seems to be wrong. If you agree, I can just remove it.

---

src/BloomExe/Book/AppearanceSettings.cs line 876 at r7 (raw file):

Previously, JohnThomson (John Thomson) wrote…

I suppose it's OK that this (or the previous line) might set RequiresXmatterUpdate? Does every caller of this function follow it by saving the book, so that RequiresXmatterUpdate gets properly dealt with? Is it worth commenting on this method that it's important to follow it with a Save so that any required xmatter update happens?

I don't think we actually do call Save on all paths.
I'll think about this some more. It is tied in with the comments below about calling EnsureUpToDateMemory.

---

src/BloomExe/Book/Book.cs line 4678 at r7 (raw file):

Previously, JohnThomson (John Thomson) wrote…

I'm not thrilled that we're doing this whole process just to get Xmatter updated, especially since the name EnsureUpToDateMemory implies that nothing will be done if we think it's already up to date. As it happens, this method doesn't currently check that, but I think only because it's a chunk of the main EnsureUpToDate which does. In fact, I think I left it as a separate method only to better preserve our Git change history; it's unfortunate for such an arbitrary chunk to be further established by giving it another caller.
Could we pull out the chunk of EnsureUpToDateMemory that actually updates xmatter and give it a less conditional-sounding name?

Sigh. We do have a BringXmatterHtmlUpToDate already, but apparently if we only call that here, we lose the data-book values. E.g. the title is lost. I guess I need to investigate more why that is...
I agree this is some less than ideal code... maybe we can discuss other ideas.

Note, there is a comment on the only caller to EnsureUpToDateMemory which states it does a check to see if it needs to be done, but I don't think that comment is accurate. I added a review comment in the code there. But if you agree, I can just remove the comment.

---

src/BloomExe/Book/XMatterHelper.cs line 322 at r7 (raw file):

Previously, JohnThomson (John Thomson) wrote…

This apparently means something like "use the front cover page from the special CoverIsImage template instead of the one from the book's own xmatter." That feels like exposing (and requiring understanding of) an implementation detail. How about something like "forceCoverToBeASingleImage"?
But even that isn't what this is heading towards. When my latest change lands, the cover won't be a simple image; it will be an image container with a single (background) image overlay. Someday we will probably allow other overlays on the cover, including (I hope) one identified as the book title. Not sure how that will be implemented; possibly there will be three options in appearance, something like Normal (Front Cover), Single Image, Single Image with moveable title? Or maybe some trick to identify a particular text overlay as the title?
How about coverImageShouldFillPage? Or maybe we get better consistency with other places by just calling it coverIsImage? Or mazimizeCoverImage? (I like that last one because it is clearer that it won't actually fill the page unless it is the right shape; content-fit still applies. But that's true everywhere we use coverIsImage.)

Actually, we don't use object-fit: contain in this case; we use object-fit: cover.
That happens because the img in the new pug gets the bloom-imageObjectFit-cover class.
So it truly does fill the page.

The only caller which passes this right now calls it with BookInfo.AppearanceSettings.CoverIsImage && CollectionSettings.HaveEnterpriseFeatures, so I've gone with coverIsImage for now, but open to more discussion.

---

src/BloomExe/Book/XMatterHelper.cs line 387 at r7 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Is it any improvement to start out with
var newPageSource = xmatterPage;
if (...) {
...
newPageSource = SelectSingleNode(...)
}
var newPageDiv = _bookDom.RawDom.ImportNode(newPageSource)...
?
Not shorter, but I think it makes it clearer that what's special about the new path is just finding a different source for the page to copy into the current book.

Yes, that's better. Thanks.

---

src/BloomExe/Book/XMatterHelper.cs line 536 at r7 (raw file):

Previously, JohnThomson (John Thomson) wrote…

SafeXmlElement has a HasClass method that could be used here (or maybe make this method redundant)

Done.

---

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

Previously, JohnThomson (John Thomson) wrote…

Ouch. My coming PR will mess this up, with the cover image becoming a background-overlay that is not using either content-fit or cover, but absolute size and position attributes set by code to make the size of the overlay simulate content-fit. If we want the "cover" effect it will have to be achieved with a special case in the code that sets those attributes.

And it's going to be a pain anyway, because the size adjusting normally only runs (in JS!) when the page is opened. But somehow the cover image will have to get adjusted to the right size and position every time we update the xmatter, even if the user never edits that page, at least before publication. I guess that's my problem, though.

One benefit is that if we do end up with crop controls on the cover, the user will have full control over what part of the image shows. Maybe that means we don't need a special case to make it simulate object-fit:cover?

Those things aside, I'm not seeing any case for new code to force a dark background. We want a special case for paper comic PDFs to show the cover color. We just possibly want a white background for partly-transparent images. Otherwise we want nothing printed where there isn't image.

The special case is because we are treating this page as full bleed.

[JohnThomson]

Reviewed 6 of 6 files at r8, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @andrew-polk and @hatton)

---

src/BloomExe/Book/AppearanceSettings.cs line 872 at r7 (raw file):

Previously, andrew-polk wrote…

No; just stating that the comment seems to be wrong. If you agree, I can just remove it.

It's not doing it, so either the comment or the code was wrong. We don't seem to have missed it, and backwards compatibility can't be much of an issue for appearance.json, so it's probably safe to just remove. I wish we knew what might have produced JSON that needed it, though.

---

src/BloomExe/Book/AppearanceSettings.cs line 876 at r7 (raw file):

Previously, andrew-polk wrote…

I don't think we actually do call Save on all paths.
I'll think about this some more. It is tied in with the comments below about calling EnsureUpToDateMemory.

Since you're still thinking I won't resolve this one yet.

---

src/BloomExe/Book/Book.cs line 4678 at r7 (raw file):

Previously, andrew-polk wrote…

Sigh. We do have a BringXmatterHtmlUpToDate already, but apparently if we only call that here, we lose the data-book values. E.g. the title is lost. I guess I need to investigate more why that is...
I agree this is some less than ideal code... maybe we can discuss other ideas.

Note, there is a comment on the only caller to EnsureUpToDateMemory which states it does a check to see if it needs to be done, but I don't think that comment is accurate. I added a review comment in the code there. But if you agree, I can just remove the comment.

Sorry to make you do that, but in the interests of improving the code rather than making it worse...seems that BringXmatterHtmlUpToDate SHOULD NOT destroy any book data. (It might plausibly save any data present in the current xmatter that isn't already in the data div, but removing is dangerous for this and other cases: we don't want to lose data-div data just because one xmatter doesn't use it.) It seems likely that we're using a version of BookData that is a member variable of the book, and (without realizing it) depending on something earlier in EnsureUpToDateMemory to initialize it properly...not sure if that idea will help...

---

src/BloomExe/Book/XMatterHelper.cs line 322 at r7 (raw file):

Previously, andrew-polk wrote…

Actually, we don't use object-fit: contain in this case; we use object-fit: cover.
That happens because the img in the new pug gets the bloom-imageObjectFit-cover class.
So it truly does fill the page.

The only caller which passes this right now calls it with BookInfo.AppearanceSettings.CoverIsImage && CollectionSettings.HaveEnterpriseFeatures, so I've gone with coverIsImage for now, but open to more discussion.

Good for now, but if you merge my image unifying PR, something will need more attention to preserve the object-fit:cover behavior.

---

src/BloomExe/Book/XMatterHelper.cs line 387 at r7 (raw file):

Previously, andrew-polk wrote…

Yes, that's better. Thanks.

Not important, but if you're doing another pass, there is no longer any need to declare newPageDiv before it is initialized.

---

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

Previously, andrew-polk wrote…

The special case is because we are treating this page as full bleed.

I'm happy to leave this for you and JohnH to work out, but it's not obvious to me why a full-bleed page needs a dark background, so a comment (or perhaps a cross-reference to a comment elsewhere) might be helpful.

---

src/BloomExe/Book/Book.cs line 1036 at r8 (raw file):

            string oldMetaData = string.Empty;

            // REVIEW: This comment doesn't seem to be true in Dec 2024:

Yes, I think this is an old comment, from some (brief) days when we used to do what updating we could without modifying files. Now we don't do any updating unless we can do it all the way.

[andrew-polk]

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

---

src/BloomExe/Book/AppearanceSettings.cs line 872 at r7 (raw file):

Previously, JohnThomson (John Thomson) wrote…

It's not doing it, so either the comment or the code was wrong. We don't seem to have missed it, and backwards compatibility can't be much of an issue for appearance.json, so it's probably safe to just remove. I wish we knew what might have produced JSON that needed it, though.

Done.

---

src/BloomExe/Book/Book.cs line 1036 at r8 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Yes, I think this is an old comment, from some (brief) days when we used to do what updating we could without modifying files. Now we don't do any updating unless we can do it all the way.

Done.

---

src/BloomExe/Book/XMatterHelper.cs line 322 at r7 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Good for now, but if you merge my image unifying PR, something will need more attention to preserve the object-fit:cover behavior.

As we discussed on Zulip, this will need the same treatment as full-bleed image pages such as in paper comics.

---

src/BloomExe/Book/XMatterHelper.cs line 387 at r7 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Not important, but if you're doing another pass, there is no longer any need to declare newPageDiv before it is initialized.

Done.

[andrew-polk]

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

---

src/BloomExe/Book/AppearanceSettings.cs line 876 at r7 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Since you're still thinking I won't resolve this one yet.

I did add some comments regarding this whole process in Book.cs.
I'm not sure that comments here are particularly helpful. (i.e. at what level(s) would you put them?)

---

src/BloomExe/Book/Book.cs line 4678 at r7 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Sorry to make you do that, but in the interests of improving the code rather than making it worse...seems that BringXmatterHtmlUpToDate SHOULD NOT destroy any book data. (It might plausibly save any data present in the current xmatter that isn't already in the data div, but removing is dangerous for this and other cases: we don't want to lose data-div data just because one xmatter doesn't use it.) It seems likely that we're using a version of BookData that is a member variable of the book, and (without realizing it) depending on something earlier in EnsureUpToDateMemory to initialize it properly...not sure if that idea will help...

I wasn't able to find a better way. I added some comments to that end.

---

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

Previously, JohnThomson (John Thomson) wrote…

I'm happy to leave this for you and JohnH to work out, but it's not obvious to me why a full-bleed page needs a dark background, so a comment (or perhaps a cross-reference to a comment elsewhere) might be helpful.

I figure JH will change it if it affects the UX in an undesirable way.
For now, I've just commented that I'm doing the same thing as paper comic pages.

[JohnThomson]

LGTM, however, one thing you did in response to one of JohnH's comments doesn't seem to have gotten committed. Also, the PR that makes all images be background images is now merged, which means cover and other images that are supposed to be object-fit:cover will need attention (not really because of this PR, but the new case of a cover image that is object-fit:cover will also be broken). BubbleManager.adjustBackgroundImageSize needs to somehow figure out that we want this and simulate that instead of contain.
I think it would be quite reasonable to just merge this, but in that case, you should create a card for the problem and cross-reference it from this card and also BL-14069

Reviewable status: 11 of 15 files reviewed, 4 unresolved discussions (waiting on @hatton)

---

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

Previously, andrew-polk wrote…

Done.

This was JohnH's comment, and I don't see the need; but you say it's done and I can't find anything new, so I thought I'd just point that out...