6.x <Modal> component introduces DOM element around children

[mattmazzola]

Environment Information

"office-ui-fabric-react": "6.148.1"
Chrome: Version 72.0.3626.119 (Official Build) (64-bit)
Windows: Version 10.0.17763 Build 17763

Please provide a reproduction of the bug in a codepen:

Just opens a model so you can inspect DOM nothing special.
https://codepen.io/mattmazzola-the-reactor/pen/xBEwrZ

Actual behavior:

The 6.x modal introduces a new DOM element which means the styles we had on the containerClassName selector which applied to the direct children apply to the <div class="ms-Modal-scrollableContent scrollableContent-54" data-is-scrollable="true"> instead. Previously the children of the <Modal> were directly under the containerClass and you could count on this behavior.

In my case the containerClass selector was declaring:
display: grid; which would affect it's 3 children. Since the new DOM element was introduced it made these styles apply to the scrollableContent which didn't make sense.

Expected behavior:

Upgrading from 5.x to 6.x shouldn't require us to change the CSS of our app.
It was my understanding the major version increase was only due to requirement of react 16 and not app level breaking changes.

I believe we could work around this issue by changing our CSS but I think there is probably a more appropriate fix of changing how the framework renders Modals so that it's markup is hidden from the consuming app.

Priorities and help requested:

Are you willing to submit a PR to fix?
No

Requested priority:
Normal)

Products/sites affected:
Conversation Learner

[KevinTCoughlin]

@micahgodbolt is our guidance regarding major version upgrades as they relate to selector specificity published publicly?

[micahgodbolt]

the only API we guarentee are the public interfaces of our components. DOM structure and styling, while we try not to change unnecessarily, are not part of that API and therefor can change even without a major release. This is one of the reasons for moving to the styles interface which creates a contract for how styles are applied.

As for needing to make CSS changes when moving from a major version to another, I think that's perfectly acceptable, as major changes are supposed to have our breaking changes.

[mattmazzola]

Hmm, I understand technical reasons for this answer but from consumer perspective I makes me want to use Office Fabric less.

[...DOM structure changes...] are not part of that API and therefor can change even without a major release

This means we can't freely take up minor versions since they could potentially have these DOM structure changes which would be breaking for our app. It wouldn't violate your API contract but pose immense risk to us.

I think that's perfectly acceptable, as major [version] changes are supposed to have our breaking changes

Yes, I know it is accepted based on SemVer rules, but I thought it was an unintended change since I was under impression it was bumped only for React requirement. You could have also removed the <Modal> component entirely and called it an acceptable breaking change even though it would frustrate all the consumers.

Regardless of the stuff above what's the decision going forward.
Is the DOM element staying or going to be removed in a later version?

[micahgodbolt]

@mattmazzola sorry about the hastey response earlier ( i caught it in passing during the evening ). I agree it's frustrating that our DOM change affected styles you applied via a prop that we provided to you. That's a breaking of trust, and shouldn't happen again.

In this situation there was a bug involving scrolling in modals which was resolved by adding a scrollable section around the content. I agree that it would have been better to have made this change without changing the DOM. That's one annoyance with React in that every time you add something, you add a div.

We will continue to be cautions about DOM changes, and be sure to over communicate when those happen. We recently changed our checkboxes from buttons to inputs, and we recognized that this could certainly affect our customers! There were several emails regarding this change, and we'll consider all of these changes worth communicating.

If you need additional help making this scenario work properly (not in a hacky way) I'd be happy to help drive those changes.

[micahgodbolt]

@kkjeer what do you think about us moving the 'containerClassName' to the scrollableContent div? This appears to be the intention of this className.

[mattmazzola]

Ok, I assume that means the element is staying.

adding a scrollable section around the content

I assume element was added for a reason I just don't know the resaon. Since element is added in all cases around the children it would help me to understand why the scrollable behavior could not be absorbed by the parent div through some type of composition.

[In] React every time you add something, you add a div

