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 CSS properties can cause style inconsistencies #8689

Closed
bvaughn opened this issue Jan 4, 2017 · 6 comments
Closed

Shorthand CSS properties can cause style inconsistencies #8689

bvaughn opened this issue Jan 4, 2017 · 6 comments

Comments

@bvaughn
Copy link
Contributor

bvaughn commented Jan 4, 2017

I'm reporting a bug.

What is the current behavior?

  1. Render a host component with a style that contains both a shorthand CSS property and an over-writing longer form of the property (eg overflow and a conflicting overflowX and/or overflowY property).
  2. Update the host component (re-render) with the same shorthand property (eg overflow) but remove the longer form property (eg overflowX).
  3. The host component style is now invalid.

Examples

This bug can be reproduced here: https://plnkr.co/edit/7O1xZUqbD13ST2BAxcm9?p=preview

Update: I have since updated the Plnkr to use border instead of overflow since it makes the problem more immediately obvious to spot.

Example

  1. Render style={{ overflow: 'hidden', overflowY: 'auto' }}
  2. Render style={{ overflow: 'hidden' }}
  3. Expected: <div style="overflow: hidden;" />
  4. Actual: <div style="overflow-x: hidden;" />

Alternate example

  1. Render style={{ overflow: 'auto', overflowX: 'hidden', overflowY: 'hidden' }}
  2. Render style={{ overflow: 'auto' }}
  3. Expected: <div style="overflow: auto" />
  4. Actual: <div style="" />

You can also reproduce this bug with properties like margin, padding, border, etc.

Caveats

Note that if the shorthand value changes between renders things work as expected (because React explicitly updates the shorthand style).

I know this is very edge-case behavior and so may not be worth fixing. I originally noticed it by way of issue bvaughn/react-virtualized/issues/525.

Which versions of React / browser / OS are affected

This bug reproduces in Chrome, Firefox, and Safari using React 15.4.1 as well as the unreleased ReactDOMFiber renderer.

@bvaughn
Copy link
Contributor Author

bvaughn commented Jan 5, 2017

I think the underlying cause here is that ReactDOM only updates the style attributes that have changed (eg overflowX, overflowY). But removing one or both of these properties from a CSSStyleDeclaration impacts the overflow property as well, resulting in a CSSStyleDeclaration instance that doesn't necessarily match the most recently specified style object..

I was not able to reproduce this issue in a unit test (using Jest) because Jsdom doesn't trickle down the effects like the major browser vendors seem to (eg updating overflowX doesn't impact overflow).

This issue may not be worth fixing. It's kind of an edge case. The only ways to "fix" this that I can think of would be:

  1. Special-case updates for shorthand properties like overflow.
  2. Explicitly set/reset all properties defined on the nextProp.style object on CSSStyleDeclaration (rather than doing the minimum edit) in addition to removing properties that were only present on lastProp.style.
  3. Use cssText instead. (I assume we don't do this because it's slower?)

@bvaughn bvaughn changed the title overflowX (or overflowY) sometimes breaks subsequent overflow style Specific and shorthand style name bug Jan 5, 2017
@bvaughn bvaughn changed the title Specific and shorthand style name bug Shorthand CSS properties can cause style inconsistencies Jan 5, 2017
@syranide
Copy link
Contributor

syranide commented Jan 5, 2017

Overlapping styles are not allowed. The solutions are very costly and it was (AFAIK) decided that it is not reasonable to support it. Warnings would be very helpful but that hasn't be done yet. There are a number of issues discussing it if you're interested in more information.

@bvaughn
Copy link
Contributor Author

bvaughn commented Jan 5, 2017

Overlapping styles are not allowed

That's news to me. It may be the right decision but I don't think it's communicated clearly (at least not that I've seen). And it works most of the time which may lead someone to assume it's supported.

The solutions are very costly

Fair enough. I was curious how costly cssText would be compared to manually managing the updated properties as we are currently doing but I have not done any benchmarking. Looks like this specific thing has been discussed previously but without a clear resolution.

@bvaughn
Copy link
Contributor Author

bvaughn commented Jan 5, 2017

Searching through React issues now I do see this mentioned a few other places (eg #5397 and #6348). When I first observed this issue it was related to overflow. I didn't immediately realize it affected all shorthand properties and so when I searched for pre-existing bugs- I failed to find them because my search was overly narrow.

@bvaughn
Copy link
Contributor Author

bvaughn commented Jan 5, 2017

I'll close this issue as a dup of #6348 and #5397.

Thanks @syranide!

@esr360
Copy link

esr360 commented Apr 11, 2020

Sorry to comment on an old and closed issue but is there no where currently open to discuss this issue? I'm facing it as demonstrated here https://codesandbox.io/s/elated-flower-zxvkf?file=/src/App.js and don't really like that "avoid using short-hand properties" is being pushed as an acceptable Developer Experience, especially for CSS enthusiasts.

Border, for example, is an extremely common property. To avoid this issue I have to author my styles like this:

borderWidth: '1px',
borderStyle: 'solid',
borderColor: 'blue'

Instead of

border: '1px solid blue'

Whilst I understand there are costly performance issues meaning a solution is not viable, I'd still like the issue to be considered valid, and open (as I believe it is still valid, and it is not yet solved). Thanks.

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

No branches or pull requests

3 participants