-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Regression: new way to set font-size and admin bar height cause problems in themes #6526
Regression: new way to set font-size and admin bar height cause problems in themes #6526
Comments
Setting font size back to |
That |
Actually I noticed the |
I've added a PR. backdrop/backdrop#4741 |
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 |
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 |
@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... 🤷 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. |
@indigoxela I merged backdrop/backdrop#4741 to avoid the major regression issue but I still have a question.
I have the same thought as @izmeez here, where if The way I see this, I don't think any kind of variable is going to work quite like we want.
In short, I don't think the idea we had of creating a custom CSS property for |
If we remove the variable, I need to know in advance. Because two of my contrib themes recently adapted the var (unreleased). I have an idea, what we can do for #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.
IMO that's a thesis to refute. 😉 |
I belief, something like this would work:
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. |
Conceptually, it seems like it could work @indigoxela. We would switch to 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. |
We'll figure out. 😉 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. Fixing existing CSS while considering backwards compatibility is quite time consuming and 1.28 is around the corner. 😉 Or is there anything important to fix here? (Asking because of the needs-more-feedback label...) |
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. |
@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. |
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... 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. |
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" 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. 😬 |
@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. |
@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 have no whatsoever idea, what result you're after, or even what you mean. 😆 |
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. |
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. |
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. |
Thank you for persisting on this @indigoxela and thanks for testing this so extensively @izmeez 🙏🏼
Yup, out of scope here. I've filed #6558 and suggested a possible solution for that. |
@izmeez thank you so much for your thorough testing. 🙏 |
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 |
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. |
@izmeez Confirmed, 3.05 works even better. 👍 (PR updated) |
I'm glad your testing confirmed that |
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. |
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 backdrop/backdrop#4741
Follow up fix
Derive admin-bar height from font size and account for possible other regressions.
The text was updated successfully, but these errors were encountered: