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 b1a22f6:

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: 43f3fbf79244dfe2557322b1418f73d93bd1b054 (build)

📊 Bundle size report

_{🤖 This report was generated against 319234c25f29defac6afc728dd109737fd1b803f}

Perf Analysis (@fluentui/react)

No significant results to display.

Perf Analysis (@fluentui/react-northstar)

@microsoft/cxe-red & @microsoft/cxe-coastal can you please review?

ping @GeoffCox as he's working on v8 transition tools and docs

We have an offline sync about this RFC few weeks ago (17th March) with @khmakoto, @levithomason, @ling1726, @miroslavstastny & @GeoffCox.

The summary:

• zIndex
  • To check if these values (1000000) can be changed via props in v8
  • Trace in history when these zIndex values appeared
  • Prototype layer manager to increase zIndex based on parent (DOM or JSX??)
• Context approach
  • Agreed to proceed, no comments ✅

My only two concerns are communication on this and cleanup in the future.

Is it the user's concern to remove the compat context once no longer needed?

Yes, it user's concern. But I think it that it's okay, once customers will stop to use @fluentui/react@8 or @fluentui/react-northstar they will need to perform cleanup anyway (remove providers, entries in package.json files, etc.).

zIndex

• To check if these values (1000000) can be changed via props in v8

No, styles are hardcoded:

These styles will be always added when a layer is not using LayerHost, https://developer.microsoft.com/en-us/fluentui#/controls/web/layer.

• Trace in history when these zIndex values appeared

• zIndexes.ts first appeared in Add Keytips to office-ui-fabric-react #4629, but zIndex: 1000000 was already in Layer.styles.ts
• Layer SCSS to MergeStyles #3979 created Layer.styles.ts, before it was Layer.scss and z-index: 1000000 was there
• The style rule was introduced in Layer: now supports React context passing through, which affects all Layered components. #659 or in [email protected] (!!!)

• Prototype layer manager to increase zIndex based on parent (DOM or JSX??)

@GeoffCox AFAIR it was yours proposal. Can you please share ideas and goals behind it?

For example, elements with equal values zIndex will be handled by DOM order i.e. a component that will be later mounted - wins.

Another issue, that any smart handling might be a huge breaking change for customers (see the history, these values were there for ages). But without changes in v8/v0 I don't see a way how it could be integrated as we can't rely on DOM order:

<!-- Elements are not nested, they are siblings -->
<div class="ms-Layer"></div>
<div class="fui-Popup"></div>

Increasing zIndex based on a value of sibling does not sound like a great approach. For example, a change in DOM structure and it's broken:

<div>
  <div class="ms-Layer"></div> <!-- ⬅️ z-index is there ->
</div>
<div class="fui-Popup"></div>

This might not be a case for v8, but it's true for v0.

zIndex

@layershifter The problem we are trying to solve is that portals are stacked on top of each other when nested right ?

<V8Portal>
  <v9Portal />
<v8Portal>

In this case we should make sure that the v9 portal renders over the v8 portal.

V8 already has a mechanism to modify z-index for layers (Link to docs)

<Callout layerProps={{ className: overrideZIndex }}  />

In this case I propose 2 ways of going forward

portalProps (naming not binding) for components

Our components that use portals should expose a similar portalProps prop that is typed consistently across all the components that will render portals

<Popover portalProps={{ className: overrideZIndex }} />
<Tooltip portalProps={{ className: overrideZIndex }} />
<Menu portalProps={{ className: overrideZIndex }} />

Compatware for v9

We can continue the idea of compat context for v9 to set zindex

<V9PortalCompat zIndex={1000000}>
   <V9Portal />
<v9PortalCompat />

Probably out of topic of this particular RFC, but regarding z-indexes on root elements:

It should be avoided at all cost . instead we can leverage isolation. That would mitigate those z-indexes battles in a convenient way.

No z-index, no problem:

Not sure that adding another property to our components API to basically "hack/workaround" things is the proper way forward.

Probably out of topic of this particular RFC, but regarding z-indexes on root elements:

It should be avoided at all cost . instead we can leverage isolation. That would mitigate those z-indexes battles in a convenient way.

No z-index, no problem:

Not sure that adding another property to our components API to basically "hack/workaround" things is the proper way forward.

Sorry, but I didn't get how isolation could help with existing zIndex values in @fluentui/react@8. Can you please clarify?

Sorry, but I didn't get how isolation could help with existing zIndex values in @fluentui/react@8. Can you please clarify?

right, as I mentioned this is probably Out of topic for this RFC, but as you started convo regarding z-indexes I wanted to add my POV on the topic. this solution would "help" I guess only if we changed the css definition in v8, but I guess that's a no go.

hope it's clear now

@layershifter The problem we are trying to solve is that portals are stacked on top of each other when nested right ?

Correct, effectively it's this case (modified CodeSandbox). I don't see any other solution than applying the same zIndex for v9 Portals.

Summary

• There are no concerns about React Context part and it's being implemented in feat: add portal compat implementation to v8 & v0 #22541
• There are no other solution as apply the same zIndex for v9 Portal as in Layer from v8. This is a legacy part that is impossible to change. Ability to customise zIndex (RFC: Portal compatibility #21774 (comment)) is also a valid requirement, but it's out of scope of this RFC. I will create a separate one to standardise the approach (similarly to positioning prop).

This RFC has enough approvals and if there will be no additional input I am going to merge it on Wednesday. Cheers 🍻