Add support for using !important in inline style values

[necolas]

Proposed solution for supporting the !important priority for inline styles (#1881). I couldn't write a test for this change; it looks like jsdom doesn't fully support the browser APIs being used. Please let me know if you've got any ideas.

I used RNW's early-warning benchmarks to check how this change affects the inline-styles timings in Chrome. I couldn't see any difference when styles don't use !important. There is a 10-15% increase in time taken when every node in the tree (>600) has at least one style value using !important and going through the new code path. Overall it looks like it avoids introducing a performance regression for existing using of style while enabling use of !important if needed.

A micro-optimization PR for hyphenateStyleName in fbjs helps a little too.

(This patch is related to #12179, which adds a warning when !important is used.)

[aweary]

I used RNW's early-warning benchmarks to check how this change affects the inline-styles timings in Chrome. I couldn't see any difference when styles don't use !important

@necolas is there any way you could publish the benchmarks with this change? I'm curious how it performs on lower-end mobile devices as well.

[necolas]

No problem: https://necolas.github.io/react-native-web/benchmarks/react-patch/

I've tested on a Moto G4 and a 2011 MBP so far.

The React implementation is React 16.2 + this patch.

The inline-style implementation is using plain inline styles. For a baseline reference, you can run the same inline-style test using React 16.2 here. The results look the same to me.

The inline-style-important implementation is exactly the same as inline-style, except every style value includes the !important priority. That's about 630 elements setting 12-16 style properties each. Pretty extreme and takes >50% longer to complete the mount tests (I'm assuming style.setProperty is a slower browser API), but it renders correctly.

[necolas]

Do you think this feature will make it into React 16.3?

[bvaughn]

This change looks reasonable to me.

I wish there was a way to test it, but I jsdom doesn't seem to support it properly as you say.

[bvaughn]

I'll defer the final decision on this to @sophiebits because I am not familiar with this part of the codebase.

@gaearon has expressed concerns that this could be considered a breaking change. (Personally, I feel it's more of a bug fix but I also see his POV.)

@sebmarkbage may also have opinions about this wrt future CSS APIs. (He's on PTO though so I wouldn't expect a response from him at the moment.)

[aweary]

The benchmarks and implementation look good to me 👍

[gaearon]

To clarify what I meant a little bit.

A bug fix and a breaking change are not mutually exclusive. Sometimes we release a breaking bugfix in a patch. But this is rare and we only do this when it worked just a minor or a patch ago, and we know we recently messed up and the cost of not patching it up is high.

In this particular case the updates never worked, and the initial render worked for the last time in 0.14. So this hasn’t worked at all for about two years. At this point I am confident there is code in the wild that depends on this being broken.

For this reason I don’t think we can consider it a non-breaking bugfix. It doesn’t solve anything that hasn’t already been broken for two years. So in my view releasing this in a patch or minor (and definitely breaking people) is more disruptive than our usual workflow for breaking changes (add a warning that behavior will change, then change it in the next major).

All of the above assumes we actually want to go with this API. I think this might be a good candidate for an RFC first. It’s not clear to me which alternatives were considered and why they were rejected. The DOM API doesn’t search a string—perhaps there is a good reason for this, and we shouldn’t either?

[gaearon]

See above

[aweary]

All of the above assumes we actually want to go with this API. I think this might be a good candidate for an RFC first. It’s not clear to me which alternatives were considered and why they were rejected.

The primary hesitation according to #1881 was performance and IE8 (#1881 (comment)). We don't support IE8 and @necolas' benchmarks look good so I think the main question now is whether this is the right API. The only alternative I've seen proposed is using an object like { value: 'blue', important: true}. An RFC sounds good.

I'm 👍 for saving this for a major release.

The DOM API doesn’t search a string—perhaps there is a good reason for this, and we shouldn’t either?

Can you clarify here, what DOM API?

[necolas]

I think using an RFC feels like overkill for what is a fix to an old regression. I understand the reasons why it could be considered a breaking change and delayed until React 17, although I'm not sure if the argument is that styling bugs are a special case or that fixing old bugs is always a breaking change (ref #6411). I'll look again at work arounds in RNW or leave it broken and flag the surface where this is an issue for the library. Thanks :)

[gaearon]

For #6411, the surface area is vastly smaller IMO. But it would be interesting to first add a warning, and then see how often it fires at FB.

[necolas]

#12179 is a patch to add a warning. Would you like me to reopen it or would you prefer to try it internally on www dev?

[necolas]

Closing this due to lack of maintainer interest

[gaearon]

FWIW I don't think we're opposed to supporting this feature per se. It's just that it's not clear a string-based API is the best one here.

[necolas]

For such a little feature, designing a new object API seems like it would introduce even more overhead in the areas you are concerned about - DX, bundle size, render time. It's the browser API that's the slow part, and any React API is going to have to use it with the current way styles are applied. And if React doesn't want to support important at all, that's also fine. Either way, I have to look into avoiding important for now.

[gaearon]

The part that feels wrong to me is that everybody has to pay for an indexOf lookup that nobody uses today (because the feature doesn't exist) and won't be commonly used in the future.

[sebmarkbage]

The string search perf angle is valid. It can become significant for many large URLs.

The XSS issue is another one. I don't see an immediate problem but the overlap of SSR and string manipulation is a tough space and might require an even more expensive validation.

[necolas]

Yeah I was concerned about that, but there was no indication of an issue in my tests, and you've already introduced a cost like that for custom properties. Everyone would also have to pay a cost if you introduced object values in style. So if any kind of cost in unacceptable you may as well say that this feature will never be supported and add a dev warning to the library. My 2¢

[sebmarkbage]

Type checking again a value is really fast though typically have to be done regardless at some point in the optimization pipeline (even if this was native/wasm).

I felt like the custom properties thing was motivated by that we probably actually want to change the API to only support the setProperty path which is a major breaking change but more likely to be in line with future direction of CSS such as Typed CSS OM. So eventually we'd get rid of that.

[necolas]

IIRC, setProperty is significantly slower than the other (less complete) inline styles API in the browser. Probably why you've been avoiding it unless required, e.g., for custom properties (or !important)

The XSS issue is another one.

👍 hadn't thought about XSS

[sebmarkbage]

In our measurements setProperty has been significantly faster, which is another reason to do it. Do you have a benchmark in mind? Perhaps it's something we're doing.

[necolas]

At first, I looked into only using setProperty for this patch too. That API requires that style properties be converted to the CSS hyphenated form (which is also why I looked at adding memoization to the hyphenation helper you have in fbjs, but it didn't make a noticable difference).

When running the RNW perf regression tests modified to assess this patch, I noticed that using setProperty for all styles (plus converting every property to the hyphenated form) was much slower than the current implementation. Trying it again now (on a 2011 MBP), the time spent in script during rendering of "Mount deep tree" with inline-styles increases from ~43ms to ~66ms (+53%).

[sebmarkbage]

Ah. The plan was to not convert every property to the hyphenated form but instead require people to specify the hyphenated form in their code. That is the breaking change.