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

Allow specifying multiple fallback values for inline styles (e.g. for vendor prefixing with SSR) #6467

Closed
AntouanK opened this issue Apr 9, 2016 · 17 comments

Comments

@AntouanK
Copy link

AntouanK commented Apr 9, 2016

I've been using the "string" hack to override CSS values in React components.

For example, if you want to have display with different values, you do

styleObj.display = '-webkit-box;display: -moz-box;display: -ms-flexbox;display: -webkit-flex;display: flex';

I have an npm module for poly-filling my styles in my React components.
https://www.npmjs.com/package/poly-style

All of that worked perfectly in v0.14. No warnings no nothing.
Updated to v15.0 today, and everything is breaking.
a) Normally there are deprecation warnings. I didn't notice any warnings on that change.
b) in the changelog I cannot see anything related to that, to understand what changed.

Am I missing something?

@gaearon
Copy link
Collaborator

gaearon commented Apr 9, 2016

I don’t think setting a property value to something like 'value; other-name: other-value' was ever officially supported. It might have worked by coincidence because, as mentioned prominently in the blog post, we used innerHTML to populate the initial tree when a component is mounted. It so happens that this hack you were using is compatible with the way we used to glue HTML code on initial mount.

However I don’t think this way of specifying multiple CSS values for a single property was every supported. You won’t find it anywhere in the documentation or example. Moreover, I’m pretty sure it was broken before 15 in a different way: yes, mounting a component with a style like this would work in 0.14, but updating it would fail for the same reasons this doesn’t work at all in React 15. Updates were always using DOM API for setting styles rather than innerHTML, so this hack would not work with updates. React 15 just normalizes the behavior between mounting and updates, and always uses DOM API to create DOM elements.

I see your point though. I don’t have a good solution to this inside React. This was discussed in #2020 (comment) before, and the conclusion is you should probably handle this outside React by feature testing. For example you can create a DOM element once when your code initializes, set different values of style.display on it, and see whether it sticks or gets reset. When it sticks, it means the browser supports the given value. This way you can build up a dictionary of mappings between standard values and the ones supported by the browser. Most likely there are some libraries that can do this for you.

I hope this helps! This isn’t a bug as you were relying on a hack, and we never officially supported it, as explained in #2020 (comment).

@gaearon gaearon closed this as completed Apr 9, 2016
@AntouanK
Copy link
Author

AntouanK commented Apr 9, 2016

Thanks for the response.

It's a hack in a sense that you don't set the "CSS" values directly. But all you do is set a string to a
variable.
It's a valid value as a string.

JS does not allow the same key in an object to have different values, so there's no other way to do that.

