Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TextField] Outlined label gap is missing when first rendered hidden #17355

Closed
2 tasks done
EliteMasterEric opened this issue Sep 8, 2019 · 26 comments · Fixed by #17680
Closed
2 tasks done

[TextField] Outlined label gap is missing when first rendered hidden #17355

EliteMasterEric opened this issue Sep 8, 2019 · 26 comments · Fixed by #17680
Labels
bug 🐛 Something doesn't work component: text field This is the name of the generic UI component, not the React module!

Comments

@EliteMasterEric
Copy link

  • 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 😯

When I place a TextField with an outline within a SwipeableViews tabbed view, the notch within the outline that provides space for the label does not appear on tabs other than the first one. The notch appears on TextFields in the initial tab.

Expected Behavior 🤔

Outlined TextFields on secondary tabs should have a properly sized notch to contain the TextField's label.

Steps to Reproduce 🕹

Steps:

  1. Place a TextField within a tab SwipeableView, with an outline.
  2. Open the page. The TextFields on the starting tab should have notches in the outline.
  3. Switch tabs. Tabs besides the initial one will have no notches in the outline.

I have reproduced this issue on a minimal CodeSandbox: https://codesandbox.io/s/mutable-butterfly-36u26

Context 🔦

I encountered this issue attempting to place a TextField within a tabbed view.

SwipeableViews is not part of this project, but it is used alongwide Material UI in the official documentation: https://material-ui.com/components/tabs/#fixed-tabs.

Your Environment 🌎

Tech Version
Material-UI v4.4.0
React v16.8.6
Browser Firefox 69.0, Chrome 76.0.3809.132
@oliviertassinari oliviertassinari added the component: text field This is the name of the generic UI component, not the React module! label Sep 8, 2019
@oliviertassinari
Copy link
Member

@mastereric Thank you for taking the time to fill the issue template! We had a similar, incomplete report in the past. I would propose that we apply the same solution that we have been using with the tabs component so far:

  React.useEffect(() => {
    updateIndicatorState();
    updateScrollButtonState();
  });

In the case of the outlined text field, this translates into:

