Color names not working with ExpandShorthands about php-css-parser HOT 12 CLOSED

Hi @BenPayton,

Thanks for the report <3

The shorthand border property can indeed be specified with the values in any order, so this definitely looks like a bug.

There are some restrictions on the ordering of values in the background shorthand, but I don't see that your input violates these. I also note that your output includes background-attachment: scroll when you didn't specify this in the input, which doesn't seem right either.

We'll need to investigate this further. If you are able to delve into the code to find out why what's going wrong is, that would be greatly appreciated, but no worries if not.

from php-css-parser.

Thanks. A quick look at RuleSet\DeclarationBlock\expandBackgroundShorthand() shows it sets an array of default properties in $aBgProperties. It appears that background-attachment: scroll and background-color: transparent end up as rules unless it specifically finds different values in the original shorthand declaration. Since my example has no background-attachment value, it gets scroll by default. As for background-color, it looks like it specifically looks for instances of Color and I'm guessing a color name isn't being recognized as such.

from php-css-parser.

I think if we were to recognize named colors and parse them into Color, it would allow us to uniformly fix the problems in both expansions.

from php-css-parser.

distinguishing a border-style from a colour, which would need a list of named values of one or other to separate them (there are far fewer border style constants, so I'd use those for the sieve).

I think border-style has a higher chance of a new value being added to the spec than a new named color being introduced. The last named color added was rebeccapurple in honor of Rebecca Meyer and that was already in violation of the long-standing rule against adding new named colors.

from php-css-parser.

Your analysis makes sense.

A quick look at RuleSet\DeclarationBlock\expandBackgroundShorthand() shows it sets an array of default properties in $aBgProperties. It appears that background-attachment: scroll and background-color: transparent end up as rules unless it specifically finds different values in the original shorthand declaration. Since my example has no background-attachment value, it gets scroll by default.

On reflection, I think that is correct behaviour. I often forget that the shorthand syntax replaces all longhand properties, including any that are not provided (with default values being used for those missing, and no 'cascade' for them). Apologies.

As for background-color, it looks like it specifically looks for instances of Color and I'm guessing a color name isn't being recognized as such.

Yes. Color doesn't cover named colours, only rgb(a), hsl, etc., and hex values. Named colours are retained as string.

Looking at expandBackgroundShorthand, it seems there is a missing else which would cover the case of a named colour (assuming there's nothing else it could be).

The border issue is similar, but it looks like in expandBorderShorthand there needs to be a means of distinguishing a border-style from a colour, which would need a list of named values of one or other to separate them (there are far fewer border style constants, so I'd use those for the sieve).

Longer term, we are considering introducing classes to represent specific types of named values (e.g. #353), instead of using string, but that's some way off.

In the meantime, we are backporting bugfixes to an 8.5.x branch.

@BenPayton,

Would you be willing to have a crack at a couple of pull requests (PRs) to fix these two problems? (I think the background and border issues should be tackled seperately, for now.) We'd also want tests added (that fail before and pass with the code change). No worries if not (I see you only just created a GitHub account), but it would speed things up if you are able to contribute.

Thanks.

from php-css-parser.

Thank you for the response. Agreed on the default background rules, that should be correct behavior. It makes sense both expandBackgroundShorthand and expandBorderShorthand simply need checks for named colors. By replacing color names with hex values in my own project before running expandShorthands everything appears to work as expected.

I don't think I will have the time commitment to contribute unfortunately, so I will leave it to someone more capable. Thanks again for your help.

from php-css-parser.

IMHO expandShorthands isn’t functionality expected in a parser and we should consider removing it. It sounds like something a third-party could maintain as a separate lib.

expandShorthands needs to be updated whenever the specs change. For example, we don’t recognize (or reset) background-origin nor background-clip or background-size, which can also be specified in the shorthand (but that wasn’t the case when the functionality was first implemented).

Similarly, I am not sure whether we support expanding background shorthands correctly if more than one background was specified…

from php-css-parser.

IMHO expandShorthands isn’t functionality expected in a parser and we should consider removing it. It sounds like something a third-party could maintain as a separate lib.

I agree. I've just created #512 for this.

from php-css-parser.

So I'd suggest closing this as "sorry, won't fix anymore". Would you be okay with this, @sabberworm @JakeQZ ?

from php-css-parser.

So I'd suggest closing this as "sorry, won't fix anymore". Would you be okay with this, @sabberworm @JakeQZ ?

Fine by me. Though I would still like to pursue support for parsing named colors as Color instances.

from php-css-parser.

Though I would still like to pursue support for parsing named colors as Color instances.

I agree and have created #514 for this.

from php-css-parser.

So I'd suggest closing this as "sorry, won't fix anymore". Would you be okay with this, @sabberworm @JakeQZ ?

Fine by me. Though I would still like to pursue support for parsing named colors as Color instances.

Agree with all this. Though it would be good for the existing (incomplete) implementation of expanding shorthands to be made clearly available for anyone who wants to use it as a basis for providing that functionality in a separate package (or just in their own project) - I've commented to that effect in #512.

from php-css-parser.