Enhance, debug, and clean up scripts for CSS migration (BL-12857)

[StephenMcConnel]

This change is 

[JohnThomson]

A few possible problems to think about. I've never seen the whole code, so some of them may be unhelpful.

[JohnThomson]

I'm not clear why we would not want to override earlier output...or if there really is a good reason not to, why it would be OK to overwrite every previous output except the first. (Same for appearance.json)

[StephenMcConnel]

This handles the case where both customCollectionStyles.css and customBookStyles.css have css that need to be migrated. By the time this code runs, the original filename is lost, so I compromised by providing two different output files. The migration code erases any earlier output when it starts, so a duplicate has to happen in this run.
It's quite possible I've been overly paranoid in this instance, and the problem never arises in practice.

[StephenMcConnel]

Now that I think about it, I think this situation never arises in the migration output folder because the migration is specific to a single file matching a specific checksum. So this code can be simplified here.

[JohnThomson]

That would be, customBookStyles.css and customCollectionStyles.css both have content? That's all we're downloading? Because it would be common, and not a problem, to have lots of others like basePage.css.

[StephenMcConnel]

Yes, that would be referring to customCollectionStyles.css and customBookStyles.css in the folder both having content. I've reworded the message to make that explicit. I've also decided it's not really a problem warranting a warning. It's quite reasonable for a collection to have a customized look, but for a single book in the collection to make another adjustment specific to that book.

[JohnThomson]

Technically, a rule whose selector includes .marginBox could go on to actually target something inside the marginBox. For example, .marginBox .narrowStyle {width: 200px} is fine. But I can't think of a likely reason for such a rule to mention .marginBox, so no point in improving the code unless we encounter a lot of it. Might just be worth commenting, though?

[StephenMcConnel]

I've added a comment based on what you wrote here.

[JohnThomson]

I think an overall purpose comment would be helpful here. Along the lines
We're looking for rules that affect the marginBox's size and position (top/left/height/width) because the new theme system controls these with variables that affect the .bloom-page margin instead of absolutely positioning the .marginBox, so setting its top or left will have no effect, and setting its height or width will probably do something unwanted. If such a rule also changes safe properties, we split it into two rules, one that has only the problem declarations and one that has only the safe ones. NOW EXPLAIN WHY THIS IS USEFUL. (Maybe some other code later looks for just the problem rules and does something with them?)
(Sorry, maybe it's obvious with more of the context than this review is showing me.)

[JohnThomson]

Please don't tell me WHAT you're doing in a comment, especially when it's easy to tell in the code. Tell me WHY we care about rounding a (surely very unlikely) marginBox width of less than ,05mm to zero.

[StephenMcConnel]

I've written a more extensive comment that covers both calculations.

[JohnThomson]

I'm puzzled.

1. Why do we want to sort selectors at all?
2. If I'm reading the calling code correctly, rule is a CssRule object (could we declare that?). These are not expected to have a selectors property, so maybe this code will never do anything?
3. Since I can't find any doc on this property, I'm not sure what it contains. For example, given an overall selector like ".bloom-page.A5Portrait .marginBox", is rule.selectors[0].trim() supposed to return ".bloom-page.A5Portrait"? So this would convert it to ".A5Portrait.bloom-page .marginBox"?
4. Is the first dot actually meant to match a dot, rather than any character? (Should it have a backslash?) So for example it then wouldn't attempt to sort ":not(.frontCover).bloom-page.front-matter"? (Of course it won't anyway because the parens don't match.)
5. If we need the trim(), it's presumably because rule.selectors[0] includes the space separating it from the next component of the selector. If that's true, so we need to put that space back below?
6. Assuming there IS a reason to sort them, will it matter than a selector like .bloom-page.A5Portrait:not(.front-matter):not(.back-matter) does not get changed (because the parens don't match)? (If you need to make it handle that, remember that splitting at dots and sorting will do something horrible to :not(.front-matter))

[StephenMcConnel]

1. I don't know that we need to, but JohnH's original code was doing this, albeit not as safely.
2. I haven't been able to find out anything about the classes used here. "bun" (which is how JohnH set things up) has all sorts of things built in that don't have any intellesense type operation.
3. The selector property is an array of strings that contain the comma separated values from the CSS selector of the rule. The trim() operation would only remove spaces around a comma or the overall beginning or end of the CSS selector.
4. good catch. :-) Yes, I only try to sort extremely simple selectors: no spaces, no colons, no parentheses...
5. We don't need to put the space back. The stringify method imported form css has its own ideas of formatting.
6. I think the only reason to sort the selector is to make simple cases easier to compare visually by human being. This may not be a compelling reason. :-)

[JohnThomson]

What happens with a rule like margin: 2px or margin: 2px 3px 4px 5px? Will we get a warning (because it translates into four margin-X declarations) or not (if we just get one declaration with property margin)?

[StephenMcConnel]