Yes, maybe attribute checking would be a workaround, but it's impossible to do that server side.
(also it's a lot of work to get it right)

Since react is removing a value depending on it's content, shouldn't you document that and warn the users? I don't understand why you think it's OK to hide from documentation. For anyone that uses inline styles, it's very important to know that.
Normally react is very good at throwing warnings/errors.

If I stringify the style attribute value myself and set it, is that also a behaviour that react is going to intercept? Shouldn't the user be able to set any attribute value he/she wants?

PS. it worked fine in 0.14 with updates. Check my app that uses that : https://hack.ernews.info

@gaearon
Copy link
Collaborator

gaearon commented Apr 9, 2016

Since react is removing a value depending on it's content, shouldn't you document that and warn the users?

It’s not React removing the value. Your browser does it. Try to assign a string like this to node.style.display without React (or just in style inspector) and you will see that this is the case.

We documented that React is switching to using createElement() in the blog entry several times. That is the reason why anything relying on concatenation will break. In my opinion this was clearly a leaking implementation detail and relying on such details always comes at your own risk.

PS. it worked fine in 0.14 with updates.

Would you mind providing a specific example code in JSFiddle that demonstrates it working? I don’t see how updates could possibly work correctly because React always used style DOM API for updates, and it can’t work the way you describe just because you rely on string concatenation which doesn’t happen in this case.

@AntouanK
Copy link
Author

AntouanK commented Apr 9, 2016

I see.
Maybe I don't understand exactly how it works then.
I'll work on some examples and I'll post any solution I find.

Thanks.

@gaearon
Copy link
Collaborator

gaearon commented Apr 9, 2016

Here’s a scenario that world not work in 0.14:

  1. On initial render, display is none
  2. After an update, it is set to this long string

So even the existing “support” for it was inconsistent and confusing. It was broken already, you just happened to be lucky to not hit these cases.

I agree it’s not good we don’t have a good solution for auto prefixing multiple values. We could potentially allow this by letting you pass an array for the value. Let’s reopen to hear what others think.

cc @zpao

@gaearon gaearon reopened this Apr 9, 2016
@oliviertassinari
Copy link
Contributor

This way you can build up a dictionary of mappings between standard values and the ones supported by the browser. Most likely there are some libraries that can do this for you.

Yes, indeed https://github.com/rofrischmann/inline-style-prefixer is a good prefixing library.
You can use it to use the right prefixed property.
E.g display: 'flex', will become display: '-webkit-flex',.

However, when dealing with server-side rendering, things get tricky.
In my case, this solution is not enough. I prefix for all user agent on the server side and only for the browser agent on the client side. As pointed out here #1979. I think that it's a good trade-off for server side caching.

I feel like we have only two options:

  • Only prefix for the user-agent on the server side
  • Drop the inline style for a CSS based solution

@AntouanK
Copy link
Author

AntouanK commented Apr 9, 2016

@gaearon Like you said, the fact that react was using innerHTML made that possible. So the browser picked up that string and only applied the value that it accepted.
I don't see any good way now to set the style with multiple values for display for example.

I will work on attribute checking for fixing my app on v15, but server-side rendering will be impossible now for inline styles.
( I'm only talking for cases where like display you need different values on the same key. prefixed keys are different so it's fine )

Do you think it's important enough for react to treat style attribute as a special case and provide a solution on this?
Since a JS object is not enough to cover cases like the above.

I mostly think that flex-box is a common case that users are going to need to polyfill for the next couple of years.

@AntouanK
Copy link
Author

AntouanK commented Apr 9, 2016

FWIW, I just updated my module to make some simple checks on runtime for IE and Safari.
https://github.com/AntouanK/poly-style/blob/master/src/polyStyle.js
Seems like on other platforms flex works fine... at least for my needs.
Probably needs a lot more work to get every single edge case poly-filled.

Server-side rendering is going to be an issue though.

@zpao
Copy link
Member

zpao commented Apr 11, 2016

This was never an officially supported use case so we're unlikely to do anything about this in the short term. The fact that it ever worked was simply a by-product of how we optimized initial render in the past.

On the client-size we've generally recommended something that picks the right values for you based on the user agent, but as noted, that's not great for server rendering. My recommendation has always been to just use plain old style sheets for this. There are several places where the DOM APIs don't support all of the things you can do with stylesheets in an optimized way (:hover is another example - adding and removing specific styles in JS in response to events is pretty crappy even if it's possible).

Inline styles (read: JS styles) is something we're investigating more over the coming months. I'm sure that will shape how we think of styles in React in the future, but right now we aren't planning on changing anything. The issue you're having and other related issues are all a part of the considerations.

@quasipedia
Copy link

@gaearon @zpao : Any news on how you (FB) is approaching this?

The proposal made by @gaearon about using an array seems a promising one to me, and one that would have a minor overhead in the general React rendering cycle. Did you make any decision on that?

If I may, I'd like to offer some less technical feedback: I am a bit confused by the ambivalence with which FB is dealing with the CSS in JS idea. On one hand it was @vjeux 's presentation that set the world of FE development in motion around this idea, but on the other hand it seems FB is not committed in backing-in full support for it (see @zpao comment on recommending "plain old style sheets", for example).

