Regression: new way to set font-size and admin bar height cause problems in themes

[indigoxela]

Description of the bug

This change to rem for font size: font: normal 0.75rem "Lucida Grande", Verdana, sans-serif; breaks at least one contrib theme - bootstrap_lite.

Zulip chat: https://backdrop.zulipchat.com/#narrow/stream/218635-Backdrop/topic/Admin.20Bar.20issues.20with.201.2E28.2E0-preview

Fix: font: normal 12px "Lucida Grande", Verdana, sans-serif;

Reason: the bootstrap_lite theme sets font-size on html to 10px, but html is the base for rem calculation.

Questionable, for sure, but still we don't want to break it.

So, in this line https://github.com/backdrop/backdrop/blob/1.x/core/modules/admin_bar/css/admin_bar.css#L18 set the font size value back to 12px. That's all.

(The other rem based value (--admin-bar-height) is OK, as it's new and doesn't affect any existing calculations.)

Immeditate fix

Back to px based font size for backwards compatibility, fixed by https://github.com/backdrop/backdrop/pull/4741

Follow up fix

Derive admin-bar height from font size and account for possible other regressions.

[izmeez]

Setting font size back to 12px is a good idea to avoid problems with existing sites.

[bugfolder]

Questionable, for sure, but still we don't want to break it.

That font-size: 10px; in bootstrap.css is inherited from the D7 version of Bootstrap module. BSL was created to provide an upgrade path from the many D7 sites built on D7 Bootstrap module (currently 114K sites for both D7 and D8 versions).

[herbdool]

Actually I noticed the font-size: 10px in the CSS coming from the CDN. Out of the box, it's coming from https://stackpath.bootstrapcdn.com/bootstrap/3.4.1/css/less/scaffolding.less.

[herbdool]

I've added a PR. https://github.com/backdrop/backdrop/pull/4741

[izmeez]

I have tested locally with install of bootstrap_lite theme and confirmed the issue and confirmed the PR works as expected. The code change is simple.

I am still not sure that leaving --admin-bar-height as rem will work provide backward compatibility. Bootstrap lite using web developer tools there is a bit of overlap between the admin bar and the rest of the page. This may have been present all along, I haven't tested that. However, I'm finding on local testing that setting --admin-bar-height: 36px; is needed to avoid overlap in bootstrap lite, This may be an isolated issue with that theme.

[klonos]

Setting to WFM, based on @izmeez's comment above (although more testing by others welcome), and code is simple and looks good. Not marking it RTBC yet, as I'd like more people to chime in with regards to whether there is consensus or not when it comes to switching back to px.

[indigoxela]

I am still not sure that leaving --admin-bar-height as rem will work provide backward compatibility.

@izmeez there's no "backward" in this case, as this is a newly introduced property. It's now not based on the same unit, which might - in edge cases - lead to a slightly different value, than expected. That's unfortunate, but... :shrug:

BTW, I tested latest core with all of my contrib themes, and only one has some noticeable shift (when admin-bar is active), none of those contrib themes break in any way, though. So also consider it tested with Lateral, Monochrome, Pelerine, Scenery and Snazzy. And for those themes, the new property --admin-bar-height comes very handy.

[quicksketch]

@indigoxela I merged https://github.com/backdrop/backdrop/pull/4741 to avoid the major regression issue but I still have a question.

there's no "backward" in this case, as this is a newly introduced property. It's now not based on the same unit, which might - in edge cases - lead to a slightly different value, than expected. That's unfortunate, but...

I have the same thought as @izmeez here, where if rem is based on the root font size, using rem for the --admin-bar-height is going to be unreliable, since a smaller root font size will decrease that CSS variable, but the height of the admin bar will be unchanged, since it uses a fixed px font-size. The same thing could happen with a larger root font size.

The way I see this, I don't think any kind of variable is going to work quite like we want.

• rem: Only accurate if the root base font size is 16px.
• em: Only accurate if the variable is used within a container where the font-size matches the admin bar front size (12px currently).
• px: Always accurate regardless of the theme font size, but becomes incorrect if the #admin-bar font size is changed.

In short, I don't think the idea we had of creating a custom CSS property for --admin-bar-height is necessarily going to work. I think perhaps we should go back to what @indigoxela had provided previously which is just using em directly and not use a variable at all

[indigoxela]

I don't think the idea we had of creating a custom CSS property for --admin-bar-height is necessarily going to work. I think perhaps we should go back to what @indigoxela had provided previously which is just using em directly and not use a variable at all

If we remove the variable, I need to know in advance. Because two of my contrib themes recently adapted the var (unreleased). If it weren't available, I'd have to create something different to be able to cover both, pre 1.28 and post 1.28 - because the admin bar height slightly changed.

I have an idea, what we can do for https://github.com/backdrop/backdrop-issues/issues/6517. To make the admin-bar var calculation based on the font-size (dynamically, but in px). That would also work in themes, that do odd stuff with html font-size.

I don't think any kind of variable is going to work quite like we want.

IMO that's a thesis to refute. :wink:

[indigoxela]

I belief, something like this would work:

:root {
  --admin-bar-font-size: 12px;
  --admin-bar-height: calc(var(--admin-bar-font-size) * 2 + var(--admin-bar-font-size));
}

The admin bar height consists of the base font-size, actually line height plus some extra space for the icons, plus the padding-top and padding-bottom (0.5em each).

Later, if we decide to make font size configurable via UI, that --admin-bar-font-size value would come from the setting.

[quicksketch]

Conceptually, it seems like it could work @indigoxela. We would switch to px because they're the only reliable unit regardless of context, but we calculate the height off the font size which is also a variable. I like that it sets us up for a UI setting too possibly. And themes would even have the option of overriding that custom property in the mean time.

My only question is what would be needed if the margins/padding/line-height don't work out so neatly. Is there a situation where we couldn't determine the height from font-size alone? I instinct tells me that it should work, since everything is in ems, you should be able to add up the number of ems to get the multiplier.

[indigoxela]

Is there a situation where we couldn't determine the height from font-size alone?

We'll figure out. :wink: Currently, there are still some open questions re line-height and the font in use, and also line-height in "iconized" items vs. line-height in plain items, when it comes to dynamic values. That all will need thorough testing - that's why I'd suggest to discuss in the related (sort of follow-up) issue. In this issue here we just fixed a regression. Not in a perfect way, because of different units in use, but in a way that minimizes problems.

Fixing existing CSS while considering backwards compatibility is quite time consuming and 1.28 is around the corner. :wink:

Or is there anything important to fix here? (Asking because of the needs-more-feedback label...)

[izmeez]

I like the concept, I'm just having trouble following the math in the example. I don't know if that matters at this point.

[indigoxela]

I'm just having trouble following the math in the example...

@izmeez probably because it's not elaborated, yet. :wink: It's just a quick example for a possible concept.

[herbdool]

Regarding:

:root {
  --admin-bar-font-size: 12px;
  --admin-bar-height: calc(var(--admin-bar-font-size) * 2 + var(--admin-bar-font-size));
}

I tested and it works alright for me. This might be another regression that we should get get into this release?

Previously the admin bar height was 3 times the font size (33px to 11px) so this is still keeping in line with that.

I compared the admin bar height (with this "patch") between the admin bar on Seven vs a front end theme. I changed the admin bar background to be a different color to the .admin-bar body top border so I could see more easily. By trial and error it seems that 2.2 times is the limit before the top border on body starts peaking out. Not sure if that matters.

:root {
  --admin-bar-font-size: 12px;
  --admin-bar-height: calc(var(--admin-bar-font-size) + 2.2 * var(--admin-bar-font-size));
}

[kiamlaluno]

I apologize if this is a silly question: Why is calc(var(--admin-bar-font-size) + 2.2 * var(--admin-bar-font-size)) used instead of calc(3.2 * var(--admin-bar-font-size))?

[indigoxela]

@herbdool many thanks for chiming in. However, IMO this isn't actually the issue to discuss the additional property (and deriving one from the other). We simply do not have the time to elaborate and test this properly. I already spotted some interesting edge cases with my example approach, which need a closer look and more testing.

Your (already merged) PR fixed the immediate problem, particularly for bootstrap_lite, possibly for other themes, too. That's why it was important to merge.

[indigoxela]

I apologize if this is a silly question: Why is ...

There are no silly questions. :wink: My sample code was only to quickly demonstrate, what I had in mind. The final code will look different. And is hopefully easier to read and understand.

[kiamlaluno]

I was referring to @herbdool comment posted right before mine. 😉 I apologize I was not clear.

[jenlampton]

Do we already have another issue where we are working on a switch to variables? If not, should we make one so this one can be closed?

[quicksketch]

@jenlampton There is already a variable, but it doesn't work correctly. We fixed the critical issue at hand that was causing the admin bar to display at weird sizes from contrib themes, but the code for letting contrib find the admin bar size is still not working reliably. I merged the first PR just because it was a pretty big issue, now there's a less-critical part of the same root issue that still needs fixing.

[izmeez]

@quicksketch Reading your last comment helps to clarify things. So if I understand the regression HAS been fixed and maybe this issue should be closed. We do have another issue #6517 to allow the admin bar font size to be changed and maybe it is where the discussion of how best to use variables can be moved or another issue can be opened. It would be nice to close this issue and refocus in a separate issue.

