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.

Sign up for GitHub

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

Jump to bottom

[material-ui][LoadingButton] Crashes React when Chrome has Translated the Page #27853

Closed
scottfr opened this issue Aug 19, 2021 · 13 comments · Fixed by #35198
Closed

[material-ui][LoadingButton] Crashes React when Chrome has Translated the Page #27853

scottfr opened this issue Aug 19, 2021 · 13 comments · Fixed by #35198
Assignees
@aarongarciah
Labels
bug 🐛 Something doesn't work component: button This is the name of the generic UI component, not the React module! external dependency Blocked by external dependency, we can’t do anything about it
Milestone
Material UI: v6

Comments

@scottfr
Copy link

scottfr commented Aug 19, 2021 •
edited by oliviertassinari
Loading

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 🕹

  1. Set up Chrome languages to use a non english language and enable translation from english (https://support.google.com/chrome/answer/173424)
  2. Go to: https://next.material-ui.com/components/buttons/
  3. 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)
  4. Toggle the loading toggle, the demo will crash
@scottfr scottfr added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Aug 19, 2021
@qiweiii
Copy link
Contributor

qiweiii commented Aug 23, 2021

Found an explanation: facebook/react#11538 (comment)

There are a few solutions to fix this:

  • Add a container div outside children in LoadingButton
  • Add span on text nodes in LoadingButton examples in docs
  • Add <html translate="no"> to html tag

@eps1lon
Copy link
Member

eps1lon commented Aug 23, 2021

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. #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.

@eps1lon eps1lon added bug 🐛 Something doesn't work component: button This is the name of the generic UI component, not the React module! external dependency Blocked by external dependency, we can’t do anything about it and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Aug 23, 2021
This was referenced Sep 30, 2021
Closed
Closed
@BartJanvanAssen
Copy link
Contributor

BartJanvanAssen commented Nov 16, 2022 •
edited
Loading

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:

  1. open dialog
  2. switch google translation in browser adress bar
  3. press the loading button and set the loading prop to true.
  4. 💥 Crash.

Any change we can just implement the suggestion of @eps1lon? Would be pleased to implement it if you want it.

@BartJanvanAssen
Copy link
Contributor

BartJanvanAssen commented Nov 18, 2022 •
edited
Loading

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:
image
after switching on the translation:
image

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.

@BartJanvanAssen BartJanvanAssen mentioned this issue Nov 18, 2022
Merged
1 task
@BartJanvanAssen
Copy link
Contributor

Made a PR to get the translate bug in loading button fixed. Let me know if i need to make any adjustments.

@mnajdova
Copy link
Member

@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

@michaldudak
Copy link
Member

👍 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.

@michaldudak michaldudak linked a pull request Nov 24, 2022 that will close this issue
[lab][LoadingButton] Apply wrapping element to prevent React crash on Google page translation #35198
Merged
1 task
michaldudak added a commit to michaldudak/material-ui that referenced this issue Jan 3, 2023
@michaldudak
[docs] Document the workaround for crashing a translated page (mui#27853
61e4a43 
)
@michaldudak michaldudak mentioned this issue Jan 3, 2023
Merged
@oalexdoda
Copy link

I confirm this is still happening.

@adrien-febvay
Copy link

adrien-febvay commented Jan 16, 2024 •
edited
Loading

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.

@michaldudak
Copy link
Member

@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.

@DiegoAndai
Copy link
Member

@michaldudak is this the fix?: #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?

@michaldudak
Copy link
Member

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.

@DiegoAndai
Copy link
Member

Adding it to the v6 milestone

@DiegoAndai DiegoAndai added this to the Material UI: v6 milestone Feb 9, 2024
@DiegoAndai DiegoAndai changed the title v5 LoadingButton Crashes React when Chrome has Translated the Page [material-ui][LoadingButton] Crashes React when Chrome has Translated the Page Feb 19, 2024
@DiegoAndai DiegoAndai moved this to Backlog in Material UI Feb 19, 2024
@aarongarciah aarongarciah mentioned this issue Jun 6, 2024
Closed
@github-project-automation github-project-automation bot moved this from Backlog to Done in Material UI Jun 14, 2024
@oliviertassinari oliviertassinari mentioned this issue Aug 22, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@aarongarciah aarongarciah

Labels
bug 🐛 Something doesn't work component: button This is the name of the generic UI component, not the React module! external dependency Blocked by external dependency, we can’t do anything about it
Projects
Material UI
Status: Done
Milestone
Material UI: v6