backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 38 forks source link

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

Open stpaultim opened 4 years ago

stpaultim commented 4 years ago

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

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.

jenlampton commented 3 years ago

we add all new CSS changes to the existing stylesheets

I don't get it. We need to find a way to put the CSS changes someplace where they won't affect any existing site. Adding CSS changes to stylesheets that are already in use on live sites is what breaks them.

and then add SS that revert it back to how it was before the change (for backwards-compatibility).

When you say "revert it back" does that mean that this additional stylesheet is an exact copy of the previous version of the core stylesheet? Those wouldn't be supplemental anymore, so I'd call them "backwards-compatible stylesheets" instead. (I'll call this option A, below)

Or, would they only contain the inverse of the new rules that were added to the original stylesheet? So if we added a text-align: center to the original stylesheet, the supplemental one would add a text-align: left to put it back how it was before? (I'll call this option B, below)

I can understand the intent with wanting to have an exact copy of the stylesheet before we introduced the change (option A), but that file would need to be the one that remains in-use on a production site to prevent breakage. And -- this is the important part -- it would also need to have exactly the same filename as it did before the update. (so, copying it to a file named whatever-bc-1.18.css wouldn't fly...)

Adding any CSS changes to a production site can cause it to break, but there are several ways changing the name of a core stylesheet can also have breaking affects on a production site:

  1. A stylesheet can be overridden by adding that stylesheet in a theme's .info file and then providing a file with the same filename in the theme. (I do this on nearly all my sites with menu-dropdown.theme.css and menu-toggle.theme.css. These two stylesheets are intended for exactly this purpose, hence the theme in their name. I have seen it done lots of other core stylesheets, though.)

  2. A stylesheet can be removed by adding that stylesheet in a theme's .info file but not providing the file. Here's an example from Basis:

; Remove unwanted core CSS
stylesheets[all][] = system.theme.css
stylesheets[all][] = menu-dropdown.theme.css
  1. A stylesheet can also be removed or re-ordered in hook_css_alter(). The keys of the $css array are the filename of the CSS files.

By changing a stylesheet's filename in a release (or switching all existing sites to use a different file entirely -- option A), all the CSS that was removed or replaced before the release will be added back to that site, possibly causing breakage.

By adding additional stylesheets to existing sites (option B), we would also be adding CSS changes to a production site that had previously solved the problem in a different way. Now you have the "revert" CSS on a site, but not the the CSS it's reverting, also possibly causing breakage.

Whatever it is we decide to do, it needs to not affect the existing sites at all. That means not changing stylesheets that are already in use on existing sites (and potentially overridden, or removed) and not adding any additional stylesheets to existing sites. Whatever changes we do decide to make, the difference in CSS must only apply for new installs.

It's the fact that there's a proposal to add new CSS changes to additional CSS files.

Unfortunately, as noted above, there's no way to avoid breaking production sites that doesn't include putting the new CSS changes into additional CSS files.

Wouldn't that mean that users would never get updates to CSS for their site? As in, years down the track they're still seeing an old version of Basis that looks way different to new sites, when in reality they just didn't want to break one little thing at the time...

Yep, that's exactly what it means. We can't do any CSS updates automatically or sites will break.

Any of the changes that new sites are getting would need to opt-in for existing sites, so that people would have a chance to review the breakage and fix it on their own schedule.

ghost commented 3 years ago

I meant 'option b'.

Say Basis has .hero-block {padding-bottom: 10px;} and we decide to change that. We change Basis to be .hero-block {padding-bottom: 0;} and then add a new CSS file (basis_1-17-1.css) that has .hero-block {padding-bottom: 10px;}.

findlabnet commented 3 years ago

Not too difficult for site architects?

stpaultim commented 3 years ago

I've opened a new issue in the BackdropCMS.org issue queue to discuss a proposal for bringing this issue towards some kind of decision. Let's keep the technical discussion here and the process discussion there (at least that is my suggestion).

https://github.com/backdrop-ops/backdropcms.org/issues/728

ghost commented 3 years ago

I feel like a single list of all options with their pros and cons would be helpful, but it feels overwhelming to try and get that organised...

jenlampton commented 3 years ago

When an existing site is updated and they want the latest changes, they get the change in Basis 👍

I'm assuming this will be done by manually copying the new css file to their theme? If the change were forced on them instead, it could break their site, which would be 👎

When an existing site is updated and they don't want the change, then the new CSS file (basis_1-17-1.css) is added to their site which revents the change in Basis 👍

and...

When a site has overridden or removed Basis' styling, they won't see the change (presumably that's why they overrode/removed it)

As explained in detail above, adding a new CSS file to existing sites means they will see this change which could also break their site, so actually 👎

ghost commented 3 years ago

I'm assuming this will be done by manually copying the new css file to their theme?

No, there's no 'new' CSS file. We update Basis' normal CSS file with the change, in the same way that if there's a bug elsewhere in core we just fix it in the original file.

I guess you can call this an 'opt-out' change. All sites get the update by default (as with all other updates/fixes in core), but users can choose to opt-out if they don't want breakages so then a new CSS file gets added which reverts the change.

I'm really just trying to explain the suggestion @quicksketch made in the meeting. Please either watch that or talk to him, because I'm not sure I can keep explaining it better...

ghost commented 3 years ago

The main problem here seems to be backwards-compatibility. Reading our philosophy of the matter, it says:

Easier updates: Backwards compatibility is important. 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:

Things I take from that:

So, with the lack of an elegant solution available to this problem for both core CSS changes and core theme CSS changes, I stand fully (and very likely alone) in the 'make CSS changes to core to improve it where necessary, and note possible breakages in release notes' corner.

jenlampton commented 3 years ago

Easier updates: Backwards compatibility is important.

CSS is definitely a gray area... It's not an API, but it does affect backwards-compatibility.

Most Drupal site owners don't stay on top of updates because it usually requires a developer to do so. Since Backdrop has a built-in "update" button, hiring a developer to make updates to a Backdrop site is no longer necessary. By building breaking changes into our process, we're effectively un-doing the good we did by providing that self-update button: we're taking the control we gave to site-owners away again. :(

Backdrop CMS enables people to build highly customized websites affordably, through collaboration and open source software.

Keeping things affordable is also part of our mission statement. Every time we break sites that are not under active development those site owners will have unexpected costs. I'm concerned that this is going to discourage people from updating their sites.

One of the pain-points of owning Drupal 7 sites is the cost of long-term maintenance. By removing the requirement that a developer make the updates to a site, the long-term maintenance of a Backdrop site becomes significantly lower than that of a Drupal site.

So, with the lack of an elegant solution available to this problem for both core CSS changes and core theme CSS changes

I personally care a lot more about grumpy clients, increased costs, and broken sites a lot more than if a perfectly-good backwards-compatible solution is elegant enough. But I am not a core committer, so I think we should keep working on this. Soo... I have another idea! :) I think @stpaultim was also thinking about something similar:

Supplemental CSS selectors https://github.com/backdrop/backdrop-issues/issues/4782

I haven't written a proof-of-concept PR for this yet, but I'm hoping that people may find this approach more elegant than previous backwards-compatible solutions :)

klonos commented 3 years ago

Interesting! ...in Drupal-land they seem to have decided to ship a starter theme in core, in order to address this issue:

https://www.drupal.org/project/drupal/issues/3050384

Problem/Motivation

Classy, which is the current base theme provided by core is stale because most changes are not allowed to retain backwards compatibility. We need to retain backwards compatibility because some design requirements of themes depending on Classy could depend on markup and/or CSS provided by Classy.

While reviewing different approaches, we talked to members of the frontend community. Something that came up was that many of them recognize themselves in a situation where they are using Classy, but they've ended up overriding most if not all templates. This is a good practice since markup should be adjusted specifically to the design requirements of the theme. If theme overrides all of the CSS and markup, it would be possible to make changes safely in Classy. However, this is not something enforced by Drupal so we cannot rely on that at the moment.

Proposed resolution

Provide a starterkit theme in core. This will give us more freedom for the maintenance of the theme since making changes to the starterkit doesn't affect any production themes directly. The contrib space contains multiple examples of this that we could look into as part of figuring out the specific. The key change will be that in the process of creating a new theme based on starterkit, all templates are copied to the new theme, rather than them being loaded run time from a base theme.

The starterkit theme should include a command line tool that allows generating a new theme. The CLI tool should allow configuring the name and the machine name of the theme.

image

I think that we should consider this option as well. ...but I would like us to have a form in the admin UI that generates a theme, besides the command-line tool.

Also see:

jenlampton commented 3 years ago

Our issue is about css, the Drupal solution solves HTML changes, but not css. Interesting, but not related.

On Sun, Mar 21, 2021, 2:01 AM Greg Netsas @.***> wrote:

Interesting! ...in Drupal-land they seem to have decided to ship a starter theme in core, in order to address this issue:

https://www.drupal.org/project/drupal/issues/3050384

Problem/Motivation

Classy, which is the current base theme provided by core is stale because most changes are not allowed to retain backwards compatibility. We need to retain backwards compatibility because some design requirements of themes depending on Classy could depend on markup and/or CSS provided by Classy.

While reviewing different approaches, we talked to members of the frontend community. Something that came up was that many of them recognize themselves in a situation where they are using Classy, but they've ended up overriding most if not all templates. This is a good practice since markup should be adjusted specifically to the design requirements of the theme. If theme overrides all of the CSS and markup, it would be possible to make changes safely in Classy. However, this is not something enforced by Drupal so we cannot rely on that at the moment. Proposed resolution

Provide a starterkit theme in core. This will give us more freedom for the maintenance of the theme since making changes to the starterkit doesn't affect any production themes directly. The contrib space contains multiple examples of this that we could look into as part of figuring out the specific. The key change will be that in the process of creating a new theme based on starterkit, all templates are copied to the new theme, rather than them being loaded run time from a base theme.

The starterkit theme should include a command line tool that allows generating a new theme. The CLI tool should allow configuring the name and the machine name of the theme.

I think that we should consider this option as well. ...but I would like us to have a form in the admin UI that generates a theme, besides the command-line tool.

— 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-803536637, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADBER2BJIWAXCLTN53D2QTTEWYXDANCNFSM4JFUG4IQ .

klonos commented 3 years ago

Hmm 🤔 ...I thought it was meant to solve both CSS and HTML changes.

jenlampton commented 3 years ago

Templates don't cascade in the same way, either one is used or another, but there's no possibility of many being used for one thing. With CSS you could end up with a site that uses code from both the sub-theme, a base-theme, and core (or, any module, actually).

klonos commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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:

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 commented 3 years ago

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 commented 3 years ago

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.

stpaultim commented 2 years ago

At the last Backdrop LIVE we had a full session on the topic of adding a new theme to core (https://github.com/backdrop/backdrop-issues/issues/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 commented 2 years ago

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 commented 2 years ago

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 commented 3 months ago

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