backdrop-issues icon indicating copy to clipboard operation
backdrop-issues copied to clipboard

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

Open stpaultim opened this issue 6 years ago • 73 comments

UPDATED: August 27, 2020 by @stpaultim

Possible solutions:

  • issue #4512 - Supplemental stylesheets
  • issue #4782 - Supplemental CSS selectors

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. https://github.com/backdrop/backdrop-issues/issues/4167#issuecomment-554654706 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 #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: #4166]
  5. https://github.com/backdrop/backdrop-issues/issues/4167#issuecomment-666914785 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. https://github.com/backdrop/backdrop-issues/issues/4167#issuecomment-667600343 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 #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 #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

  • https://github.com/backdrop/backdrop-issues/issues/2797
  • https://github.com/backdrop/backdrop-issues/issues/4166
  • https://github.com/backdrop/backdrop-issues/issues/5769

Additional information

This idea was inspired by issues such as:

[META] Issues with Basis https://github.com/backdrop/backdrop-issues/issues/4094

Should we set default margins on 'p' tag in Basis? https://github.com/backdrop/backdrop-issues/issues/4166

Remove the hover color from header account links in Basis https://github.com/backdrop/backdrop-issues/issues/3748

Basis & Flexible Layout Templates: Remove white space between two hero blocks placed next to each other. https://github.com/backdrop/backdrop-issues/issues/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 avatar Oct 28 '19 00:10 stpaultim

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.

stpaultim avatar Oct 29 '19 00:10 stpaultim

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

oadaeh avatar Oct 29 '19 02:10 oadaeh

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 avatar Oct 31 '19 23:10 stpaultim

@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.

stpaultim avatar Nov 01 '19 00:11 stpaultim

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.

ghost avatar Nov 16 '19 16:11 ghost

I have an updated idea similar to https://github.com/backdrop/backdrop-issues/issues/4167#issuecomment-548618065 ... 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.

jenlampton avatar Dec 05 '19 22:12 jenlampton

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 avatar Dec 06 '19 07:12 ghost

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.).

ghost avatar Dec 06 '19 07:12 ghost

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.

jenlampton avatar Dec 06 '19 18:12 jenlampton

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.

quicksketch avatar Jul 31 '20 04:07 quicksketch

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.

stpaultim avatar Aug 01 '20 05:08 stpaultim

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.

klonos avatar Aug 01 '20 06:08 klonos

@klonos I don't like the recommendation of copying a core theme (or module) into the contrib location, because people will be very likely not to update it (even if they wanted to) since we would have no mechanism to remind them, like we do for core and contrib.

I don't like the suggestion of adding sub-themes in core mainly because they will show up in the UI and confuse end-users. But it also seems like we'd only be kicking the can down the road. We'll eventually face the problem where the sub-theme needs to he changed. To prevent breaking sites that are using the sub-theme, would we add a sub-sub theme, and then a sub-sub-sub theme? This also seems like a lot of overhead for what seems to be only a few lines of css for each change...

On Fri, Jul 31, 2020, 11:33 PM Gregory Netsas [email protected] wrote:

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 https://api.backdropcms.org/documentation/creating-sub-themes 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 need to. In such cases you may then need to tweak any subthemes you have created to 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.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/backdrop/backdrop-issues/issues/4167#issuecomment-667482383, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADBER5X7WSCPN5X5MRLSD3R6OZLTANCNFSM4JFUG4IQ .

jenlampton avatar Aug 01 '20 17:08 jenlampton

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.

stpaultim avatar Aug 01 '20 22:08 stpaultim

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.

jlfranklin avatar Aug 01 '20 23:08 jlfranklin

@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)?

stpaultim avatar Aug 02 '20 17:08 stpaultim

  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.

jlfranklin avatar Aug 02 '20 20:08 jlfranklin

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! :)

jenlampton avatar Aug 02 '20 20:08 jenlampton

@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 avatar Aug 03 '20 02:08 klonos

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.

...as a PoC, I have filed this 6-liner PR: https://github.com/backdrop/backdrop/pull/3209

Without changing any code in the "Appearance" page, things look like this (a copy of Basis from 1.14.4 placed under /themes/basis in a 1.16.2 installation):

image

...and here's how the "Available updates" page looks in the same scenario, also without any changes in that:

image

klonos avatar Aug 03 '20 12:08 klonos

While I like option 4 (Add an optional "supplemental stylesheet" to Basis), I'm not sure if there's really need for such an action, if we limit core theme changes to security and other very important fixes, put extra time in looking for solutions which don't break sites, and clearly announce core theme changes in the release notes.

olafgrabienski avatar Aug 03 '20 15:08 olafgrabienski

...put extra time in looking for solutions which don't break sites.

That's the problem. In cases like this specific breakage that triggered this issue here, we did not know that things broke till only after the release 🤔 ...no matter how careful we are, we're only humans, so there's always a chance that we'll miss things.

klonos avatar Aug 03 '20 15:08 klonos

this specific breakage that triggered this issue here

@klonos Which one was the specific breakage? The issue description says, the idea was inspired by several issues.

olafgrabienski avatar Aug 04 '20 13:08 olafgrabienski

I would suggest make changes apply only to new sites, and list updates in the README or a CHANGES.txt file (or something like that), to be applied manually by those who want to.

Maybe even add a cancellable notification in status report that there were changes to CSS?

docwilmot avatar Aug 04 '20 15:08 docwilmot

@olafgrabienski I cannot find the specific issue for it now. I was referring to change that we've introduced that broke something (image alignment IIRC), and we then had to revert that.

klonos avatar Aug 06 '20 22:08 klonos

make changes apply only to new sites, and list updates in the README or a CHANGES.txt (...) to be applied manually by those who want to.

Would that mean, if I want some CSS updates and don't have a sub-theme, I have to apply my changes again on every core update? If that'd be the case, let's better go with suggestion 4 instead, the "supplemental stylesheet" approach which provides a setting for this.

olafgrabienski avatar Aug 07 '20 07:08 olafgrabienski

I was referring to change that we've introduced that broke something

I see, but really breaking something (as in "bug") is not the point here, in that case we revert the bug with the next release. If I understand this issue here correctly, it's about 'breaking' a site only because we make unexpected changes. These changes tend to be helpful but they change a known behavior and might conflict with expectations or workarounds.

olafgrabienski avatar Aug 07 '20 07:08 olafgrabienski

...I've found the issue: #1140

We've introduced the change in 1.13.3 and then had to revert it in 1.15.2, because we realized that we broke things for existing sites 😓 ...see comment https://github.com/backdrop/backdrop-issues/issues/1140#issuecomment-521476053 and later.

klonos avatar Aug 07 '20 20:08 klonos

As I understand it, the issue this topic is addressing is what happens when someone uses Basis as their theme, and then Backdrop has an update to Basis that might break their changes.

What happens in a similar situation with Views? What if someone changes one of the admin views (e.g. /admin/content), and then a new version of Backdrop includes some changes to that view too? What happens with Layouts?

I think the answer is (correct me if I'm wrong) that Backdrop's new changes aren't included because the view/layout has been 'overridden'. Updates are only made to default views/layouts. If the user at some point reverts the view/layout to the default, then they'll get those new updates.

Themes are slightly different. We strongly discourage 'hacking' core (i.e. making changes to a theme directly), so we don't need to worry about those situations (whatever happens is the user's own fault). Therefore we're only worried about someone copying the Basis theme out of core and then making changes to their copy, or creating a sub-theme of Basis.

If they make their own copy, we can update core Basis no worries. Their theme won't be affected, unless they 'revert' to the default Basis theme.

So the only issue left is if they sub-theme Basis. In this case, can we make it so that updates are only applied to core Basis if the site's active or admin theme don't list Basis as the base theme in their info files? If the active/admin theme aren't sub-themes of Basis, we can update Basis no worries. However if they are sub-themes, then the updates aren't applied.

But how do we allow users to get the changes to Basis even if they're using their own copy or a sub-theme? Well if they override a view or layout, we don't allow them to get the latest changes to them until they revert their own changes, so why do we need to do that with themes? If people want the latest changes, they can use core Basis.

ghost avatar Aug 09 '20 11:08 ghost

Or copy the changes to their own copy or a sub-theme.

oadaeh avatar Aug 09 '20 18:08 oadaeh