Some of these questions need to have JohnH here to answer them because I don't know enough about the appearance / theme setup to know whether margins and paddings are really problematic or not. His original code marked them as problematic, but then did nothing with them. I've added "margin:" and "padding:" to the mix in addition to "margin-" and "padding-".

[JohnThomson]

You're telling me WHAT you're doing, but not WHY. We now have a rule that sets some or all of top/left/height/width (and nothing else) for not only .marginBox (which we do NOT want to set these on), but potentially many other elements as well, which we also don't want, AFAIK. What good is that?

[StephenMcConnel]

I've extensively revised and expanded the comment.
The rule being modified has a selector which includes both .marginBox and a size property such as .A5Portrait. Removing the .marginBox from the selector results in variable setting influenced by the page size (and possibly other properties if the rule has other properties in the selector).

Remember, the output from the tool is supposed to be processed by a human being before being put to use. Think computer assisted migration. Quoting from the issue "We do not intend to make some huge AI version of migrate-css-files-to-variable-system.ts. We just want to provide some help to a human creating a replacement custom CSS."

[JohnThomson]

Thanks for clarifying. Just one thing still has questions, and I may be being too picky even there...

Reviewed 1 of 5 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @StephenMcConnel)

---

custom-css-analysis-and-migration/migrateCssToAppearance.ts line 187 at r1 (raw file):

Previously, StephenMcConnel (Steve McConnel) wrote…

I've written a more extensive comment that covers both calculations.

Much better, thanks.

---

custom-css-analysis-and-migration/migrateCssToAppearance.ts line 263 at r1 (raw file):

Previously, StephenMcConnel (Steve McConnel) wrote…

1. I don't know that we need to, but JohnH's original code was doing this, albeit not as safely.
2. I haven't been able to find out anything about the classes used here. "bun" (which is how JohnH set things up) has all sorts of things built in that don't have any intellesense type operation.
3. The selector property is an array of strings that contain the comma separated values from the CSS selector of the rule. The trim() operation would only remove spaces around a comma or the overall beginning or end of the CSS selector.
4. good catch. :-) Yes, I only try to sort extremely simple selectors: no spaces, no colons, no parentheses...
5. We don't need to put the space back. The stringify method imported form css has its own ideas of formatting.
6. I think the only reason to sort the selector is to make simple cases easier to compare visually by human being. This may not be a compelling reason. :-)

So if the rule started out with something like
.bloom-page.A5Portrait.outside-front-cover .marginBox, .bloom-page.A5Portrait.outside-back-cover .marginBox
The earlier code will remove the .marginBox, and this will sort the first selector. so we end up with
.A5Portrait.bloom-page.outside-front-cover, .bloom-page.A5Portrait.outside-back-cover
Assuming there is some reason to sort the classes in the first selector, why not those in the other?

Since it's not easy to find doc on the selectors property, I think it would be helpful to comment on that, and perhaps also give an example or two of what it is intended to do (and to leave alone).

Normally I would suggest unit tests for something like this, but if JohnH hasn't already done that you'd have to figure out how to set them up for the whole project, and it is a thing we only expect to use once, ourselves, and check the results by hand. So probably not worth it. I'm probably being too picky even trying to improve comments.

[StephenMcConnel]

Reviewable status: 4 of 5 files reviewed, 2 unresolved discussions (waiting on @JohnThomson)

---

custom-css-analysis-and-migration/migrateCssToAppearance.ts line 145 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

I think an overall purpose comment would be helpful here. Along the lines
We're looking for rules that affect the marginBox's size and position (top/left/height/width) because the new theme system controls these with variables that affect the .bloom-page margin instead of absolutely positioning the .marginBox, so setting its top or left will have no effect, and setting its height or width will probably do something unwanted. If such a rule also changes safe properties, we split it into two rules, one that has only the problem declarations and one that has only the safe ones. NOW EXPLAIN WHY THIS IS USEFUL. (Maybe some other code later looks for just the problem rules and does something with them?)
(Sorry, maybe it's obvious with more of the context than this review is showing me.)

Done.

---

custom-css-analysis-and-migration/migrateCssToAppearance.ts line 263 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

So if the rule started out with something like
.bloom-page.A5Portrait.outside-front-cover .marginBox, .bloom-page.A5Portrait.outside-back-cover .marginBox
The earlier code will remove the .marginBox, and this will sort the first selector. so we end up with
.A5Portrait.bloom-page.outside-front-cover, .bloom-page.A5Portrait.outside-back-cover
Assuming there is some reason to sort the classes in the first selector, why not those in the other?

Since it's not easy to find doc on the selectors property, I think it would be helpful to comment on that, and perhaps also give an example or two of what it is intended to do (and to leave alone).

Normally I would suggest unit tests for something like this, but if JohnH hasn't already done that you'd have to figure out how to set them up for the whole project, and it is a thing we only expect to use once, ourselves, and check the results by hand. So probably not worth it. I'm probably being too picky even trying to improve comments.

I found the documentation for the css package and added a comment pointing to it next to the import statement.
I enhanced the sort function to handle all of the selectors, not just the first one. I also added some comments to the function.