In my team, we chose to go with JS styles for our project because we felt - design-wise - to be a superior choice and offer a number of advantages we wanted to benefit from. When we chose to go down this road we knew we would have hit some bumps (e.g.: CSS pseudo-classes), and that we were threading uncharted waters, but we also thought that React as a project was experimenting with the same technology, and that difficulties would have been hammered out in the community and with the support of FB.

The community certainly is doing a great job (Fela, Radium, etc...), but I'm a bit taken aback from FB not recognising the importance of supporting a widely adopted pattern like the one described in this thread. True: is a hack. But right now it is the only hack that allows to get the job done, is in production in various places, and it is what everybody who ventured down this road is using.

Probably FB has excellent reasons not to work with the community on this, but I think it would be very helpful if you guys where more explicit about your stance on JS styling, so that people considering going down the rabbit hole know they are on their own. :)

@zpao
Copy link
Member

zpao commented Jul 26, 2016

Fela generates stylesheets from inline styles. Radium doesn't but I wouldn't be surprised if they move in that direction. jsxstyle generates stylesheets. aphrodite generates stylesheets.

As this is pushed into production by more and more people, the overwhelming solution appears to be generating stylesheets, which allows you to work around all of the issues with true inline styles (node.style.foo = bar). You can have a library that generates multiple display values. You have have pseudoclasses.

FB is currently approaching this by using stylesheets - we also don't use inline styles much. @vjeux can probably speak to this more as he did some experimentation internally. I think the approach was also going to be the same - generate stylesheets.

So my advice if you're using JS styles: don't use the pure form for an entire application. It's fine for a couple things but you hit a wall pretty quickly. For apps, use some helper which generates stylesheets.

@vjeux
Copy link
Contributor

vjeux commented Jul 26, 2016

We are still figuring out how to properly manage js styles on the web. For React Native, we are using js styles everywhere and it's been working really well for us.

For web, the current strategy is to generate CSS. The reason is that we have a lot of CSS transforms that we run at bundle time that is unpractical today to reproduce in JavaScript. But this is likely a fb-specific problem.

@quasipedia
Copy link

Thank you @zpao and @vjeux for having got back to me so promptly. It's good and important to have a snapshot of where FB stands right now. A couple of clarifications:

  • What does "we are still figuring out how to properly manage js styles on the web" means concretely? Is js styles something that FB is committed to adopt in the future and actively working on (like: having people thinking about and experimenting with the problem) or is more about "yeah... maybe... eventually... if somebody stumble upon a genius way to do it"?
  • What about support in React for approaches that are not necessary in line with where FB is heading? For example: this very feature request clearly is about something that FB is not using and not planning to use either... how much is FB committed to serve the needs of a community whose usage patterns may not be necessarily in line with that of the corporation sponsoring the project?

I guess the goal by asking the two above really is: should we - as react end users who boldly ventured where no man had gone before ;) - expect support for our usage patterns, or should we fold back and change our architectures to conform to what FB uses for itself? Honest question, given last week' @gaearon' surprise with create-react-app (something of no use for FB but beneficial for the community) ...

@vjeux
Copy link
Contributor

vjeux commented Jul 27, 2016

This is a good opportunity to explain the current situation regarding to styles with React.

When I started working on React Native 2.5 years ago, we needed to figure out a solution for doing styles. At the time we were adding all the styles as props. My thinking was: at some point we'll need to implement a CSS selector engine but right now I'm lazy and want to get started so we'll write styles in js and use inline styles to display them.

It turns out that not only this worked out really well but solved a bunch of problems we had with CSS. So we kept going with that in React Native.

We were going to open source React Native in a few months and I was super worried that this unconventional way of doing styles was going to overshadow the launch like jsx did for React. So I decided to do a talk to explain the current way we write CSS at Facebook and this other crazy alternative.

My goal was just to defuse the drama around the launch, but it also spawned a huge field of research around different ways of writing styles in a React context. I had absolutely not anticipated that coming.

Now that a lot of people were excited about writing styles in js, the question was: should we do it at Facebook? We've thought hard about it but were in a tricky situation.

