[TextField] Update labelWidth at each render #17583
Conversation
Details of bundle changes.Comparing: 274aeb8...29c2da1
|
|
Thanks for working on this. Could you explain what you're trying to fix? Preferably with a test case or a codesandbox to illustrate the issue? |
oliviertassinari
changed the title
oliviertassinari
added
bug 🐛
| @@ -93,13 +93,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(() => { | |||
As far as I know At least add a test that only runs in the browser (that's why we have them) so that we can swap out the implementation easily. |
|
@eps1lon I have looked at the performance cost of this reflow. I have used one controlled text field component in dev mode. After 8 change events, I had 2ms spent in this reflow vs 24ms of rendering and 93 ms of scripting (will be less in prod). Early results seem to indicate the overhead is negligible. Would you confirm? |
|
@neon98 it seems that you have deleted your forked repository |
|
@oliviertassinari yes! would it be a problem? |
|
@neon98 It means that this PR can't be revised. |
|
Yeah this is a scary change. 10% of rendering time in dev is not negligible in my book but if you say so then I won't object. |
|
With more hindsight, I'm not sure what path we should take with this issue. I have looked more closely at the performance impact, with more samples. The "recalculate styles phase" introduced by this logic costs between 8% and 3%. Interestingly the TextareaAutosize has also a reflow logic that seems to cost more. Also, if somebody provides a rich label, the reflow is present (but not forced, users can dodge it with memoization). Maybe it's not important, is the brute force approach fast enough? They are two observable APIs that we could potentially leverage IntersectionObserver and ResizeObserver but they are not supported everywhere. No matter what, we need a new limitation section in the text field documentation for SSR: #17305 (with a background color on the label that matches the main background). @eps1lon What would you recommend? |
|
Alright, I'm closing. I think that we should focus on a limitation docs update and wait to get more upvotes for the problem in #17355. @neon98 Thank you for taking the time to open a pull request. If you want to continue the topic, you could focus on #17305. Otherwise, we are handling, what I think is close to an edge case here, it's not incredibly important. |
Closes #17355