diff --git a/packages/material-ui/src/TextField/TextField.js b/packages/material-ui/src/TextField/TextField.js
index efa52da765..9820b1a5a0 100644
--- a/packages/material-ui/src/TextField/TextField.js
+++ b/packages/material-ui/src/TextField/TextField.js
@@ -94,13 +94,18 @@ const TextField = React.forwardRef(function TextField(props, ref) {

   const [labelWidth, setLabelWidth] = React.useState(0);
   const labelRef = React.useRef(null);
-  React.useEffect(() => {
+
+  const updateOutlinedLabel = React.useCallback(() => {
     if (variant === 'outlined') {
       // #StrictMode ready
       const labelNode = ReactDOM.findDOMNode(labelRef.current);
       setLabelWidth(labelNode != null ? labelNode.offsetWidth : 0);
     }
-  }, [variant, required, label]);
+  }, [variant]);
+
+  React.useEffect(() => {
+    updateOutlinedLabel();
+  });

   warning(
     !select || Boolean(children),

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work good first issue Great for first contributions. Enable to learn the contribution process. labels Sep 8, 2019
@neon98
Copy link
Contributor

neon98 commented Sep 10, 2019

Hey @oliviertassinari ! I'd love to work on this issue.

@oliviertassinari
Copy link
Member

@neon98 Sure, I think that it's a good one to take :).

@oliviertassinari oliviertassinari changed the title Outlined TextField Notch is missing in a Tab view Outlined TextField Notch is missing when first rendered hidden Sep 27, 2019
@oliviertassinari oliviertassinari added waiting for 👍 Waiting for upvotes and removed good first issue Great for first contributions. Enable to learn the contribution process. waiting for 👍 Waiting for upvotes labels Sep 27, 2019
@oliviertassinari
Copy link
Member

Note, we have tried to solve the problem in #17583 but give up because of the reflow concern: #17583 (comment).

@dnavamosler
Copy link

@mastereric Thank you for taking the time to fill the issue template! We had a similar, incomplete report in the past. I would propose that we apply the same solution that we have been using with the tabs component so far:

  React.useEffect(() => {
    updateIndicatorState();
    updateScrollButtonState();
  });

In the case of the outlined text field, this translates into:

diff --git a/packages/material-ui/src/TextField/TextField.js b/packages/material-ui/src/TextField/TextField.js
index efa52da765..9820b1a5a0 100644
--- a/packages/material-ui/src/TextField/TextField.js
+++ b/packages/material-ui/src/TextField/TextField.js
@@ -94,13 +94,18 @@ const TextField = React.forwardRef(function TextField(props, ref) {

   const [labelWidth, setLabelWidth] = React.useState(0);
   const labelRef = React.useRef(null);
-  React.useEffect(() => {
+
+  const updateOutlinedLabel = React.useCallback(() => {
     if (variant === 'outlined') {
       // #StrictMode ready
       const labelNode = ReactDOM.findDOMNode(labelRef.current);
       setLabelWidth(labelNode != null ? labelNode.offsetWidth : 0);
     }
-  }, [variant, required, label]);
+  }, [variant]);
+
+  React.useEffect(() => {
+    updateOutlinedLabel();
+  });

   warning(
     !select || Boolean(children),

no work for me

@dnavamosler
Copy link

any solution?

@oliviertassinari
Copy link
Member

Do you guys experience the issue with the Tabs demos?

@mui mui deleted a comment Oct 17, 2019
@oliviertassinari oliviertassinari added component: tabs This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process. hacktoberfest https://hacktoberfest.digitalocean.com/ and removed waiting for 👍 Waiting for upvotes labels Oct 17, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 17, 2019

While we have a broader issue with the outlined TextField and the notch for the label, I propose that we focus on the tabs demos. I think that we can improve performance and dodge this problem with the following fix (example with one tab demo, to apply to all of them).

We can replace:
https://github.com/mui-org/material-ui/blob/5d564f9c1be5bf20b51be1a479d292bf443291ba/docs/src/pages/components/tabs/SimpleTabs.tsx#L18-L28

with

return ( 
   <Typography 
     component="div" 
     role="tabpanel" 
     hidden={value !== index} 
     id={`simple-tabpanel-${index}`} 
     aria-labelledby={`simple-tab-${index}`} 
     {...other} 
   > 
     {value === index ? <Box p={3}>{children}</Box> : null}
   </Typography> 

Does anyone want to give it a try?

@EliteMasterEric
Copy link
Author

Hey @oliviertassinari,
It appears that your latest solution, which deletes the contents of the tab when you switch away from it, fully fixes the rendering issue, as shown in this demo:

https://codesandbox.io/s/notch-bug-demo-fix-0ooj8

The only issue is that in the full application, this destroys components I am storing references for, and thus I will need to adapt the solution somewhat.

@oliviertassinari
Copy link
Member

@mastereric Thanks for the feedback, so I think that we should apply these changes.

@joshwooding joshwooding removed the hacktoberfest https://hacktoberfest.digitalocean.com/ label Nov 3, 2019
@MomoRazor
Copy link

Hi there, I'm not sure if I should open up a new issue, but basically I'm having the same exact issue, with the difference that I'm not using SwipeableViews, but rather a custom made hiding navigation system that I'm leveraging from a theme I bought. Should I create a new issue with my specific environment? or should I simply put more information here? Thanks.

@oliviertassinari
Copy link
Member

@MomoRazor Same issue and workarounds.

@oliviertassinari oliviertassinari removed the good first issue Great for first contributions. Enable to learn the contribution process. label Nov 30, 2019
@oliviertassinari oliviertassinari changed the title Outlined TextField Notch is missing when first rendered hidden [TextField] Outlined label gap is missing when first rendered hidden Nov 30, 2019
@oliviertassinari
Copy link
Member

We have found a solid solution in #17680. It should do it.

@oliviertassinari oliviertassinari removed the component: tabs This is the name of the generic UI component, not the React module! label Nov 30, 2019
@oliviertassinari oliviertassinari added priority: important This change can make a difference and removed priority: important This change can make a difference labels Nov 30, 2019
@U-4-E-A
Copy link

U-4-E-A commented Dec 2, 2019

I am having a similar issue where I have a multi-tab form with various TextField components on it. The label gap is missing on any tab that is not the first visible tab.

@Virius
Copy link

Virius commented Dec 16, 2019

any updates?

@incepter
Copy link

I was about to create the same exact issue.
The problem is element.offsetWidth returns 0 when the element is in an invisible tree of elements.
The only reliable solution i found is that we should take the innerText of the label, mount it in a visible span (may be at the end of body) take it offsetWidth and then remove it, the problem of this approach is that we should be aware of the used font styles... ).

@incepter
Copy link

I did a useInvisibleLabelWidth hook, and it's working in my use cases and working great. Let me know if interested. (I will create a small npm package for it so we can start).

@Jrocam
Copy link

Jrocam commented Dec 18, 2019

I did a useInvisibleLabelWidth hook, and it's working in my use cases and working great. Let me know if interested. (I will create a small npm package for it so we can start).

Amazing 😄 Link please when you have it.

@incepter
Copy link

incepter commented Dec 18, 2019

Here you go.
This is a working example on code sandbox.
I created a small package react-invisible-element. for now it only exports useInvisibleRefWidth whish returns the label width we need for our outlined variants.

  const inputLabel = React.useRef();
  const labelWidth = useInvisibleRefWidth(inputLabel);
  return (
     <div className="App">
       <div style={{ display: "none" }}>
         <TextField
           variant="outlined"
           value={value}
           label="Input label with issue OK"
           onChange={onChange}
           InputProps={{
             labelWidth
           }}
           InputLabelProps={{
             ref: inputLabel
           }}
         />
       </div>
     </div>
   );

The hook is just a poc for now! I believe it contains a lot of flaws and can be useful.

@mbrookes
Copy link
Member

@incepter Once it's stable, we could potentially use your approach to fix the issue natively.

@incepter
Copy link

incepter commented Dec 19, 2019

@mbrookes I think we can start with this.
I rewrote the react-invisible-elements into react-offsets which returns both the offsetWidth and offsetHeight.

The usage now is:

import useNodeOffsets from 'react-offsets';

const myPossibleHiddenElementRef = React.useRef();
const myElementOffsets = useNodeOffsets(myPossibleHiddenElementRef);
const { offsetWidth, offsetHeight } = myElementOffsets;
<Component ref={myPossibleHiddenElementRef} />

So if we rewrite Textfield to another one that may exist in an invisible tree:

function NewTextField(props) {
  let inputLabelRef = React.useRef();
  if (props?.InputLabelProps?.ref) {
    inputLabelRef = props.InputLabelProps.ref;
  }
  const offsets = useNodeOffsets(inputLabelRef);
  return (
    <TextField
      InputProps={{
        ...props.InputProps,
        labelWidth: offsets.offsetWidth,
      }}
      InputLabelProps={{
        ...props.InputLabelProps,
        ref: inputLabelRef,
      }}
      {...props}
    />
  );
}

The same applies for the Select component.

I used this code with a CPU throttle of 6 time slower than my laptop and I did not see the element created into dom (even with a style no one will be missing (color, big font size and max zIndex))

Here is the poc sandbox

UPDATE: I will add deps array to the hook to trigger the effect at needs. ie: useNodeOffsets(ref, [value, label....])

Please note that I am started using this package in my project.

@incepter
Copy link

incepter commented Dec 19, 2019

I tried to change in the TextField component, the changes would look like this.

All Mui tests are passing, but I am not sure if this approach could be used natively.
Is there any contribution guides ? How can I test the new component edits locally ?

image

@mbrookes
Copy link
Member

mbrookes commented Dec 19, 2019

Is there any contribution guides ? How can I test the new component edits locally ?

https://github.com/mui-org/material-ui/blob/master/CONTRIBUTING.md#testing-the-documentation-site

@Surm4
Copy link

Surm4 commented Dec 29, 2019

I just render this item with height: px in the first tab. Then it loads label width correctly.
Same problem happens in '@material-ui/core/MobileStepper' component

@danielgek
Copy link

@oliviertassinari i see there already a pr for this #17680, but it's been left out in the last few releases, is this something that the team is planning on fixing soon ?(i can help on pr if nedeed)

@oliviertassinari
Copy link
Member

@danielgek Thank you for proposing your help. @eps1lon leads this effort. Given that the text field is, with the button, the most visited component of the documentation, I think that we should increase the priority of this resolution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: text field This is the name of the generic UI component, not the React module!
Projects
None yet