mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.8k stars 32.25k forks source link

Chromecast button breaks Button rendering #11363

Closed acroyear closed 6 years ago

acroyear commented 6 years ago

In Chrome, I'm trying to add a cast button like the following.

        <Button is="google-cast-button">Cast</Button>

I already know that chrome isn't working in one regard - it should be converting the content and applying the event handlers but it isn't, nevermind all that, I'm working that separately.

My main issue is that Button styles aren't getting applied.

    <Button variant="raised" color="secondary" onClick={this.handleClick}>
      this button appears styled
    </Button>
    <br/>
    <Button variant="raised" color="secondary" is="google-cast-button">This button is unstyled</Button>

if you view the rendered contents, the Normal button has class="Mui-etc", but the Cast button has classname="Mui-etc" and so it doesn't render right as far as the theme or the ripple effect (the ripple hits the whole page).

Replication on https://codesandbox.io/s/3q794k1l3q

Context

Context is creating a media (music) app where I would like to include on-screen chromecast support for mobile.

Your Environment

Tech Version
Material-UI 1.0.0b47
React 16.3.2
browser Chrome
etc
acroyear commented 6 years ago

Replication added. Easier than I thought. :)

acroyear commented 6 years ago
<button tabindex="0" class="MuiButtonBase-root-615 MuiButton-root-600 MuiButton-raised-605 MuiButton-raisedSecondary-607" type="button"><span class="MuiButton-label-601">this button appears styled</span><span class="MuiTouchRipple-root-618"></span></button>
<br>
<button tabindex="0" **classname**="MuiButtonBase-root-615 MuiButton-root-600 MuiButton-raised-605 MuiButton-raisedSecondary-607" type="button" **is="google-cast-button"**><span class="MuiButton-label-601">This button is unstyled</span><span class="MuiTouchRipple-root-618"></span></button></div>
acroyear commented 6 years ago

@oliviertassinari is the incomplete label about my bug report and you still would like something from me, or is it your way of saying the Button code itself is 'incomplete'?

oliviertassinari commented 6 years ago

An even simpler reproduction https://codesandbox.io/s/pm3rv8p2yq

<Button is="">This button is unstyled</Button>
acroyear commented 6 years ago

ok. i was rendering both in order to show the expected-actual side by side. :)

oliviertassinari commented 6 years ago

I was able to reproduce the same problem with a raw React version:

import React from "react";
import Button from "@material-ui/core/Button";

class Index extends React.Component {
  render() {
    return (
      <div>
        <Button>1</Button>
        <Button is="foo">2</Button>
        <button style={{ backgroundColor: "yellow" }}>3</button>
        <button style={{ backgroundColor: "yellow" }} is="foo">
          4
        </button>
        <button className="button">5</button>
        <button className="button" is="foo">
          6
        </button>
      </div>
    );
  }
}

export default Index;

capture d ecran 2018-05-14 a 23 44 34 https://codesandbox.io/s/yjjq44qxjv

The Cast button component in your app is handled entirely by the framework.

https://developers.google.com/cast/docs/chrome_sender_integrate

So I guess you can not use Material-UI for that. Anyway. It's such a narrow use case that I don't think its worth putting more energy into it. If you know a simple workaround let us know. We might add it into the core of the library.

acroyear commented 6 years ago

https://github.com/facebook/react/issues/12810

If "className" becomes "classname" instead of "class" just because there's an "is" attribute, then that's a bug, but clearly not material-ui's to fix. Filed with FB above.

It isn't a 'narrow' use case in that it is meant to be a primary feature of using Chromecast in mobile progressive web apps. It may be that react's breaking of className -> classname also means they are breaking something else, too, which may be why the Chromecast feature stopped working. It was working in old material-ui 0.20, but I was running that with react 15.6.2.

For now I'm going to try to code up some serious hackery in a special Button wrapper that will try to add the 'is' attribute to the button after creation. if that doesn't work, i'll try creating a new button using 'is' and the classnames material-ui already applies and swap it out in the dom hierarchy at the right spot.

oliviertassinari commented 6 years ago

It isn't a 'narrow' use case in that it is meant to be a primary feature of using Chromecast in mobile progressive web apps.

@acroyear The data are suggesting it's a very narrow use case. Unless I'm wrong, you are the first one to raise it on React land, after 4 years and 12k issues: https://github.com/facebook/react/issues/12810.

add the 'is' attribute to the button after creation.

This can definitely work

i'll try creating a new button using 'is' and the classnames material-ui already applies

This works:

        <Button
          component={({ className, ...props }) => (
            <button {...props} class={className} />
          )}
          is="foo"
        >
          2
        </Button>
acroyear commented 6 years ago

The problem is it worked before. I have it working in material 0.20 and react 15. :)

thanks for the hint; if I wasn't so tired I'd have started work on it last night. I guess my main issue is that I kept thinking you couldn't provide "class=" in JSX, that it had to be className=, since that's what I'm using in material-ui and in react-bootstrap (3) that I just presumed it was a requirement.

acroyear commented 6 years ago

A problem with component= is that if using a simple component instead of a full one with a constructor, the internal component gets reconstructed with every prop change since you can't control shouldComponentUpdate. This can get messy in a music player where you get prop updates all the time (the current time of the media player). May need to layer the player a bit so the button row doesn't update when only the currentTime changes.

oliviertassinari commented 6 years ago

The problem is it worked before. I have it working in material 0.20 and react 15. :)

@acroyear Oh, this is such a bad news. material-ui 0.20 was using inline style and react 15 wasn't changing the className behavior.

the internal component gets reconstructed with every prop change since you can't control

Yeah, I should have provided a better example. We should avoid the creation of component in the render method:

const HackButton = ({ className, ...props }) => (
  <button {...props} class={className} />
)

function Foo() {
  return (
    <Button
      component={HackButton}
      is="foo"
    >
      2
    </Button>
  )
}

export default Foo
acroyear commented 6 years ago

That might solve the general 'is' case, but it looks like the web component specifics need more. It seems that if the "is=" is added by a setAttribute call, or if the element is created basically by innerHTML (which react16 uses more often than 15 did from what I can see), then Chrome doesn't execute the web component's internal code properly.

This means that the only way to get the button to create properly is to use a ref.

const e = document.createElement('button', 'google-cast-button');
this.myRef.current.appendChild(e);

and so the material-ui way would be to have a

acroyear commented 6 years ago
class WebComponentButtonBase extends React.Component {
  constructor(props) {
    super(props);
  }

  componentDidMount() {
    if (this.props.is) {
      // if the Button is rendered within withStyles, then the ref is WithStyles rather than the button, so use this instead
      const button = ReactDOM.findDOMNode(this);
      const buttonRipple = button.children[1];
      const parent = button.parentElement;

      const castButton = document.createElement('button', 'google-cast-button');
      castButton.className = button.className;
      castButton.appendChild(buttonRipple);
      parent.replaceChild(castButton, button);
    }
  }

  render() {
    const props = this.props;
    const coreProps = {
      color: props.color || 'primary',
      onClick: props.action,
      "aria-label": props.label
    };
    if (props.disabled) coreProps.disabled = true;
    if (props.fab) {
      coreProps.variant = "fab";
      if (props.standalone) coreProps.classes = { root: props.classes.standalone }
    }
    else coreProps.classes = { root: props.classes.root };
    return <Button {...coreProps}>{props.children}</Button>;
  }
}

const WebComponentButton = withStyles(styles)(WebComponentButtonBase);

export const ChromecastButton = props => {
  const icon = <Cast/>;
  const coreProps = {
    label: "cast",
    // disabled: props.isLoading,
    fab: props.fab
  };

  return <WebComponentButton {...coreProps} is="google-cast-button">{icon}</WebComponentButton>;
};

Well, kinda works but doesn't. I get the ripple span, but it doesn't trigger (this I kinda expected). The real issue is that the IDs generated for the withStyles classnames change AFTER it has been mounted.

so at the point of componentDidMount, I have "WebComponentButtonBase-root-1342", but the style in the header for data-meta="WebComponentButtonBase" has incremented up to WebComponentButtonBase-root-1784". It keeps updating but my new cast button is now hopelessly behind the times with a style that no longer exists. Same with the MuiButtonBase classes. They've incremented as the wrapping theme has changed but again my own button is out of date.

By swapping out the button, I've swapped out Button's idea of what its component is. It wants to apply any further changes to its original ref/component, but that's not what I'm showing on screen anymore.

back to the drawing board. :)

acroyear commented 6 years ago

ok, I found an implementation, but i have to hard-copy the Material-UI button styles (and I lose ripple) but at least it is functional. http://jwsdev.net/jottings/web-components-is-a-problem-in-react-js/ describes the basic approach.

What I did just find is that it all SHOULD have worked (if I did the class={classNames} version you described above to get past the className issue from react). What appears to be the problem is that Chrome is ignoring the specification in createElement. The second parameter, according to the spec, should be {is: 'parent-component-type'}, but Chrome is not acknowledging that, at least not for google-cast-button, and not working if react is following the specification like they should. I've filed a bug with them, http://jwsdev.net/l/castbug

acroyear commented 6 years ago

To follow-up, Chrome has acknowledged that it was always a bug on their part, using an older syntax and not the newer one. The launcher has been updated to a modern web component "google-cast-launcher" which works properly in react can be styled using class=.

I still will play with it to see if I can embed it in a button (or derive from ButtonBase and get the ripple effect to work, but it is possible to find the button styles and apply them, but for now, i'm no longer hacking around the bug and all is well.

acroyear commented 6 years ago

ok, I can't do it easily.

The reason is ButtonBase - it takes ComponentProp (in this case, 'google-cast-launcher') and instantiates it...with className=.

This is a problem for web components. When working with web components, you need to use class=, not className=. So to do this, I would need to first change ButtonBase to have it look at the component property value. If it has a '-', then use class= otherwise className=. That seems a lot of work (and a little risky) for such a minor feature. I'll just stick with my current for now.

acroyear commented 6 years ago

ok, if i put it inside a Button, everything is right except the size, so i need to constrain the width and height of the cast component to match how big the icons are. So when my svg set is for 24px, then setting width and height to 24 and all is well. i'll write that up in my blog a little later on today.

acroyear commented 5 years ago

For those who may stumble into reading this, I just want to know that Chromium has fixed their Chromecast code to support the proper 'is' syntax, and now react (and MUI) code can correctly render web components and then use componentDidMount for any initialization code.

oliviertassinari commented 5 years ago

:)