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 missing unitless CSS properties #19286

Closed
wants to merge 1 commit into from
Closed

Conversation

kripod
Copy link

@kripod kripod commented Jul 8, 2020

Summary

Similarly to a proposal for Preact, I acquired the list of standard and experimental CSS properties which accept unitless values using MDN data.

Test Plan

These changes only expand the list of unitless CSS properties, avoiding some issues with unwanted 'px' postfixes.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 8, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 8fb862b:

Sandbox Source
React Configuration

@sizebot
Copy link

sizebot commented Jul 8, 2020

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 8fb862b

@sizebot
Copy link

sizebot commented Jul 8, 2020

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 8fb862b

@@ -9,7 +9,9 @@
* CSS properties which accept numbers but are not in units of "px".
*/
export const isUnitlessNumber = {
animation: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one looks wrong to me. I assume it refers to the "duration" or "delay" parts of the shorthand? Looking at the animation-duration docs, it says:

The time that an animation takes to complete one cycle. This may be specified in either seconds (s) or milliseconds (ms). The value must be positive or zero and the unit is required. A value of 0s, which is the default value, indicates that no animation should occur.

The animation-delay docs say the same:

The time offset, from the moment at which the animation is applied to the element, at which the animation should begin. This may be specified in either seconds (s) or milliseconds (ms). The unit is required.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit hidden under the grammar for animation: <time> || <easing-function> || <time> || <single-animation-iteration-count> || <single-animation-direction> || <single-animation-fill-mode> || <single-animation-play-state> || [ none | <keyframes-name> ] where <single-animation-iteration-count> is valid syntax which can be a single number.

Reproducible in chrome:
animation-shorthand-unitless

--https://codesandbox.io/s/animation-unitless-mg06t

Copy link
Contributor

@bvaughn bvaughn Jul 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Thank you for clarifying.

I'm still concerned that changing animation may be a bad idea, since the majority of the numeric values this shorthand property covers do require units.

Then again, as I've mentioned elsewhere on this PR, this is not my area of expertise.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since animation: 1px is meaningless/invalid, I don’t think adding animation can break anything, at least (except if perhaps someone has a unitless number that gets px and thus ignored today).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm not familiar with this part of the code, so I don't know what the implications of adding this rule for a shorthand property that can contain multiple numeric values. I agree animation: 1px is meaningless though 😆

maskBorderOutset: true,
maskBorderSlice: true,
maskBorderWidth: true,
maxLines: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This property isn't standardize and I think has been superseded by line-clamp? So maybe we shouldn't add it here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s right, I just wanted to add any property with a status of standard or experimental.

@bvaughn
Copy link
Contributor

bvaughn commented Jul 10, 2020

Since this is a change to a part of ReactDOM that I'm not very familiar with, would you mind doing some testing to ensure the before/after behavior for each of these new properties is correct?

Screenshots would be excellent too.

@gaearon
Copy link
Collaborator

gaearon commented Jul 10, 2020

This adds some bytes to the bundle size but so far (?) no one has asked for these specific properties. Is it worth doing?

If the fix is just to remove the inconsistency, maybe it's not worth doing now. Instead we could flip the list and use a simpler heuristic in the future (#13567). Which would be a breaking change, but would do away with any inconsistencies.

@trueadm
Copy link
Contributor

trueadm commented Jul 10, 2020

I'm not sure there's any real gain from making this change. It adds more size to ReactDOM and there's no real demand for these properties (other than this PR). Furthermore, adding px is definitely a historial feature that isn't as relevant these days. Folks should be opting to use relative units rather than static units (like px), otherwise they break their app/website to users who use zooming tools or accessibility tooling – invaliding Web Content Accessibility Guidelines. Specifically:

  • WCAG 1.4.4: Users must be able to resize text without assistive technology up to 200 percent, without loss of content or functionality. (Level AA)
  • WCAG 1.4.8: Ideally, we should provide appropriate spacing between lines and paragraphs, and we shouldn’t be requiring the user to scroll horizontally to read a line of text on a full-screen window. (Level AAA)
  • WCAG 1.4.10: Users must be able to resize text without being forced to scroll both horizontally and vertically to read that content. (Level AA)

Ideally, we should move React to use a small whitelist like mentioned above, with the ultimate goal of removing this feature entirely in a future version of React.

@kripod
Copy link
Author

kripod commented Jul 12, 2020

Thank you, I appreciate all those feedbacks and thoughts mentioned above. I haven’t been aware of alternative approaches before submitting this PR, and for now, I decided to close this PR until a more sustainable solution emerges.

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

Successfully merging this pull request may close these issues.

8 participants