Depends on what the "something" is of course, but in my experience the only issue was previous requirement to have a single child for all elements. Since they added <React.Fragment> or <> this isn't an issue anymore and has helped us in some cases. I understand you don't need to tell me why the framework is designed the way it is but if it's going to be a breaking change that affects our apps I would like to now why so when I have to make the changes to accommodate it I can rationalize and also tell rest of my team why it's necessary.

[micahgodbolt]

I agree, we have better tools now with React.Fragment which certainly help the div hell!

Hooks will also make it easier to add functionality without additional wrapping divs (those are coming in Fabric 7).

Without hooks, such composition is currently difficult as we'd need to mix the functionality of two pretty complex controls (focus zone and scrollable pane), but I think there are ways we can support the scrollable scenario with less impact to other users. We'll keep this issue open and explore them.

We could:

1. Move the containerClassName to the scrollable section, thereby keeping the class on the containing element
2. Manually support the scrollable functionality without needing to wrap content in scrollable pane (which might be a bit overkill for this scenario).

[aftab-hassan]

@micahgodbolt Is this an issue we would like to fix using hooks in Fabric 7?

Or would we like to explore other ways to solve this, starting with moving the containerClassName and manually supporting scrollable functionality, you mentioned?

If this change is required in the short term, I can work on it. If not, can we consider adding it to the backlog?

[micahgodbolt]

I think #1 is the better solution here. The expectation was that the 'containerClassName' would wrap the modal content....putting a scrollable pane inbetween breaks that relationship. Let's write up that PR and see if there is other feedback/ideas.

[aftab-hassan]

I think #1 is the better solution here. The expectation was that the 'containerClassName' would wrap the modal content....putting a scrollable pane inbetween breaks that relationship. Let's write up that PR and see if there is other feedback/ideas.

Okay, going with "Move the containerClassName to the scrollable section, thereby keeping the class on the containing element". I'll have that PR out.

[mattmazzola]

I'll have that PR out.

Any update on this?

[aftab-hassan]

@mattmazzola I can take a look later this week. Please check back for updates.

[mattmazzola]

What's the status on this?

[mattmazzola]

@micahgodbolt Did you have any input or update on this? It's been over 2 months since last decision to create a PR for moving the class to the scrollable div but I assume plans have changed. I had been holding off the upgrade to 6.x from 5.x until this was settled. If the class not going to be moved/changed I will do the work to update our app but if the class will move then that work is wasted.

[micahgodbolt]

@mattmazzola Thanks for the bump, I was looking one more time at this problem and wanted to see if it was possible to use the scrollableContentClassName to pass in your grid styles. Seeing as this prop is already passing in a className to the scrollableContent, I don't see value in passing in the containerClassName as well.

[micahgodbolt]

This obviously never got addressed. @mattmazzola were you able to use scrollableContentClassName to address the DOM changes? 2 versions past 5 now, I don't think we're going to make any major DOM changes to the Modal again, as it'll probably break someone expecting this specific DOM.

[mattmazzola]

I ended up making CSS changes to skip over the added DOM element to preserve the effect. Change was something like .modal to .modal > *.
I think using scrollableContentClassName but I couldn't get it at the moment.

Also, originally the scrollable container was the only DOM element added under the rood modal to it could have been easily merged. Now there are two more DOM elements which would make it even more breaking change.

See:

Anyways, I suppose this can be closed. DOM can't change anymore and our product is working.

[micahgodbolt]

Yeah, DOM order is a HARD thing to guarantee. The extra div is part of the 'draggable modal' code. Future controls will work hard to allow for style customization without needing to rely on DOM order/contents.

[micahgodbolt]

Here's a demo of scrollableContentClassName. That "should" be a pretty same CSS anchor for now.

https://codepen.io/micahgodbolt/pen/dyPJwGo?editors=0110

[msft-github-bot]

This issue has been automatically marked as stale because it has marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment. Thank you for your contributions to Fabric React!