[core] The hidden prop does not always visually hide elements

[shtengel]

Im using the hidden prop of Container to show only the current Tab and to hide the rest (copied from the Tabs example)
was working fine until i upgraded to version 4.9.9 from version 4.9.1 (could be higher version but i think 4.9.1)

• The issue is present in the latest release.
• I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

<Container hidden={tabIndex != 1}> </Container>

The Container is not hidden even though tabIndex = 1

Expected Behavior 🤔

Container should not be visible

Steps to Reproduce 🕹

<Container hidden={tabIndex != 1}>sdfsdfsdfds</Container>

https://codesandbox.io/s/empty-browser-n8fwh?fontsize=14&hidenavigation=1&theme=dark

• Works on 4.9.1 *

Steps:

1. upgrade to version 4.9.9
2. use a Container with hidden prop as True

Tech	Version
Material-UI	v4.9.9
React	v16.12.0
Chrome

[eps1lon]

I think this is a general issue with overriding display. Setting a default display means that the user agent styling for [hidden] will be overridden.

[oliviertassinari]

@shtengel What's the use case for using hidden, why not changing the styles?
I can count the usage of display: block; at least 10 times in the other MUI components.

[eps1lon]

What's the use case for using hidden, why not changing the styles?

hidden has different semantics. It's also a web standard so supporting that isn't really something that needs to be questioned.

[oliviertassinari]

hidden has different semantics. It's also a web standard so supporting that isn't really something that needs to be questioned.

https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/hidden. We had a pass discussion on the topic in #15178, luckily, we solved the issue without even trying :).

However, we also have these components to consider: AppBar, Avatar, Badge, ButtonBase, BottomNavigation, Table, Stepper, ListItem, Switch, StepIcon, Slider, InputLabel, CircularProgress, CardMedia, CardActionArea, and SvgIcon that ignore the hidden attribute (I have likely missed a couple others).

[shtengel]

@oliviertassinari what do you mean changing the style?
i am using hidden to hide and show Tabs
if you go to https://codesandbox.io/s/empty-browser-n8fwh?fontsize=14&hidenavigation=1&theme=dark
and change the material-core version to 4.9.1 you'll see that the hidden feature works fine.
its somewhere between 4.9.1 and 4.9.9

[oliviertassinari]

@shtengel Why are you using the hidden prop over the alternatives? For instance, instead of

<Container hidden={{A ? true : undefined }} />

did you consider using

<Container sx={{ display: A ? 'none' : undefined }} />

or probably the best option:

{A ? <Container /> : null}

?

[shtengel]

@oliviertassinari Not a major reason, just according to here: https://reactjs.org/docs/faq-styling.html
CSS classes are generally better for performance than inline styles so i try to avoid using style wherever i can. if Container is using styles then it won't make much difference

[oliviertassinari]

In https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/hidden, we have:

Appropriate use cases for hidden include:

1. Content that isn't yet relevant but may be needed later
2. Content that was previously needed but is not any longer
3. Content that is reused by other parts of the page in a template-like fashion
4. Creating an offscreen canvas as a drawing buffer

Inappropriate use cases include:

5. Hiding panels in a tabbed dialog box
6. Hiding content in one presentation while intending it to be visible in others

I think that we should ignore 2. 3. and 4. I think that we should also ignore the use case to only set a node invisible (encourage inline style or CSS instead).

I have a concern with 1. and especially with React suspense: oliviertassinari/react-swipeable-views#453 (comment), it might be important.

On the other hand, a. I haven't seen any progress of the hidden prop in React, b. developers could always add an extra DOM node, c. what's the overhead of supporting hidden (we have 30+ components to update)?

Out of this tradeoff equation, I'm leading toward recognizing the problem (it exists) but ignoring it (we have more to lose) => won't fix.

@eps1lon What do you think?

[shtengel]

@oliviertassinari im using the hidden prop as suggested here https://material-ui.com/components/tabs/
but instead of <Typography> i decided to use <Container>

[oliviertassinari]

@shtengel Oh, ok, perfect, so it's simply a leftover from 0369bfc. I will update it, so you don't see this pattern anymore.

[shtengel]

@oliviertassinari Ok :) Thanks for the quick reply!
so to use style instead of hidden?
or to wait for new version of material-ui/core

[oliviertassinari]

so to use style instead of hidden?

@shtengel I recommend you to use <Container style={{ display: A ? 'none' : undefined }} /> or {A ? <Container /> : null} unless you have a good reason to use hidden instead.

[MartinDevillers]

Ran into this gotcha today. First, the hidden attribute has nothing to do with MUI or React, but is a default HTML attribute from the web standard. Second, the hidden attribute is overridden if a display: ... style is applied to the same element. It doesn't matter if these styles are inline or coming from CSS (the latter surprises me the most since I'd expect the hidden attribute to take precedent over any global CSS). As a result, the hidden attribute is "ignored" by any MUI component that happens to set the display attribute.

You can try this on any of the examples from the official docs. For example, playing around with CodeSandbox I see hidden work on Card, CardContent and Typography, but ignored on CardActionArea, CardMedia, CardActions and Button.

So yea, considering the above it's best to avoid the hidden attribute as its behavior is different on a per-component basis. Moreover, if in any next version of MUI a component is updated to use display internally, it'll change the semantics of hidden for that component and break stuff in existing code bases.

[eps1lon]

Re-opening since #20648 did not address the actual issue

[oliviertassinari]

@topazbarziv Regarding #28460, do you have thoughts on #20452 (comment)?

[topazbarziv]

@oliviertassinari A little bit strange way, but good enough. Thank you.