Closed Bug 1297784 Opened 5 years ago Closed 5 years ago

Turn performance controls into a React component

Categories

(DevTools :: Performance Tools (Profiler/Timeline), defect, P2)

50 Branch
defect

Tracking

(firefox51 fixed)

RESOLVED FIXED
Firefox 51
Tracking Status
firefox51 --- fixed

People

(Reporter: gregtatum, Assigned: gregtatum)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(1 file)

Begin the process of de-XULing UI and using React.
Priority: -- → P2
Assignee: nobody → gtatum
Comment on attachment 8784494 [details]
Bug 1297784 - Turn performance controls into a React component;

https://reviewboard.mozilla.org/r/73934/#review71810

::: devtools/client/performance/components/recording-controls.js:19
(Diff revision 1)
> +   * Manually handle the "checked" and "locked" attributes, as the DOM element won't
> +   * change by just by changing the checked attribute through React.
> +   */
> +  componentDidUpdate() {
> +    if (this.props.isRecording) {
> +      this._recordButton.setAttribute("checked", true);

I think this is against the spirit of React; I think this should be stored in state, and rerendered if checked
Comment on attachment 8784494 [details]
Bug 1297784 - Turn performance controls into a React component;

https://reviewboard.mozilla.org/r/73934/#review71818

r+ if the button react component only touches the DOM in the render function via setState
Attachment #8784494 - Flags: review?(jsantell) → review+
Comment on attachment 8784494 [details]
Bug 1297784 - Turn performance controls into a React component;

https://reviewboard.mozilla.org/r/73934/#review72580

::: devtools/client/performance/components/recording-controls.js:19
(Diff revision 1)
> +   * Manually handle the "checked" and "locked" attributes, as the DOM element won't
> +   * change by just by changing the checked attribute through React.
> +   */
> +  componentDidUpdate() {
> +    if (this.props.isRecording) {
> +      this._recordButton.setAttribute("checked", true);

If that simplifies the code, feel free to use a class instead of attributes.

::: devtools/client/themes/performance.css:96
(Diff revision 1)
>  }
>  
>  /* Recording buttons */
>  
> -#main-record-button {
> -  list-style-image: url(images/profiler-stopwatch.svg);
> +#clear-button::before {
> +  background-image: var(--clear-icon-url);

nit: remove this, and add .devtools-clear-icon on the button

::: devtools/client/themes/performance.css:153
(Diff revision 1)
> +#recordings-pane .devtools-toolbar {
> +  line-height: 0;
> +}
> +
> +.devtools-button[checked]::before  {
> +  /*filter: var(--checked-icon-filter);*/

nit: remove this
Comment on attachment 8784494 [details]
Bug 1297784 - Turn performance controls into a React component;

https://reviewboard.mozilla.org/r/73934/#review71818

A few observations here. I'm not sure if you're taking issue with the fact that I'm saving the reference of the record button to `this._recordButton` rather than using setState, or if you're taking issue with me mutating the DOM directly, so I'll address my approach on both.

First the [React docs for refs](https://facebook.github.io/react/docs/more-about-refs.html#the-ref-callback-attribute) show saving the refs to the raw DOM element this way, so I think it's a canonical approach to storing that information.

Next, I think it is valid to store state inside of the component outside of the state object. From my own observations I think it is mainly for things that live outside of the component lifecycle and will not affect the output of the render function. [For instance in debugger.html](https://github.com/devtools-html/debugger.html/blob/master/public/js/components/App.js#L54-L68) you can see they are instantiating KeyShortcut directly on the instance outside of the state object, but then a handler on `this.keyShortcut` can trigger the state to update, which in turn changes the output of the render function.

Finally I was doing a bit more research just now on why the button component wouldn't let me change the value directly. It looks like [React has the concept of managed vs. unmanaged form elements](https://facebook.github.io/react/docs/forms.html#controlled-components). So for an input, if you set the value directly it becomes a managed component and you can use attributes like checked and it will affect the DOM element. While if it's an unmanaged component it will not change the checked value. Buttons don't really have any idea about how checked affects them, but the style in devtools uses the existence of the checked attribute to change the styling. I tried following this same managed/unmanaged logic with the button that applies to inputs, but I guess this concept is not being applied to the button element by React, so I had to create that behavior directly as I did here.

I could try to move this behavior to state, but it doesn't seem like the right path to me.

Thoughts?
Comment on attachment 8784494 [details]
Bug 1297784 - Turn performance controls into a React component;

https://reviewboard.mozilla.org/r/73934/#review74518

::: devtools/client/performance/components/recording-controls.js:19
(Diff revision 1)
> +   * Manually handle the "checked" and "locked" attributes, as the DOM element won't
> +   * change by just by changing the checked attribute through React.
> +   */
> +  componentDidUpdate() {
> +    if (this.props.isRecording) {
> +      this._recordButton.setAttribute("checked", true);

In a way it's against the spirit of React, but React also gives these kinds of escape hatches to work around issues. This will work fine with React and it is how people get around these issues. According to Greg React simply won't apply the `checked` attribute to a button for some reason. If that's the case, this could is OK and shouldn't cause any problems with how React works.

Ideally we shouldn't have to do this though. Tim mentioned that there are classes you could use instead? If so, try switching to that and see if it works, but worst case this code is OK to get around the above issues.
Comment on attachment 8784494 [details]
Bug 1297784 - Turn performance controls into a React component;

https://reviewboard.mozilla.org/r/73934/#review72580

> If that simplifies the code, feel free to use a class instead of attributes.

I'm going to do a follow-up on the CSS for the tool, I'll tackle that then. The CSS for this is not in the performance.css file, but in the overall devtools stylesheet.

> nit: remove this, and add .devtools-clear-icon on the button

This one is another one that isn't working with the current styling if I do it that way.

> nit: remove this

Thanks for catching this!
I pushed up a slight revision to the patch that removes a commented out CSS rule. This doesn't invalid my previous try run, so I'm going to go ahead and flag this as checkin-needed.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0a038b4dc136
Turn performance controls into a React component; r=jsantell
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0a038b4dc136
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Depends on: 1300473
(In reply to Phil Ringnalda (:philor) from comment #13)
> https://hg.mozilla.org/mozilla-central/rev/0a038b4dc136
> 
+recordings.start.tooltip=Toggle the recording state of a performance recording.

This was an excellent opportunity to remove unrequired and confusing trailing periods in tooltips, as reported numerous times for devtools files. Please keep this in mind and don’t let it be a reoccurring issue.

See bug 951132, bug 1175756 comment 5.
Thanks Ton for letting me know about this. As a newer member of the team I wasn't aware of this issue.
Depends on: 1326950
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.