Issue #11827 Fix ShadowDOM input bug with setDefaultValue

[chrisdieckhaus]

Fixes Issue #11827.

#7253 successfully fixed the number input backspace bug with removing decimals by not re-setting the default value if node === activeElement. However, shadowDOM elements are wrapped in a shadowRoot element, so comparing them to node always triggered an update to node.defaultValue (which triggered input's validation that removed the decimal point). I've updated setDefaultState to check for shadow DOM element, and if so, iterate to the root activeElement to compare against node.

I haven't added tests yet because I need some input on that. I'm assuming the tests should go in ReactDOMInput-test.js. However, I'm not sure how to properly mock a shadow DOM element in these tests, and would appreciate some input on that.

• complete PR checklist
• add tests

[jquense]

Generally this sort of thing is "tested" via a manual fixture. Check out fixtures/dom from the repo root for some existing ones and you can add one there. There is no automation, so don't be confused if you can't run them via a cli or something :P

[nhunzaker]

Looks good. Just need to verify manually

[nhunzaker]

Huh. Nice!

[aweary]

@chrisdieckhaus looks like you need to run yarn prettier so CI passes :)

[chrisdieckhaus]

Added manual test case to number-inputs/index.js. Let me know if that's not the right place to put that.

[nhunzaker]

The formatting is perfect here, but it doesn't look like this attaches to a Shadow DOM. What is the best way to do that?

[chrisdieckhaus]

I’m not entirely sure how to deal with a Shadow DOM in the test fixtures. I’ll look into it.

[nhunzaker]

Marking this as "request changes" for the issues list.

[chrisdieckhaus]

Hi @nhunzaker is this good to merge?

[nhunzaker]

It looks like this injects<input type="number" value={this.state.value} onChange={this.onChange} />, which is the form React requires, however this won't wire on onChange, just set an HTML string uncontrolled by React. This will create a false positive for the bug because React doesn't interact with this injected markup.

We need to render a React component inside of a shadow dom sub-tree. It looks like #11827 includes a reproduction that does just this:
https://jsfiddle.net/69z2wepo/94566/

Drawing from that example, you could do:

import NumberTestCase from './NumberTestCase';

export default class NumberTestCaseShadowDOM extends React.Component {
  componentWillMount() {
    // I tucked the definition in here because I wasn't sure what extending HTMLElement
    // would do in IE9. Maybe this isn't necessary.
    class ShadowNumberInput extends HTMLElement {
      connectedCallback(){
        ReactDOM.render(<NumberTestCase />, this.attachShadow({ mode: 'open' }));
      }
    }

    customElements.define('with-shadow-dom', ShadowNumberInput);
  }

  render() {
    return (<shadow-number-input {...this.props} />);
  }
}

Here's a working example: https://jsfiddle.net/2n77ootd/2/

[chrisdieckhaus]

So whenever I go with that sort of an implementation, I get the following error when it tries to render the shadow element:

TypeError: Failed to construct 'HTMLElement': Please use the 'new' operator, this DOM object constructor cannot be called as a function.

It seems like custom elements are not compatible with ES5 style classes. I'm able to get it working with their suggested solution, native-shim.js. Is this something we'd be able to include in react? Or should we be looking for a different solution to this?

[chrisdieckhaus]

@nhunzaker thoughts on this?

[nhunzaker]

@chrisdieckhaus Sorry, not quite yet. The test case needs to render React inside of a shadow dom root. Like use ReactDOM.render inside of a Shadow DOM to verify that it coordinates properly with the parent DOM (Sunny DOM 😛?).

I left some thoughts. Essentially I think we just need to render the same number input test case inside of a Shadow DOM, then mount an associated custom element with React (inside of the render method of the fixture component).

[chrisdieckhaus]

Ah yeah I see what you mean. I'll play around with it and see what I can come up with.

[chrisdieckhaus]

@nhunzaker, I added the native-shim.js file I mentioned earlier. I'm not sure if it's something that's acceptable to add, though. Take a look at it when you get a chance.

[nhunzaker]

Hey! Awesome. I'm out of pocket for a bit, but I'll look at this first thing in Monday (traveling). Thank you for sticking with me!

[chrisdieckhaus]

Ah this is interesting. Custom Elements are not supposed (yet) in Firefox, which prevents the Number Input test case page from rendering.

[nhunzaker]

@chrisdieckhaus Hmm. We probably need to do a compatibility check. I think it would be fine if the fixture said something like "This browser does not support custom elements, this fixture is invalid".

I apologize for getting back to this as late as I have. If you'd like me to pick this up, I'd be happy to carry this through.

[gaearon]

Would #11896 also fix this?

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

[stale]

Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you!