[lab][LoadingButton] Apply wrapping element to prevent React crash on Google page translation by BartJanvanAssen · Pull Request #35198 · mui/material-ui

BartJanvanAssen · 2022-11-18T14:49:38Z

I have followed (at least) the PR section of the contributing guide.

Hey folks 👋
I ran into this issue where some of our customers on our site were trapped by this react crashing loading button. I wanted to to invest some time to get it out of the package.

Use case summary:

Using the LoadingButton while have Google translation (chrome feature) active on your site.
Problem: Google modifies the button contents and React can't figure out its bindings anymore. Causes crash 💥

How to reproduce:

https://codesandbox.io/s/dry-water-eig2e7?file=/demo.tsx

Steps:

Open sandbox in external browser
Use google chrome's translate feature (see screenshot)
Click the loading button
Notice crash 💥

Why this solution?

I've implemented one of the suggestion in the issue, tested it locally and it seemed to work out perfectly.

😃 This is my first contribution to MUI, so if I need to change something, let me know.

Closes #27853

BartJanvanAssen · 2022-11-23T07:48:07Z

@gzrae @eps1lon do you want to check this out relating #27853 ?

oliviertassinari

I wonder if we even need to add wrappers, maybe changing the conditioning logic could solve this. An option mentioned in #27853 (comment).

@michaldudak why add the "on hold" label? This component is unstable.

packages/mui-lab/src/LoadingButton/LoadingButton.js

oliviertassinari · 2023-01-28T14:59:39Z

packages/mui-lab/src/LoadingButton/LoadingButton.js

 };

+const contentDivStyles = {
+  display: 'contents'


Interesting trick, it seems to have tons of limitations https://caniuse.com/css-display-contents but maybe it's OK in this context.

Should be tested with assistive technology. I think VoiceOver had big problems with this. I think it's safer to not use display: contents in the forseeable future.

I'm still unsure if display: contents can be trusted seeing all of the literature written around how problematic it is. I've removed it for now, but if we feel strong about its benefits we can spend some time testing on different combinations of screen readers and browsers. See this other comment: #35198 (comment)

packages/mui-lab/src/LoadingButton/LoadingButton.js

michaldudak · 2023-01-29T09:26:01Z

why add the "on hold" label? This component is unstable.

See #27853 (comment)

Yes, it's unstable, but if we don't have to introduce breaking changes, especially if the fix is possible in userland, should we?

oliviertassinari · 2023-01-29T20:32:18Z

Yes, it's unstable, but if we don't have to introduce breaking changes, especially if the fix is possible in userland, should we?

@michaldudak This confuses me. It sounds like we either: 1. don't want to fix this bug since we don't think it's important enough for a BC or 2. we should make this component stable since we don't want to introduce BCs. I don't understand the logic for waiting v6.

michaldudak · 2023-01-31T12:16:52Z

My reasoning is: the LoadingButton component, while unstable, has been around for a while, and likely is used in several projects. The workaround in userland is very simple, so working on a fix on our side immediately isn't crucial.
Now, it's not a hill I want to die on, so if you want to have it fixed sooner, be my guest.

oliviertassinari · 2023-01-31T19:57:14Z

@michaldudak OK, I propose that we don't tie the fix to a specific major version then. It sounds like we are more on case 1. (we don't think it's important enough). So effectivement, we are waiting for the pain to grow, more people to complain about it, to feel that working on it is the lowest opportunity cost. Maybe we will fix it in v5.34.0 or maybe in v7.0.0 or maybe never.

pvinicius · 2023-03-01T18:26:09Z

@michaldudak OK, I propose that we don't tie the fix to a specific major version then. It sounds like we are more on case 1. (we don't think it's important enough). So effectivement, we are waiting for the pain to grow, more people to complain about it, to feel that working on it is the lowest opportunity cost. Maybe we will fix it in v5.34.0 or maybe in v7.0.0 or maybe never.

that issue was responsible for a bug in my current sprint :D
but I've seen the official doc and I fixed it, thanks

mui-bot · 2024-06-06T17:03:04Z

Netlify deploy preview

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 7201311

aarongarciah · 2024-06-06T17:06:37Z

Hey @BartJanvanAssen! I'm picking picking this up to land it for v6. I introduced two changes in 9b6e3a1:

Use a <span> instead of a <div> to wrap children.
Remove display: contents.

Regarding number 2, display: contents suffered from deep accessibility issues for years, the most important one causing elements with the property applied to be removed from the accessibility tree. Most of these issues have been solved over the years, but it's still unclear if it's 100% reliable, especially on Safari. I'm wary of using this property if we're not 100% sure about it'll have the expected behavior. If we feel strong about its benefits we can spend some time testing on different combinations of screen readers and browsers. Let me know your thoughts. Some literature on the topic:

DiegoAndai

@aarongarciah may I ask you to add a section explaining this change in the migration guide, as it might be a breaking change for some users 😊

aarongarciah · 2024-06-13T08:49:28Z

@DiegoAndai entry added to the migration guide 883ace8

DiegoAndai · 2024-06-14T14:14:12Z

entry added to the migration guide 883ace8

Thanks, may I ask you to move it to the v5 breaking changes: https://github.com/mui/material-ui/blob/next/docs/data/material/migration/migration-v5/migration-v5.md#breaking-changes

aarongarciah · 2024-06-14T14:34:13Z

@DiegoAndai done

DiegoAndai

Thanks for working on this @aarongarciah! Here's the before and after testing that the issue is fixed

Before

Sandbox

Screen.Recording.2024-06-14.at.11.41.24.mov

After

Sandbox

Screen.Recording.2024-06-14.at.11.42.55.mov

… Google page translation (mui#35198) Co-authored-by: Aarón García Hervás <[email protected]>

zannager added the scope: button Changes related to the button. label Nov 21, 2022

michaldudak linked an issue Nov 24, 2022 that may be closed by this pull request

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

Closed

zannager requested a review from michaldudak December 29, 2022 10:06

michaldudak added this to the v6 milestone Jan 3, 2023

michaldudak added the on hold There is a blocker, we need to wait. label Jan 3, 2023

michaldudak removed their request for review January 3, 2023 19:17

oliviertassinari reviewed Jan 28, 2023

View reviewed changes

packages/mui-lab/src/LoadingButton/LoadingButton.js Outdated Show resolved Hide resolved

oliviertassinari removed this from the v6 milestone Jan 28, 2023

oliviertassinari reviewed Jan 28, 2023

View reviewed changes

DiegoAndai mentioned this pull request Jan 31, 2024

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

Closed

aarongarciah added package: lab Specific to the lab. component: LoadingButton The React component. and removed scope: button Changes related to the button. on hold There is a blocker, we need to wait. labels Jun 5, 2024

aarongarciah mentioned this pull request Jun 6, 2024

[lab][LoadingButton] Prevent React crash with Google Translate #42552

Closed

aarongarciah changed the title ~~[LoadingButton] Apply wrapping div to prevent react crash on Google page translation~~ [lab][LoadingButton] Apply wrapping element to prevent React crash on Google page translation Jun 6, 2024

aarongarciah force-pushed the loading-button-translatable branch from 9d8651e to 9b6e3a1 Compare June 6, 2024 17:00

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Jun 6, 2024

aarongarciah changed the base branch from master to next June 6, 2024 17:00

aarongarciah self-assigned this Jun 6, 2024

aarongarciah requested a review from DiegoAndai June 6, 2024 17:06

aarongarciah added the v6.x label Jun 6, 2024

aarongarciah closed this Jun 6, 2024

aarongarciah reopened this Jun 6, 2024

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Jun 6, 2024

DiegoAndai reviewed Jun 10, 2024

View reviewed changes

BartJanvanAssen and others added 4 commits June 13, 2024 17:27

Apply suggestion to prevent react crash on Google page translation

3d0aff5

make translation wrapping div have no styles interference

e1910d9

Use span instead of div and remove display:contents

5433fd3

Add entry in migration guide

883ace8

aarongarciah force-pushed the loading-button-translatable branch from 20b756b to 883ace8 Compare June 13, 2024 15:27

aarongarciah requested a review from DiegoAndai June 13, 2024 15:27

Move deprecation entry to correct page

7201311

DiegoAndai added the breaking change Introduces changes that are not backward compatible. label Jun 14, 2024

DiegoAndai approved these changes Jun 14, 2024

View reviewed changes

aarongarciah merged commit b51fd5d into mui:next Jun 14, 2024

This was referenced Aug 22, 2024

[button] Fix loading button regression #43400

Closed

[button] Add missing customize points for span #43436

Merged

ZeeshanTamboli mentioned this pull request Nov 2, 2024

[material-ui][Button] Add loading feature to Button #42987

Merged

Uh oh!

Conversation

BartJanvanAssen commented Nov 18, 2022 • edited by oliviertassinari Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Use case summary:

How to reproduce:

Why this solution?

Uh oh!

BartJanvanAssen commented Nov 23, 2022

Uh oh!

oliviertassinari left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

oliviertassinari Jan 28, 2023

Choose a reason for hiding this comment

Uh oh!

eps1lon Jan 29, 2023

Choose a reason for hiding this comment

Uh oh!

aarongarciah Jun 6, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

michaldudak commented Jan 29, 2023

Uh oh!

oliviertassinari commented Jan 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaldudak commented Jan 31, 2023

Uh oh!

oliviertassinari commented Jan 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pvinicius commented Mar 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mui-bot commented Jun 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Netlify deploy preview

Bundle size report

Uh oh!

aarongarciah commented Jun 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DiegoAndai left a comment

Choose a reason for hiding this comment

Uh oh!

aarongarciah commented Jun 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DiegoAndai commented Jun 14, 2024

Uh oh!

aarongarciah commented Jun 14, 2024

Uh oh!

DiegoAndai left a comment

Choose a reason for hiding this comment

Before

After

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

BartJanvanAssen commented Nov 18, 2022 •

edited by oliviertassinari

Loading

oliviertassinari commented Jan 29, 2023 •

edited

Loading

oliviertassinari commented Jan 31, 2023 •

edited

Loading

pvinicius commented Mar 1, 2023 •

edited

Loading

mui-bot commented Jun 6, 2024 •

edited

Loading

aarongarciah commented Jun 6, 2024 •

edited

Loading

aarongarciah commented Jun 13, 2024 •

edited

Loading