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

Add support for short color hex codes in Color module #4913

Closed
ghost opened this issue Jan 26, 2021 · 9 comments · Fixed by backdrop/backdrop#3519
Closed

Add support for short color hex codes in Color module #4913

ghost opened this issue Jan 26, 2021 · 9 comments · Fixed by backdrop/backdrop#3519

Comments

@ghost
Copy link

ghost commented Jan 26, 2021

Description of the bug

The Color module works like so: the user, via the UI, selects a colour to use for a part of the page (say, orange for the page background). Color module takes the original colour of the page background from the theme (#ffffff), then searches through CSS files for matching colour hex codes. It then replaces matches with the new colour, then saves the changes to a new stylesheet which overwrites the default one.

However, when the user selects a colour via the UI, the colour chosen is given as a long hex value (e.g. #ff7a00). If the theme specifies its colours as short hex values (e.g. #fff), they don't match and so Color doesn't replace them.

Steps To Reproduce

To reproduce the behavior:

  1. Make sure Basis is the default theme
  2. Go to /admin/appearance/settings/basis
  3. Select a different colour for the 'Main background' (e.g. red) - note that the preview changes appropriately
    image
  4. Edit Basis' skin.css to change the body background from #ffffff to #fff (line 16)
  5. Refresh /admin/appearance/settings/basis
  6. Select a different colour for the 'Main background' (e.g. green)

Actual behavior

The main background in the preview reverts to the default (white).
image

Expected behavior

The main background in the preview should change to green.

Additional information

This bug forces theme authors to ignore Backdrop's CSS coding standards by specifying long hex values instead of the preferred short hex values.

@indigoxela
Copy link
Member

This bug forces theme authors to ignore Backdrop's CSS coding standards

It might seem a bit heretic, but... Wouldn't it be easier to adapt the coding standard to real life handling?

If the color module changes the handling of short hex values, it might lead to changes in existing themes (color palettes). Changing the standard definition, won't hurt anyone and guarantees that existing theme implementations stay untouched.

In the end a coding standard is just an agreement. 😉

@ghost
Copy link
Author

ghost commented Jan 26, 2021

Wouldn't it be easier to adapt the coding standard...

Easier, yes. It's just a text change in a node on the API site. But Color module was obviously meant to support short hex codes, so not doing so is a bug and bugs should get fixed (especially when the fix is an easy one).

@ghost
Copy link
Author

ghost commented Jan 26, 2021

If the color module changes the handling of short hex values, it might lead to changes in existing themes (color palettes).

I guess that's the age-old question: if there's a bug but people are just used to it (or have worked around it), is it worth fixing?

I still say 'yes'.

@indigoxela
Copy link
Member

indigoxela commented Jan 26, 2021

I still say 'yes'.

Then I'd need some time to update my contrib themes. 😉 They would suffer from side effects (I suppose).

@ghost
Copy link
Author

ghost commented Jan 26, 2021

Some notes for context:

  • The line of code I linked above that shows Color module searching for both long and short hex codes comes from a commit from Drupal in 2006
  • The user-selected long hex codes come from the change we made when we introduced the 'color' input form element in 2018

My interpretation of these facts is that:

  • The bug hasn't been around for very long at all, considering how long the functionality was there before the bug was introduced
  • People/themes coming from Drupal will be used to being able to provide either long or short hex codes (or both)

Therefore the bug should be fixed, and Backdrop themes developed/ported in the last two years that have 'worked around' the bug can simply be updated (rather than all themes developed in the last 15 years needing to be updated).

@indigoxela
Copy link
Member

The bug hasn't been around for very long at all

I did a quick check with D7 and Bartik. I do remember it correctly - that "bug" is around since quite a long time, also in D7. Short hex values don't get replaced.

But please don't get me wrong, I don't oppose to this change in any way, I just warn about possible side effects and that we should be cautious like with any CSS change.

@klonos
Copy link
Member

klonos commented Jan 26, 2021

Thanks @BWPanda 🙏 ...simple code change + works as expected. RTBC 👍

@ghost
Copy link
Author

ghost commented Jan 26, 2021

If the color module changes the handling of short hex values, it might lead to changes in existing themes (color palettes).

I was thinking about this last night, and I believe Color module already has a way for themes to specify sections of CSS that shouldn't be overridden (using 'Color Module: Don't touch'). So if themes are relying on a bug to avoid having certain colours overridden, rather than the built-in feature specifically for this purpose, then they're not doing it right and need to be updated anyway.

@quicksketch
Copy link
Member

Nice fix @BWPanda! Thanks @indigoxela and @klonos for reviewing. I merged backdrop/backdrop#3519 into 1.x and 1.18.x.

@jenlampton jenlampton changed the title Color module doesn't support short color hex codes in themes Add support for short color hex codes in Color module Mar 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants