material-ui
material-ui copied to clipboard
mui
v5 LoadingButton Crashes React when Chrome has Translated the Page
- [x ] The issue is present in the latest release.
- [x ] I have searched the issues of this repository and believe that this is not a duplicate.
Current Behavior 😯
React crashes when changing the loading state of a LoadingButton when Chrome has translated the page:
Uncaught DOMException: Failed to execute 'insertBefore' on 'Node': The node before which the new node is to be inserted is not a child of this node.
Steps to Reproduce 🕹
- Set up Chrome languages to use a non english language and enable translation from english (https://support.google.com/chrome/answer/173424)
- Go to: https://next.material-ui.com/components/buttons/
- Scroll down to "Toggle the loading switch to see the transition between the different states" (it should be written in whatever language the page has been translated to)
- Toggle the loading toggle, the demo will crash
Found an explanation: https://github.com/facebook/react/issues/11538#issuecomment-390386520
There are a few solutions to fix this:
- Add a container
divoutsidechildreninLoadingButton - Add
spanon text nodes in LoadingButton examples in docs - Add
<html translate="no">tohtmltag
I guess this will be more common now that we removed a bunch of span wrappers that we only had for styling purposes (e.g. https://github.com/mui-org/material-ui/pull/26666).
Turns out these make the components resilient to google translate.
We can never fully fix this since we rely on React and the browser here. But maybe LoadingButton is the most common problem so we'll monitor this issue. If it becomes too frequent we might want to take a look.
It may make sense to always render the label of the button but hide it so that the text content is stable. That way React won't have to reconcile text content (outside of the cases where you would also write <Button>{a ? 'one label' : 'another label'}</Button>). But we would need to check if Google translate works on hidden text content.
Any update on this one? I face this issue on version: "@mui/lab": "^5.0.0-alpha.106",
Use case:
In some application I'm using the LoadingButton in a "Confirm Dialog" (strait forward implementation: <Dialog> then <DialogActions> then <LoadingButton>)
steps:
- open dialog
- switch google translation in browser adress bar
- press the loading button and set the loading prop to true.
- 💥 Crash.
Any change we can just implement the suggestion of @eps1lon? Would be pleased to implement it if you want it.
I've looked it over, made some changes to the loading button to try and wrap it with a div and test it locally. No more crashing React. Works fine 👍🏻 . However, the spinner's circle svg (child node of the button) somehow gets disconnected of its styles. No idea how to prevent that yet.
Original state:
after switching on the translation:
I've checked this change https://github.com/mui/material-ui/pull/23237/files but attaching class 'notranslate' to the circularprogress makes no difference. styles will still be removed.
Update: Google translates the style tag, when i remove the animation and keyframes from the style tag it seems to not happen anymore. looking further..
Update 2: the styles removal got fixed by start using the StyledEngineProvider + injectFirst prop.
Made a PR to get the translate bug in loading button fixed. Let me know if i need to make any adjustments.
@BartJanvanAssen as far as I tested the issue can be fixed by using span, instead of div, which should be better in my opinion. However, this changes the structure of the component, which may create some breaking changes. This can be fixed in userland, by simply wrapping the text in the button in a span element. It's not great, but it may be better if we recommend this and come back to this change in v6. cc @michaldudak for thoughts. Here is a demo with non-english content inside the button for reproduction: https://codesandbox.io/s/rough-dream-tducty?file=/demo.tsx
:+1: I agree, this may break layouts that assume the content of the button is not wrapped in another container. Let's update the docs and add a warning block that'll advise developers to wrap the text inside the button in a span to work around this problem.
The issue is not mentionned here: https://mui.com/material-ui/api/loading-button/
It's the first result in Google when searching for "mui loading button"
My team stumbled on this issue and it made us waste time.
@DiegoAndai, what do you think about adding the fix to v6? It's a breaking change, as it could mess up layouts that depend on the lack of additional elements under the button.
@michaldudak is this the fix?: https://github.com/mui/material-ui/pull/35198
Or would it be enough to swap div for span on that file? I'm asking because of https://github.com/mui/material-ui/pull/35198/files#r1089914838.
This one is tricky because I fail to imagine breaking changes for the change 😅 Do you remember which ones you were thinking about in this comment?
It doesn't really matter if it's a div or a span - there has to be an element inside. It may break layouts that depend on the button not having this intermediate component.