[indigoxela]

Hm, diving into new knowledge: the "em square" and the actual bounding box for text, which is based on the font in use, and different with every font... There's no way to set or even determine the actual "line height" (bounding box for text) via CSS.

However, IMO we'll be able to find a good compromise, based on lots of testing and feedback. We won't be able to find the perfect solution. There will always be font-size/font-family combos, which cause the body top offset (border) and actual admin-bar height to differ by one, possibly two pixels. Especially, when things get dynamic, for example if/when we provide an admin setting for the admin-bar font size.

[indigoxela]

Here we go, the next PR is available for testing and review.

Because of my learning curve re em-box, we're now at a "magic number" --admin-bar-height: calc(var(--admin-bar-font-size) * 3.1); Done by trial and error with different fonts (open sans, verdana, arial) and different font-sizes (between 11px and 16px). This "magic 3.1" seems to be OK with most combinations.

Additionally I reduced the line height to 1.5 to prevent multi-line menu items to look like two items (confusing). Instead I increased top/bottom padding. Also because padding is easier to control via CSS, compared to the volatile text bounding box. The less we depend on line-height for layout, the better. :grimacing:

[izmeez]

@indigoxela Thank you for pushing this forward and changing the title. Is it also possible to remove reference to the first PR that was used for the initial fix? This is also new territory for me and looking at the proposed PR and diff I wonder if it may be possible to factor this out more, similar to the examples in https://css-tricks.com/a-complete-guide-to-calc-in-css/ ? Sorry I don't have something more concrete to offer.

[indigoxela]

Is it also possible to remove reference to the first PR that was used for the initial fix?

@izmeez I don't think, that's possible. And I'm not even sure, if that would be a good idea, anyway. Our future self needs both references to understand the steps we made here.

I wonder if it may be possible to factor this out more, similar to the examples in...

I have no whatsoever idea, what result you're after, or even what you mean. :laughing: Did you read my previous comment, which tried to explain, why this calculation is done the way it is done, and what needs to be tested?

[izmeez]

Yes, I did see the comment and testing required. I figured the only way to do this was locally with a patch and I have started to do that. The patch applies without difficulty and testing different font sizes from minuscule to gigantic works. I still have to do more tests with different fonts and themes.

[izmeez]

So far I have also tested with bootstrap_lite 1.4.4 and tatsu 2.0.0 both without adding reference to --admin-bar-height and they appear to work fine with different font sizes.

I have also tested with a custom theme where --admin-bar-height has been added on previous tests and now does not seem to make any difference, as though it's not needed. I'm going to have to experiment more with that.

I also need to test different fonts.

[izmeez]

The PR also works with several fonts and different sizes that I have tested on the different themes.

The only issue I have encountered, that may be expected, is with a narrow screen and a larger font size such as 16px showing overlap with any of the themes. I haven't actually tested this with a mobile device. I can't imagine using such a large font on a mobile device so it may be out of scope.

[izmeez]

With a narrow screen a font size of 14px works as a "maximum" without causing overlap although this may vary with the choice of font.

[klonos]

Thank you for persisting on this @indigoxela and thanks for testing this so extensively @izmeez 🙏🏼

I can't imagine using such a large font on a mobile device so it may be out of scope.

Yup, out of scope here. I've filed #6558 and suggested a possible solution for that.

[indigoxela]

@izmeez thank you so much for your thorough testing. :pray:

[izmeez]

I've now had a chance to do more testing with a custom theme and a different color background for the admin bar. I noticed a tiny black horizontal line along the bottom of the admin bar for font sizes less than 15px. When I changed the magic number to 3.05 there is no black line at font sizes from 8px to 18px.

[izmeez]

Personally, I like the default of 12px. Although for people who prefer less change 11px default could be used. It seems like this solution works without requiring any changes to the themes. I'm in favor of it and interested to see what others think.

[indigoxela]

When I changed the magic number to 3.05 there is no black line at font sizes from 8px to 18px.

@izmeez Confirmed, 3.05 works even better. :+1: (PR updated)

[izmeez]

I'm glad your testing confirmed that 3.05 works better. I was still a bit puzzled about the behaviour I was seeing and realize it may be related to the inclusion of the simplei module. All the same, this seems to be the best solution. In the PR diff it looks like that is the only thing that has changed so I consider this RTBC and will try to change the labels. Thank you.

[klonos]

Purely from a code perspective (coding standards etc.), the PR is RTBC ...I'm hesitant to mark the issue as such though, since I have no idea if what we are doing is the right approach or not. I simply don't consider myself experienced enough on the matter.