Skip to content
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

[BC] Decide on best way to make improvements to the CSS for core without breaking existing sites #4167

Closed
stpaultim opened this issue Oct 28, 2019 · 76 comments

Comments

@stpaultim
Copy link
Member

stpaultim commented Oct 28, 2019

UPDATED: August 27, 2020 by @stpaultim

Possible solutions:

Description of the need

A new user that does not have the time or skills to create their own theme or sub-theme should be able to build a solid, reliable, and functional site without need to install a contrib theme. However, there are a number of small problems with Basis that make that difficult, some of which have been fixed or are in the process of being fixed.

Most Backdrop developers are used to creating custom themes for sub-themes for their sites and routinely deal with these issues during that process OR have created their own sub-theme templates with fixes in place.

It is my concern that as we continue to tweak Basis and/or fix bugs, that we might end up breaking or making unexpected changes to existing sites that are using Basis as their only theme. Since we are at least 3 years away from Backdrop CMS v 2.x, I would like to decide on a best course of action for providing a robust and flexible theme (for beginners with limited theme building skills) in core that can safely be used "out of the box" and will accommodate a variety of uses.

Possible solutions that have been considered

  1. We could go on making updates to the current version of Basis at some risk of minor breaks to sites that are already build using Basis out of the box without any sub-theme or changes.
  2. We could introduce Basis v 2.x into core as an experimental theme, making it clear that it is under development until 1.x-1.16.x (or whichever version we determine)
  3. [BC] Decide on best way to make improvements to the CSS for core without breaking existing sites #4167 (comment) We could develop it as a contrib theme, but with an expectation that we eventually moving it into core at a later date. We already have https://github.com/backdrop-contrib/basis_contrib
  4. Issue Create option to add a supplemental stylesheet to Basis - to safely make css updates #4512 - Add a "supplemental stylesheet" to Basis (or default theme) that includes new css or overrides that might cause problems with existing sites. This "supplemental stylesheet" would only be used if a config option was checked, the default would have this config off, but on new sites it the install profiler would turn it on. [Added 10/28/19 - inspired by discussion here: Should we set default margins on 'p' tag in Basis? #4166]
  5. [BC] Decide on best way to make improvements to the CSS for core without breaking existing sites #4167 (comment) Create and add a sub-theme of basis to core called something like "basis_edge" or "basis_current" that includes all the changes. This new sub-theme would be the default theme on new sites, but old sites would continue to rely on the existing version of basis.
  6. [BC] Decide on best way to make improvements to the CSS for core without breaking existing sites #4167 (comment) Create a new default core theme, keeping Basis as it is but deprecated, and try to fix as many of the issues as possible in this new core theme. (This is very similar to option 2. It's not clear if either of these plans offers an avenue for ongoing improvements in CSS).
  7. Issue Add support for supplemental CSS selectors #4782: Add support for supplemental CSS selectors. All new CSS or CSS overrides that might cause problems with existing sites would be prefixed with a class matching the version of core where it was introduced. That class would only be added to sites installed on or after that core version.
  8. Issue Add a new core theme #5175 : A new theme to core - This may be the most ambitious option. The idea is that if we cannot find another way to fix CSS issues, we could simply create a new theme for core that fixes existing problems, is updated to give Backdrop core a new look, and possible contains within itself a system that allows updates and fixes safely.

Some Issues blocked by this decision

Additional information

This idea was inspired by issues such as:

[META] Issues with Basis
#4094

Should we set default margins on 'p' tag in Basis?
#4166

Remove the hover color from header account links in Basis
#3748

Basis & Flexible Layout Templates: Remove white space between two hero blocks placed next to each other.
#4095

I really don't know if this is a good idea or if it's necessary to accomplish the goals I've outlined, but I wanted to put it out there for discussion.

@stpaultim
Copy link
Member Author

stpaultim commented Oct 29, 2019

In another issue, @olafgrabienski said he would like to see:

Provide an option to add custom CSS in Backdrop core.

I assume he is talking about something like CSS injector (Drupal module). But, this got me thinking about whether or not it would be possible to create a "supplemental stylesheet" for Basis that gets added to new site automatically or that existing sites can add at anytime.

@klonos - is there an issue for this already? I could not find one (I'll create one if there is any positive feedback to this idea).

The idea would be to use this supplemental style sheet to add or override existing CSS in Basis without blindly applying it to existing sites. This had not occurred to me, but now that I think about it, it should be possible. I'm adding it as an option in the original issue description.

@oadaeh
Copy link

oadaeh commented Oct 29, 2019

CSS injector is not just a Drupal module: https://backdropcms.org/project/css_injector

@stpaultim
Copy link
Member Author

We talked about this briefly today between meetings (discussion did not get recorded) and there seemed to be some interest in option #4. That we could add a supplemental stylesheet that overrides other stylesheets. This stylesheet could be applied to all new sites and old sites to enable it through theme settings.

No decision was made, just a short discussion. I'd love more opinions on this.

@stpaultim stpaultim changed the title New Core Theme - Basis v 2.0 or something altogether new Decide on best way to make improvements to the CSS for Basis without breaking existing sites Oct 31, 2019
@stpaultim
Copy link
Member Author

stpaultim commented Nov 1, 2019

@jenlampton - posted this idea in #4166

Just brainstorming here... What if we reserved all CSS changes like this for 
minor versions only, and with each minor version added an additional 
stylesheet to Basis. Something like /core/themes/basis/css/updates/14.css. 
When you install Backdrop, we save that minor version somewhere secretly, 
and then your site would get only the stylesheets that were in core when 
you first installed.

How would we make this work... Well, we could add the CSS in preprocess_page()... if we 
recorded the minor version of Backdrop that were first installed somewhere, we could 
pull that out here to test. Maybe something like...

function basis_preprocess_page(&$variables) {
  $path = backdrop_get_path('theme', 'basis');
  $install_version = config_get('system.core', 'installed_minor_version');
  for ($i = 1; $i <= $install_version; $i++) {
    $filename = $path .'/css/updates/' . $i . '.css';
    if (file_exists($filename)) {
      backdrop_add_css($filename);
    }
  }
  ...
Then when we get to Backdrop 2.0, we merge in all the updates form the 
1.x cycle, and start over with an empty updates directory.

@ghost
Copy link

ghost commented Nov 16, 2019

Are you aware of https://github.com/backdrop-contrib/basis_contrib ? You could move development there, then merge changes into core once they're working and approved.

@jenlampton
Copy link
Member

jenlampton commented Dec 5, 2019

I have an updated idea similar to #4167 (comment) ... but doesn't involve storing an initial version.

  • every minor release that includes breaking CSS changes has those added into a style sheet named 14.css or 15.css etc.
  • For every style sheet, a checkbox is added into Basis theme settings asking whether changes from this stylesheet should be included or not.
    • for new sites, all available checkboxes are checked
    • for existing sites, no new checkbox is checked automatically
  • When Backdrop 2.x is released, all css changes from these additional stylesheets are merged into a more appropriate location, and the stylesheet numbering starts over.

@ghost
Copy link

ghost commented Dec 6, 2019

WordPress has child themes like we have sub-themes: https://developer.wordpress.org/themes/advanced-topics/child-themes/
It'd be interesting to see if we can find out how they address this issue of not breaking people's customisations when parent themes are updated...

@ghost
Copy link

ghost commented Dec 6, 2019

I'm trying to think of ways to avoid checkboxes and settings and additional CSS files, etc. That all seems overly complicated to me.

What about simply informing people that making a sub-theme of a core theme (e.g. Basis) could result in breaking changes on update. If they don't want this, all they need to do is copy the base theme into their themes folder (maybe we can even provide a UI button that does this for them...?). That copy will override core's one and future core updates won't affect it. If they ever want to go back to using the latest version of the base theme, they just need to delete the copy and clear caches. If they want some updates but not all, they can add them manually to their copy of the base theme.

This seems like a much simpler option, and uses systems already in-place in Backdrop (theme overrides, etc.).

@jenlampton
Copy link
Member

jenlampton commented Dec 6, 2019

What about simply informing people that making a sub-theme of a core theme (e.g. Basis) could result in breaking changes on update.

Because we don't want to scare anyone away from using Basis as a base theme, and this sounds scary.

if they don't want this, all they need to do is copy the base theme into their themes folder

This is not a good practice. A copied theme wouldn't get any updates to Basis -- including possible security updates -- when updates come out for core. The whole reason there are base themes (and that we wanted Basis to be better as a base theme) is so that we can avoid this practice.

@quicksketch
Copy link
Member

Another sort of suggestion we could use: Make an out of box subtheme of Basis called basis_latest or basis_edge (or something) and use it as the default theme. Then we only make changes to basis_latest and never to basis itself, which acts as a stable version.

Alternative but very similar: we could rename basis to basis_stable and then make a new basis theme that sub-themes basis_stable. All changes are made in the basis theme while basis_stable stays frozen.

Note with this approach you'd probably eventually copy almost every CSS file.

This is the approach D8 used with their default theming, although the intent for them was mostly around template files, not CSS.

I am not sure I would recommend this approach honestly, but it's another option. I don't like the added complexity of everything being a subtheme out of the box. That's too much complexity to introduce at what is usually the most accessible layer of Backdrop for new developers, IMO.

@stpaultim
Copy link
Member Author

What about simply informing people that making a sub-theme of a core theme (e.g. Basis) could result in breaking changes on update.

Isn't it too late to issue such a warning to all the sites that might already be using Basis? I'm not sure if we want to risk breaking sites that were built before such a warning was issued AND the comments raised by @jenlampton.


As we work on the "Ready to Wear" initiative, we may find the need to fix some of the CSS bugs/omissions we have in Basis. I'd like see if we can't make some progress on this issue.

While, I'm not seeing any consensus on how to proceed, in my opinion, some version of option 4 (see issue summary) seems to be our best bet so far.

@klonos
Copy link
Member

klonos commented Aug 1, 2020

If I'm not mistaken, if a contrib or custom module/theme has the same name as a core one, then the contrib/custom one takes priority. So how about changing our documentation to advise the following when subtheming core-provided themes:

We generally avoid introducing breaking changes to the Basis theme, and any core theme in general, but there may be situations where we may absolutely need to (we only had to introduce a single such change over the 6 years of the Backdrop CMS release history). In such cases you may need to tweak any subthemes you have created, to account for the breaking change, and make sure that they still look/work as expected. To avoid this, you have the following option:

  1. Copy the entire /core/themes/basis folder under the root /themes directory, so that the end folder is /themes/basis.
  2. Create your subtheme of Basis as per usual, by specifying base theme = basis in its .info file.

Since any theme placed under /themes takes priority over any core theme, your subtheme will always have /themes/basis as its parent instead of /core/themes/basis. You can now either update your theme at your own pace (independently of any Backdrop CMS core updates), or never update it at all if you so choose.

We may also add the following:

If you have realized that a core update has broken your subtheme, then follow these steps to quickly fix things:

  1. Download a copy of the previous version of Backdrop CMS, and extract it.
  2. Copy the entire /core/themes/basis folder from the older version of Backdrop, to the root /themes directory of the updated site, so that the end folder is /themes/basis.
  3. Clear caches.

Any security risks that come with this decision to use an older, specific version of a core theme should also be communicated.

@jenlampton
Copy link
Member

jenlampton commented Aug 1, 2020 via email

@stpaultim
Copy link
Member Author

stpaultim commented Aug 1, 2020

If I'm not mistaken, if a contrib or custom module/theme has the same name as a core one

@klonos - Are you just throwing that out there, or do you prefer this solution over something like #-4 (supplemental style sheets)?


NOTE: I opened #4512 to start working on a simple PR based upon #-4. I don't know if this is the direction we want to go, but I think that creating a simple PR for this option, might help move this issue forward and better evaluate if it's the best option. I'm hoping to work on this myself, but welcome anyone to get started.

@jlfranklin
Copy link
Member

Alternative but very similar: we could rename basis to basis_stable and then make a new basis theme that sub-themes basis_stable. All changes are made in the basis theme while basis_stable stays frozen.

That would mean that someone who has already sub-themed off of Basis now has a sub-theme of a sub-theme, and a surprise coming. They may not notice the change until something breaks, at which point they have to rebase off of basis_stable and check what changes they lost from basis. That isn't backwards compatible.

I think a better plan would be to leave basis as is, and only apply security updates as needed. Then create a new theme, bland and blank (think: the old Zen theme), with a new, different name that doesn't start with "B". Foundation. Pedestal. Cornerstone. Vasi. Add some notes to Basis that it is deprecated, but still supported, and will go away in BD2.0. Finally, create two or three full-color tutorial sub-themes off this new one to show how it's done, and make one of those the new default theme.

New users can pick a full-color theme, advanced users can sub-theme off bland-and-blank, and existing basis-derived themes still work the same, warts and all.

Basis may have issues, but they're well known issues. Rearranging things just makes more issues.

@stpaultim
Copy link
Member Author

stpaultim commented Aug 2, 2020

@jlfranklin - Thanks for your ideas. I think your suggestion is very similar to option 2 in the original issue description, but I added it as option 6 (possibly redundant).

I have two follow up questions for you?

  1. With your suggestion to replace Basis with a new default theme, won't we run into the same problem in the future if we need to make css changes? Your suggestion would allow us to rework the theme and make significant improvements, but I'm not sure it addresses the question of how to safely make gradual ongoing css improvements in whichever default theme we are using. Am I missing something?

  2. What do you think about the idea of introducing changes to the default theme with supplemental style sheets (something like option 4 in the issue summary)?

@jlfranklin
Copy link
Member

jlfranklin commented Aug 2, 2020

  1. With your suggestion to replace Basis with a new default theme, won't we run into the same problem in the future if we need to make CSS changes?

Not if it's done with this problem in mind.

I would discourage users from creating sub-themes off a full-color theme, so any future changes there would only affect users of said full-color theme. That should make changes that might break a sub-theme safe to apply here.

On the other hand, bland-and-blank needs to be simple and robust, establishing theming structure, nomenclature, and setting reasonable defaults. Future changes should be more conservative: security updates, clearly backwards-compatible bug fixes, and new selectors to support new features in Backdrop. Treat bland-and-blank like any other API, because it is an API. If there is a question, default to applying a change to the full-color themes with comments documenting why it wasn't applied to the parent theme.

  1. What do you think about the idea of introducing changes to the default theme with supplemental style sheets (something like option 4 in the issue summary)?

That effectively creates two themes, Basis and Basis++. It also creates (and implicitly blesses by the core team) a second sub-theming mechanism: the use of supplemental style sheets. The full-color themes better demonstrate how sub-themes are intended to be done.

@jenlampton
Copy link
Member

jenlampton commented Aug 2, 2020

That effectively creates two themes, Basis and Basis++.

Most themes that you buy from non-drupal/backdrop sources include lots of CSS that people can choose to include or exclude, depending on what they need for their site. It's a good way to provide options.

It also creates (and implicitly blesses by the core team) a second sub-theming mechanism: the use of supplemental style sheets.

Many contrib themes already set a variable, then include/exclude a stylesheet based on that variable. I don't see a problem with core adopting a pattern that's been proven successful.

I also don't see any parallels between sub-themes and supplemental stylesheets, and I don't think anyone would get them confused. Both are great tools for front-end developers! :)

@klonos
Copy link
Member

klonos commented Aug 3, 2020

@klonos - Are you just throwing that out there, or do you prefer this solution over something like #-4 (supplemental style sheets)?

I was throwing it for consideration. It's what we are suggesting to digital agencies that work with https://github.com/govCMS/ui-kit-theme / https://github.com/govCMS/govcms8_uikit_starter. Asking customers to optionally copy the theme means less headaches for both parties, but the customer acknowledges that they are opting for less maintenance overhead (which saves them $$$) at the cost of potential security risk. This gives them the freedom to update their themes at their own pace, while we keep the platform/distro updated on their behalf. #SeparationOfResponsibilities++

As stressed earlier, this is not a one-way solution. There's customers that are actively monitoring changes and are updating their subthemes accordingly, and there are others that have a build-and-forget mindset (until the next time they redesign their site). The latter usually prefer to not have their site break with any updates we do to the core/distro.

I would like to remind everyone this (emphasis mine): https://backdropcms.org/philosophy

Backdrop will attempt to keep API change to a minimum in order for contributed code to be maintained easily, and for existing sites to be updated affordably. Every big change that can't be made backwards compatible will need to be carefully considered, and measured against Backdrop's other principles.

I interpret this as that we'll occasionally need to make breaking changes (which in this case happen every once in 5-6 years, and do not affect every site). In those (rare) cases, I feel that it is fair to consider it the responsibility of site owners to keep up and update their sites (and themes). Although I like #4512 as a solution, it is adding to complexity and overhead for us, when it should have been on site owners. I'm saying this while you all know how passionate I am about BC ❤️

@jenlampton I feel that copying the core theme temporarily whenever there may be a breaking change is not such a bad option for end users. We just need to stress that although possible, it is not the recommended way - the recommended way is to keep up with updates.

My point is that it would be nice to have something like #4512; I fully support it - especially since it would help us long-term. Having said that though, it will take a considerable amount of work to support it, it will add complexity/overhead on us. Copying a core theme on the other hand is something that works now, and it's simple for end users to do. If we wanted, we could add a tiny bit of logic to warn that an outdated theme is being used (on the site status report and/or the "Available updates" page), as a reminder that they need to update as soon as possible.

@klonos
Copy link
Member

klonos commented Mar 22, 2021

I was under the impression that if you use the same names for .css files, they completely override things in the order of core -> contrib/custom + base theme -> subtheme. Right?

@jenlampton
Copy link
Member

Yes, that's true. But most theme's don't override core stylesheets with their own copies that also have the same name. Most themes have their own style.css or custom.css that adds CSS to what's coming out of core. That's not possible with template files, so you know that if someone is using their own template they are not using what's in core. With CSS they could also be using what's in core.

Putting CSS changes only into a starterkit theme could be a solution for us, but it would mean those changes are only opt-in, and would only affect new installs if they chose to create a custom theme, and if that custom theme started with the starterkit. No bugfixes for Basis :(

@klonos
Copy link
Member

klonos commented Mar 22, 2021

No bugfixes for Basis :(

Yeah, I didn't say that a starter theme is THE solution for us - just A possible solution. It does have its downsides, but:

  • no breakages for existing sites that use Basis (we basically don't touch it - ever ...unless security issues)
  • new sites get the all the bug fixes that will from now on be included in the starter theme (which we will be free to change at will)

It just means that there will need to be a mentality shift: instead of subtheming the default theme that ships with core, the recommended workflow would be to use a command-line tool or a UI equivalent, to generate a new theme, off of the starter theme. The resulting theme is a "stand-alone" theme instead of a subtheme.

I am not advocating for this, nor saying it is a good fit for Backdrop - just mentioning it in the spirit of considering all options.

@jenlampton
Copy link
Member

jenlampton commented Mar 22, 2021

new sites get the all the bug fixes that will from now on be included in the starter theme

This should really be "a tiny percentage of new sites get..." but It is an interesting idea.

My preference would be to stick with one of these other solutions that solves all our problems and doesn't increase the complexity. I prefer having a real, usable theme (Basis) as our starterkit, but I know the community loves starterkits, so I'm curious to see how everyone would feel about this by 2.x.

@klonos
Copy link
Member

klonos commented Apr 24, 2021

D9 now ships with a starterkit theme generator See: https://www.drupal.org/node/3206389

New starterkit theme generator has been added as an experimental new tool in core. The starterkit theme generator allows developers to create a starting point for their theme. The generated theme has similar markup and CSS previously provided by Classy base theme including common CSS classes and markup. Instead of inheriting the markup and CSS from a base theme, the starterkit theme generator copies the defaults into a new theme. Tooling is provided as part of the included Drupal command line interface for automating this:

php core/scripts/drupal generate-theme mytheme

As a result of this change, front-end developers can expect more frequent updates to the default markup and CSS shipped as part of Drupal core.

@jenlampton jenlampton changed the title Decide on best way to make improvements to the CSS for core without breaking existing sites [BC] Decide on best way to make improvements to the CSS for core without breaking existing sites Aug 16, 2021
@stpaultim
Copy link
Member Author

At the last Backdrop LIVE we had a full session on the topic of adding a new theme to core (#5175). I created this issue at least in part, because of my own perception that this issue has reached a logjam and a lack of any sense that it is likely to move foward in the near future. I was a little surprised to hear that @jenlampton thinks we are much closer to a solution than I thought.

I'm looking forward to hearing more about that solution.

@philsward
Copy link

philsward commented Nov 17, 2021

The discussion in my mind all revolves around "what is the end result". If it is a major change in backend structure then the "fix" should not be applied to the existing theme and instead treated as a feature request on a new theme because this type of change may affect the output of an existing site. If it's minor UI tweaks or fixes, it should be applied to the existing theme and pushed through normal releases. It all comes down to what the fix or change actually is. Not every CSS tweak will cause a meltdown for the user but other CSS tweaks might lead to unforeseen catastrophic results.

What we need is a policy created around: "what constitutes a major change vs a minor change in a theme". I don't know what the criteria would be off hand, but if an issues goes through a certain set of checks one way or the other, it determines where the issue lies: Existing theme or Future theme.

Since there doesn't appear to be a formal policy around this topic, we're stuck here trying to figure out whether updates should be applied or not applied. Draw a line in the sand and specify exactly what the approach is. Like @stpaultim stated, Basis has problems and we can't figure out if we fix them or not because there's no guidelines to help determine whether the fix is safe or needs to be addressed in a future theme.

Ideally, once a theme is promoted as "stable" and put into core, a new theme repo should immediately be created where all "breaking" requests/issues are sent. For example, converting to using CSS Grid VS whatever is used in the old theme.

There are certainly a lot of suggestions around this, some more complex than others but at the end of the day, the end result of what is being accomplished is what should determine whether an update is pushed to the stable theme or the future theme and we need a set of guidelines that help point in the right direction.

@stpaultim
Copy link
Member Author

I'm making this a feature request, so that I can vote on it in our feature survey.

My colleagues and I were just working on a CSS issue for Bartik, but I'm really not sure that we can safely merge it, given this concern. There are quite a few outstanding CSS issues that I am avoiding until this is resolved.

Just a reminder, that we have several viable solutions to solving this problem. What we lack is consensus on any one solution. It may be time to escalate this issue to the PMC.

@stpaultim
Copy link
Member Author

Putting this on the agenda for the next Backdrop LIVE to see if we can break the log jam.
https://events.backdropcms.org/discussions/backdrop-live-april-2024/how-to-make-css-changes-to-core

@stpaultim
Copy link
Member Author

It's too late to get this going for 1.29. But, solving this before 1.30 would be nice.

@stpaultim
Copy link
Member Author

We seem to have settled on this solution:
#4782

@herbdool
Copy link

I think we can close this finally, now that #4782 is merged.

@stpaultim
Copy link
Member Author

@herbdool THANK YOU!

I'm very happy to see this issue go away!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests