Fast: Fragile style specificity in FAST Microsoft components

[janechu]

Describe the bug; what happened?

Currently there are composed components in the fast-components-react-msft package that add classes to other components. This creates a specificity issue because we are relying on the rendering order of a components style in the DOM for style overrides. If a component used by another component is rendered on a higher level, this will cause the order to be incorrect.

What are the steps to reproduce the issue?

Example:

1. Create a Typography component
2. As a sibling of the Typography component, create a Heading component
3. This should render the classes for the Typography component before the Heading component therefore creating a font size issue.

What behavior did you expect?

I expect no matter where the components are used and in what order they render that the styling should be consistent.

Is there any additional context?

A possible solution for the exported packages is to duplicate the stylings and only rely on the base components possibly by extending the class name contract.

[shauntc]

We've encountered this issue on MSN and I think it needs to be marked as high priority

From my investigation the style precedence is based on render order of styled components, this would work fine except that styles are also be de-duped by the sheet registry.

This causes an issue when a fast-dna styled component is passed a CSS class which overrides its default style class from its parent component. If the styled component is re-used before the parented one then the default style class will have precedence over the passed in CSS class.

Some of options I can see for dealing with this :
1) Remove the de-duping of styles
- +Simplest implementation
- -Potentially large performance hit, getting worse with size of the site
2) Give fast-dna component styles the lowest precedence
- +Fairly simple implementation
- -Only fixes the problem for fast-dna components
3) Create a set dependency graphs to de-dupe based on
- +Most robust solution
- -Complex solution
- -Complex implementation
- ~Performance implications are unclear
- ~Unclear how much de-duping could be determined
4) Expose setting of the style index
- +Simple implementation
- -Moves the difficulty to the users of the library
- -Maintaining the current impl would mean this is also non-deterministic
5) Remove injectable styling from styled components
- +Simple implementation
- -Breaking Change
- -Feature regression

Ultimately the problem is that render order is not a deterministic way to set precedence so either it needs to move to something that will always be deterministic, or handle the complexity, or remove the handling from the library

[nicholasrice]

Thanks @shauntc, this is a great summary.

Regarding your first point, that was the original implementation of the library and we did see large performance impacts that were ultimately unacceptable. To fix that, de-duping was introduced though that is obviously causing it's own issues.

I'm intrigued by using a dependency graph to determine style element order, but I agree there are unknowns there.

Aside from a dependency graph, I'm starting to think manual indexing may be the most viable solution. CSS specificity has always been tedious to manage, so providing a clear mechanism for authors to apply and preserve component style element order addresses the issue in a manner consistent with managing CSS itself. We would still need to determine initial order though, which I believe is reverse call order in JSS (not 100% deterministic).

I'm not sure I'm following your fifth point - can you clarify that one?

[shauntc]

@nicholasrice the fifth one is removing the capability of setting an extra class name on already styled components thus there is only one class per component and no precedence issue, it is another simple but incomplete solution, only for the styled fast components

I figured that was the case with the first solution, and I agree that the dependency graph and exposing the index are the two most long-term solutions. Personally I would favor the dependency graph, being a library of reusable components, handling the complexity of style ordering would be a much better experience for the user and I think it could be built without changes to the Api of the library.
React's context api could be used to generate the tree from inside the JSSManager. I created a super rough basic proof of concept of generating depth/id paths for the styles which could be handed to the sheet manager to create the dependency tree.

Whichever solution is decided on I think temporarily implementing 2 would be a good quick solution. Forcing lower precedence on the fast component styles as they shouldn't need to override other styles

[nicholasrice]

Gotcha - yea I don't think we would want to remove that ability because an author may need to add any arbitrary class to a managed component for purposes we can't know, and it prevents overriding where the selector may have enough specificity regardless of style-element order.

Thanks for the proof of concept - I'll take a peek at that today.

[shauntc]

@nicholasrice Any updates on this?

[nicholasrice]

@shauntc No - I haven't had a chance to make progress on this.

[nicholasrice]

My hope was that component dependency tracking and resolution of a sheet order based on that data would prove viable, but I was unable to get a working prototype during my exploration. While I still feel this could be the most developer friendly solution, the following issues are preventing me from recommending it at this time:

1. It would require changes to the JSS library itself - JSS does not currently support moving a style element after it has been created - this would mean short term we would need to remove and re-create style elements in the new order.
2. Reconciliation would be frequent. When any styled component mounts or unmounts, we would need to validate the order is still correct and adjust if necessary. We would be paying this tax frequently and generally un-necessarily since the issue is somewhat edge-case.
3. Circular dependencies by nature - it is possible and viable to have component chains A -> B and B -> A, which means the issue can still repro (though likely rare).
4. General complexity – the solution is complicated and expensive

Another solution proposed by @shauntc is to enable memoization based on incoming class-names. Because the issue only repros when attempting to override a component's styles with classes, I think this solution is viable. It would, however, increase the number of style elements being created, which can negatively impact performance.

A third solution is to enumerate style order manually. This is surely the most deterministic method and gives authors complete control over sheet order, but does require manual management of indexes.

To address the issue, I'd like to propose a three-part solution.

1. Expressive documentation of the issue. Informing app authors of how and why this issue repros will go a long way to helping them avoid the issue in the first place.
2. Implement opt-in class-name memoization. By default, class-name strings will not be used to memoize stylesheets. We will add configuration to enable changing that behavior on an instance basis through props or for all JSSManager instances via a public member on the JSSManager class.
3. Add optional manual enumeration of style index. FAST-DNA will add static ordering to all FAST components so that ordering of those is predictable and determinate. It would be recommended for app authors to also do the same, but if not provided then the order would be determined using the existing algorithm. Come a major version change, we could update the indexing algorithm to be more deterministic (use component creation order rather than render order) to further reduce the spontaneity of the repro.

[janechu]

This seems like a good compromise.

The most troublesome part of this issue, in my mind, is the lack of documentation on why it happens, which if known can be largely mitigated.

I also think the approach towards granular control on ordering the styles is an acceptable requirement since that is something that those authoring CSS should already familiar with.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[chrisdholt]

closed by #3517