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

Shorthand properties don't merge with equivalent non-shorthand properties as expected. #19

Open
tkaemming opened this issue Jul 22, 2016 · 2 comments
Assignees

Comments

@tkaemming
Copy link
Collaborator

tkaemming commented Jul 22, 2016

Shorthand properties (e.g. padding, margin, font, etc.) mixed with individual property declarations don't merge as expected, and the resulting property list can violate specificity rules.

For instance, this example:

h1 { padding: 0 10px; }
h1#id { padding-top: 10px; }

should combine to either of these equivalent forms:

padding: 10px 10px 0 10px

or:

padding-top: 10px;
padding-right: 10px;
padding-bottom: 0;
padding-left: 10px;

… but right now, that property list ends up merging and resulting in this property list:

padding: 0 10px;
padding-top: 10px;

or, this property list — since the output order is non-deterministic:

padding-top: 10px;
padding: 0 10px;

These two different forms have two different results, with their computed results being:

padding-bottom: 0px;
padding-left: 10px;
padding-right: 10px;
padding-top: 10px;

and:

padding-bottom: 0px;
padding-left: 10px;
padding-right: 10px;
padding-top: 0px;

This behavior is explained under "Tricky edge cases" on the MDN documentation.

A good first step would probably be to expand shorthand properties to individual properties upon parsing (ensuring shorthand and individual properties are never merged.) Later, properties could be "compressed" back into shorthand when possible to conserve bytes.

@tkaemming tkaemming self-assigned this Jul 22, 2016
@tkaemming tkaemming changed the title Shorthand properties don't merge with equivalent non-shorthand properties. Shorthand properties don't merge with equivalent non-shorthand properties as expected. Jul 22, 2016
tkaemming added a commit that referenced this issue Jul 25, 2016
* Use `CSSStyleDeclaration` for `Properties.from_string` (and remove
  unnecessary test.)
* Remove poorly implemented failing test.
* Clean up imports for `cssutils`.

References GH-19.
@tkaemming
Copy link
Collaborator Author

tkaemming commented Jul 25, 2016

This is partially fixed — I updated this in GH-20 this so that margin and padding are expanded from their shorthand forms to specific property values. If other shorthand properties are used, a warning will get logged so that the caller (hopefully) knows that they should use the specific form when they are depending on rules to inherit correctly.

There are still a few things to do here:

  • Add support for border-width. This should be reasonably easy, except the property name prefix would need to be changed to a template, as the property-{top,right,bottom,left} pattern doesn't hold here, since this expands to border-{top,right,bottom,left}-width. See Add support for border-width shorthand property. #21.
  • If all of the specific properties are defined for a node, we should be able to collapse them back to the shorthand to simplify the output and save on bytes. I didn't do this yet, since the Properties type is a mapping of string values, which can include a priority definition (e.g. !important.) Ideally, this would only merge the specific values back into a shorthand representation if all of the property value priorities are the same. It'd be a lot simpler do to this if the Properties object was a mapping where values are Property types which include the priority attribute.
  • Add support for the more complex shorthand properties, such as font and background.

@tkaemming
Copy link
Collaborator Author

tkaemming commented Aug 4, 2016

I didn't do this yet, since the Properties type is a mapping of string values, which can include a priority definition (e.g. !important.)

Oops, I forgot about this one. Should do that before releasing the next version.

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

No branches or pull requests

1 participant