The current setup we have (cx, which I explain in the first part of the presentation) is very complex to setup and non standard but it solves a large amount of problems that are also solved with styles in JavaScript.

So we are in this situation where changing to styles in js would mean doing a lot of infrastructure work and change the way every fb engineer write code, for maybe a 10% improvement.

So far our conclusion has been that it was not worth the effort, which is very sad but a practical and realistic response. But we will keep evaluating this decision on an ongoing basis and the cost analysis may change in the future.

One promising thing is to be able to share components between React Native and web, which can be a good enough reason to do that investment, we'll see.

I hope it helps.

@quasipedia
Copy link

Hey @vjeux thanks a lot for the extensive reply, kudos to FB that recruits engineers that engage with the community! :) IMO your answer above would make for the core of a great blog post, btw!

@quasipedia
Copy link

I agree it’s not good we don’t have a good solution for auto prefixing multiple values. We could potentially allow this by letting you pass an array for the value. Let’s reopen to hear what others think.

Hi @gaearon @zpao !

The above quote is from 9 April this year: I was just wondering if after half a year you had some update on this proposal, or if any new consideration has come into play that may affect this issue.

Thank you for your awesome work! :)

@gaearon
Copy link
Collaborator

gaearon commented Jan 2, 2018

I don’t see us investing effort into supporting this.

For the past few years, browser vendors have been avoiding using prefixes for new features.
So I understand this is still a bit of a pain, but eventually it will go away:

Historically, browsers have relied on vendor prefixes (e.g., -webkit-feature) to ship experimental features to web developers. This approach can be harmful to compatibility because web content comes to rely upon these vendor-prefixed names. Going forward, instead of enabling a feature by default with a vendor prefix, we will instead keep the (unprefixed) feature behind the “enable experimental web platform features” flag in about:flags until the feature is ready to be enabled by default or exposed for a limited duration origin trial.

— Blink (https://www.chromium.org/blink#vendor-prefixes)

The current consensus among browser implementors is that, on the whole, prefixed properties have hurt more than they’ve helped. So, WebKit’s new policy is to implement experimental features unprefixed, behind a runtime flag. Runtime flags allow us to continue to get experimental features into developers’ hands while avoiding the various problems vendor prefixes had. Runtime flags also make it easier for us to have different default settings between stable builds and preview builds such as Safari Technology Preview.

— WebKit (https://webkit.org/blog/6131/updating-our-prefixing-policy/)

For what it’s worth, the current trend inside Mozilla is […] avoiding vendor prefixes by either turning things off before shipping or shipping them unprefixed if they’re stable enough. At least as a general policy; specific cases might merit exceptions.

— Firefox (http://lists.w3.org/Archives/Public/public-webapps/2012OctDec/0731.html)

Microsoft is also getting rid [of] vendor prefixes for Edge. This means that in order for developers to take advantage of special HTML5 or CSS features, they won’t have to use a specific Edge prefix. Instead, they can just code to web standards.

— Edge (http://mashable.com/2015/05/11/microsoft-edge-security/)


For code that suffers from this today and still relies on prefixes for features that are not quite supported by all browsers yet, I encourage you to either use CSS classes, or create a utility that will pick the right prefix based on what’s supported by the browser. For example you can use document.createElement('div') to create a DOM node, and then assign .style.myPrefixedProperty and see if it “sticks” by reading from it. Whichever sticks is the one you need to use for future code, so you can cache the property name and then keep using it.

This used to be problematic for server-side rendering because React 15 used to compare markup, and you couldn’t pick a specific prefix on the server side. But this isn’t a problem anymore because React 16 allows mismatches between server and client. You can use all prefixes on the server, and then specify suppressHydrationWarning={true} (new in React 16.1.0) to turn off the markup mismatch warning for those cases.

I’m sorry if this isn’t a satisfactory resolution but it just doesn’t seem worth it to change an API and introduce extra runtime checks for something that will be irrelevant in a couple of years.

@gaearon gaearon closed this as completed Jan 2, 2018
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

6 participants