Closed Bug 1245029 Opened 8 years ago Closed 8 years ago

Refactor about:debugging to full-React

Categories

(DevTools :: about:debugging, defect)

Product:
DevTools
Component:
about:debugging
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox47 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 47
Tracking Flags:
Tracking Status
firefox47 --- fixed

People

(Reporter: janx, Assigned: jdescottes, Mentored)

References

Details

Attachments

(2 files, 11 obsolete files)

bug1245029.part1.v4.patch (rebased)
23.86 KB, patch
jdescottes
: review+
Details | Diff | Splinter Review
bug1245029.part2.v7.patch (rebased)
24.77 KB, patch
jdescottes
: review+
Details | Diff | Splinter Review 
In the UI hierarchy, about:debugging currently uses React only for the Workers/Addons components (groups of TargetLists, e.g. "Extensions", "Service Workers", etc) and their sub-components.

It would be nice to rewrite the overlying UI components in React as well:

- The panels ("Addons" and "Workers"), including panel title, checkboxes and special buttons.
- The categories menu on the left.
- The overall about:debugging app.

The could be done in the following steps:
1. Include panel title, checkboxes and special buttons in the specific "components/{addons,workers}.js" components.
2. Move these panel components into an overlying AboutDebugging component.
3. Rewrite the left menu as a React component, add it to the overall AboutDebugging component.

The AboutDebugging component should initialize with a DebuggerClient (by default the local client gotten from `DebuggerServer.connectPipe()`, but later possibly a remote runtime), and then decide which target categories should be enabled for that runtime (by default "Addons" and "Workers", but this list will probably change according to various runtime types).
Julian, is this something you might be interested in taking on? I can help you along the way.
Flags: needinfo?(jdescottes)
Yes! Just starting with React, but that would be a good exercise for me.
 
And thanks for the breakdown, it helps.
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Flags: needinfo?(jdescottes)
That's great! Please let me know if you need more pointers on how to proceed.

Earlier today, I suggested finishing up my PrefCheckbox React component for this bug, but it's actually useless for the single checkbox Alex is (rightly) suggesting in bug 1227135 comment 12.

Instead, maybe it's best to just inline the <label/><input type=checkbox/> HTML and associated prefListener into components/addons.js, along with the "Load Temporary Add-on" button and "Add-ons" title? I don't know, you decide :)
Mentor: janx
Attached patch bug1245029.wip.patch (obsolete) — — Splinter Review 
Bug 1245029 - part 1 : move headers and controls to workers/addons react components

As the title says, this commit moves headers (and workers controls) to the appropriate React components. To do so, I created :
- a category-header component, rendering a provided name in a "div.header h1.header-name" 
- an addons-controls component, responsible for the "enable workers debugging" checkbox and "load addon" button

FWIW, this passes the current test suite successfully.

:janx, let me know what you think
Attachment #8715057 - Flags: feedback?(janx)
Comment on attachment 8715057 [details] [diff] [review]
bug1245029.wip.patch

Review of attachment 8715057 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! Thank you for addressing this so quickly.

And congratulations on refactoring without breaking any tests! :) (Hopefully it doesn't mean our test coverage is terrible.)

Please also note that this patch conflicts with bug 1210778. You could help Yura's work somewhat by adding a few accessibility attributes here and there (see below for more specific nits). However, no need to fix the accessibility of the checkbox or tabbed menu yet.

::: devtools/client/aboutdebugging/aboutdebugging.xhtml
@@ +30,5 @@
>        </div>
>      </div>
>      <div class="main-content">
> +      <div id="tab-addons" class="tab active"></div>
> +      <div id="tab-workers" class="tab"></div>

Nit: For better accessibility, please add the following attributes: role="tabpanel" and aria-labelledby="<id-of-corresponing-h1.header_name>".

::: devtools/client/aboutdebugging/components/addons-controls.js
@@ +10,5 @@
> +loader.lazyRequireGetter(this, "Cc", "chrome", true);
> +loader.lazyRequireGetter(this, "React", "devtools/client/shared/vendor/react");
> +loader.lazyRequireGetter(this, "Services");
> +loader.lazyImporter(this, "AddonManager",
> +  "resource://gre/modules/AddonManager.jsm");

Nit: Please add an empty line to separate lazyImporter()s and lazyRequireGetter()s.

@@ +16,5 @@
> +const Strings = Services.strings.createBundle(
> +  "chrome://devtools/locale/aboutdebugging.properties");
> +
> +exports.AddonsControlsComponent = React.createClass({
> +  displayName: "AddonsControlsComponent",

Nit: It's my fault for starting this convention, but I don't think that suffixing React classes with "Component" is a good idea anymore. You can leave this unchanged here, but at some point I'd like us to rename all the "ThingComponent"s to just "Thing"s.

@@ +18,5 @@
> +
> +exports.AddonsControlsComponent = React.createClass({
> +  displayName: "AddonsControlsComponent",
> +
> +  loadAddonFromFile(event) {

Nit: In general for React classes, I like to give methods a consistent order (React boilerplate first, then methods common to other components, then methods specific to this component):
- getInitialState()
- componentDidMount()
- componentWillUnmount()
- render()
- update()
- doSpecificThing()

If you also think this makes classes easier to read, please move the `loadAddonFromFile()` method below `render()`.

@@ +21,5 @@
> +
> +  loadAddonFromFile(event) {
> +    // TODO: I know this is horrible! need to find another way to get the
> +    // current window here
> +    let win = event.target.ownerDocument.defaultView;

The fact that we don't have easy access to the window from within React components is by design. I'm not sure how other tools handled this. Do you know if this workaround works in all situations?

::: devtools/client/aboutdebugging/components/addons.js
@@ +51,5 @@
> +
> +  removePreferencesListener() {
> +    Services.prefs.removeObserver("devtools.chrome.enabled",
> +      this.updateDebugStatus);
> +  },

Nit: It doesn't seem useful to have separate `{add,remove}PreferencesListener()`. Please inline them into `component{DidMount,WillUnmount}()` respectively.

@@ +55,5 @@
> +  },
> +
> +  onDebugEnabledToggle() {
> +    Services.prefs.setBoolPref("devtools.chrome.enabled",
> +      !this.state.debugEnabled);

Nit: It doesn't seem useful to have the `onDebugEnabledToggle()` listener here. Please move it into the `AddonsControls` component.

Nit: Please also make it set the pref according to whether the checkbox is actually checked, rather than whatever internal state we have. Maybe I'm being paranoid, but this would prevent situations where the checkbox shows the opposite of the pref value.

::: devtools/client/aboutdebugging/components/category-header.js
@@ +9,5 @@
> +loader.lazyRequireGetter(this, "React",
> +  "devtools/client/shared/vendor/react");
> +
> +exports.CategoryHeaderComponent = React.createClass({
> +  displayName: "CategoryHeaderComponent",

Nit: Maybe this is more a "TabHeader" than a "CategoryHeader"?

@@ +16,5 @@
> +    let { name } = this.props;
> +
> +    return React.createElement(
> +      "div", { className: "header" }, React.createElement(
> +        "h1", { className: "header-name" }, name));

Nit: For more accessibility, please add a unique id here for #tab-addons and #tab-workers to use as aria-labelledby (see above).

::: devtools/client/aboutdebugging/components/workers.js
@@ +31,5 @@
>          service: [],
>          shared: [],
>          other: []
> +      },
> +      debugEnabled: true

Nit: Why did you change the optional `debugDisabled` back to a mandatory `debugEnabled` that needs to be `true` here?
Attachment #8715057 - Flags: feedback?(janx) → feedback+
Thanks a lot for the feedback! Some replies below, I'll upload the modified patch in a bit. 

(In reply to Jan Keromnes [:janx] from comment #5)
> Please also note that this patch conflicts with bug 1210778. You could help
> Yura's work somewhat by adding a few accessibility attributes here and there
> (see below for more specific nits). However, no need to fix the
> accessibility of the checkbox or tabbed menu yet.
> 

Any chance bug 1210778 will land soon ? I'm afraid that the patches will have to be reworked if we land the refactor before :(

> 
> at some point I'd like us to rename all the
> "ThingComponent"s to just "Thing"s.
> 

Great, let's do that in a separate part in this bug!

> The fact that we don't have easy access to the window from within React
> components is by design. I'm not sure how other tools handled this. Do you
> know if this workaround works in all situations?
> 

It should work. An alternative would be to pass the window here as a prop.
Looking at the memory module, they actually have access to window in React components, but they use a different loader.
See BrowserLoaderModule at https://dxr.mozilla.org/mozilla-central/source/devtools/client/memory/initializer.js#8

For now I would be tempted to keep the workaround, move forward with the refactoring and address this in another chunk of the same bug.

> 
> Nit: It doesn't seem useful to have the `onDebugEnabledToggle()` listener
> here. Please move it into the `AddonsControls` component.
> 

I wanted to keep everything related to addons preferences in the same class because it will get more complex with Bug 1227137. I moved it to AddonsControls for now, we can see how 1227137 turns out later.

> Nit: Please also make it set the pref according to whether the checkbox is
> actually checked, rather than whatever internal state we have. Maybe I'm
> being paranoid, but this would prevent situations where the checkbox shows
> the opposite of the pref value.

I was unsure about this one. In the memory tool, checkboxes rely on internal state instead of reading the DOM value, so I did the same here. I'm fine with either :)

> 
> Nit: Why did you change the optional `debugDisabled` back to a mandatory
> `debugEnabled` that needs to be `true` here?

I feel it's harder to reason with negative booleans, but maybe it's just me. That's why I reverted to debugEnabled here. I can change it back if you want.
(flagging both Jan & Alex for review: since this is a refactor I guess it's better to make sure everyone is ok with the changes?)

Bug 1245029 - aboutdebugging full react refactor part1;r=janx,ochameau

React refactor part1 : refactor addons and workers panels as react components
Attachment #8715057 - Attachment is obsolete: true
Attachment #8715766 - Flags: review?(poirot.alex)
Attachment #8715766 - Flags: review?(janx)
Comment on attachment 8715766 [details] [diff] [review]
bug1245029.v1.patch (applied feedback comments)

Review of attachment 8715766 [details] [diff] [review]:
-----------------------------------------------------------------

Whaa already refactored? Great, ship it!
Are you planning to continue up to the root and refactor AboutDebugging itself?


> > at some point I'd like us to rename all the
> > "ThingComponent"s to just "Thing"s.
> > 
> 
> Great, let's do that in a separate part in this bug!
> 

Bug 1207997 is a good candidate to do this!
Attachment #8715766 - Flags: review?(poirot.alex) → review+
(In reply to Alexandre Poirot [:ochameau] from comment #8)
> Comment on attachment 8715766 [details] [diff] [review]
> bug1245029.v1.patch (applied feedback comments)
> 
> Review of attachment 8715766 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Whaa already refactored? Great, ship it!

Thanks!  

> Are you planning to continue up to the root and refactor AboutDebugging
> itself?
> 

Yes! Working on it right now :)
Comment on attachment 8715766 [details] [diff] [review]
bug1245029.v1.patch (applied feedback comments)

Review of attachment 8715766 [details] [diff] [review]:
-----------------------------------------------------------------

And thanks again! R+ with nits.

Sorry to fight you on silly `Enabled/Disabled` naming! If you still disagree, then feel free to ignore my nits and land what you feel OK with, but please make it consistent (see AddonsComponent's initial state).

(In reply to Julian Descottes [:jdescottes] from comment #6)
> Any chance bug 1210778 will land soon ? I'm afraid that the patches will
> have to be reworked if we land the refactor before :(

We use that bug to formalize a good way to make (and keep) the DevTools accessible. Right now, it uses a special Python mochitest, and we're still trying to figure out if we can make it a JS mochitest, so that it integrates better with our other tests. Which means it's less close to landing than your patch.

Testing the accessibility is actually the hard part of that bug, whereas tweaking the DOM here and there is less complex when you know about accessibility. This is why I suggested you have a look at Yura's patch and see what the accessible DOM looks like, then use that insight to make your new React components accessible (for this patch, the category headers, and maybe soon the left-side menu).

And in general, patches that land first always win, that's how the game works.

> > at some point I'd like us to rename all the
> > "ThingComponent"s to just "Thing"s.
> > 
> 
> Great, let's do that in a separate part in this bug!

Sounds good!

> > Do you know if this workaround works in all situations?
> 
> It should work. An alternative would be to pass the window here as a prop.
> Looking at the memory module, they actually have access to window in React
> components, but they use a different loader.
> See BrowserLoaderModule at
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/memory/
> initializer.js#8
> 
> For now I would be tempted to keep the workaround, move forward with the
> refactoring and address this in another chunk of the same bug.

Fine by me. I don't really like the window-prop idea, so maybe doing it like the memory module is the best idea? (I didn't use BrowserLoader originally because my code seemed to work without it, but the memory module does seem to have cleaner React code thanks to it).

> I wanted to keep everything related to addons preferences in the same class
> because it will get more complex with Bug 1227137. I moved it to
> AddonsControls for now, we can see how 1227137 turns out later.

Maybe that bug can be implemented in AddonsControls? But you're right, let's see about that later.

> I was unsure about this one. In the memory tool, checkboxes rely on internal
> state instead of reading the DOM value, so I did the same here. I'm fine
> with either :)

Call it "Fear, Uncertainty and Doubt", but I do prefer checkboxes to control a value directly, rather than blindly toggling an internal state we assume is wired the right way :)

> > Nit: Why did you change the optional `debugDisabled` back to a mandatory
> > `debugEnabled` that needs to be `true` here?
> 
> I feel it's harder to reason with negative booleans, but maybe it's just me.
> That's why I reverted to debugEnabled here. I can change it back if you want.

The problem with a `debugEnabled` is that by default (e.g. if you forget to specify it or there is a minor bug), all Debug buttons are disabled, except if you explicitly ask for one to be enabled and everything works as expected.

In my opinion, it's rather the case that all Debug buttons should be enabled by default, except in a few cases where you should explicitly set a `debugDisabled` parameter (e.g. for Add-ons when the prefs are not right).

I'm just trying to avoid a default behavior that requires a bunch of mandatory parameters properly set to `true` in order to work. Much better to address edge cases with optional parameters where needed, and keep the default behavior simple and easy to achieve.

::: devtools/client/aboutdebugging/components/addons-controls.js
@@ +20,5 @@
> +exports.AddonsControlsComponent = React.createClass({
> +  displayName: "AddonsControlsComponent",
> +
> +  render() {
> +    let { debugEnabled } = this.props;

Nit: `debugDisabled`?

@@ +28,5 @@
> +        "div", { className: "addons-options" },
> +          React.createElement("input", {
> +            id: "enable-addon-debugging",
> +            type: "checkbox",
> +            checked: debugEnabled,

Nit: `!debugDisabled`?

@@ +45,5 @@
> +  },
> +
> +  onEnableAddonDebuggingChange(event) {
> +    let enabled = event.target.checked;
> +    Services.prefs.setBoolPref("devtools.chrome.enabled", enabled);

Idea: Later this could control both "chrome" and "remote" prefs:

    if (enabled) {
      set("chrome", true);
      set("remote", true);
    } else {
      set("chrome", true);
      // No need to touch "remote" to explicitely disable Add-ons debugging.
    }

::: devtools/client/aboutdebugging/components/addons.js
@@ +47,5 @@
>    },
>  
>    render() {
> +    let { client } = this.props;
> +    let { debugEnabled, extensions: targets } = this.state;

Nit: `debugDisabled`?

@@ +54,5 @@
> +    return React.createElement(
> +      "div", null,
> +        React.createElement(TabHeaderComponent, {
> +          id: "addons-header", name: Strings.GetStringFromName("addons")}),
> +        React.createElement(AddonsControlsComponent, { debugEnabled }),

Nit: `debugDisabled`?

@@ +58,5 @@
> +        React.createElement(AddonsControlsComponent, { debugEnabled }),
> +        React.createElement(
> +          "div", { id: "addons", className: "inverted-icons" },
> +          React.createElement(TargetListComponent,
> +            { name, targets, client, debugEnabled })

Nit: `debugDisabled`?

@@ +66,5 @@
>  
>    update() {
> +    this.updateDebugStatus();
> +    this.updateAddonsList();
> +  },

Nit: `update()` is not called anywhere else than in `componentDidMount()`. Please inline it there.

@@ +70,5 @@
> +  },
> +
> +  updateDebugStatus() {
> +    this.setState({
> +      debugEnabled: Services.prefs.getBoolPref("devtools.chrome.enabled")

Nit: `debugDisabled: !Services...`? Later this can be a `!chrome || !remote`.

::: devtools/client/aboutdebugging/components/target-list.js
@@ +22,5 @@
>    displayName: "TargetListComponent",
>  
>    render() {
>      let client = this.props.client;
> +    let debugEnabled = this.props.debugEnabled;

Nit: `debugDisabled`?

@@ +27,3 @@
>      let targets = this.props.targets.sort(LocaleCompare).map(target => {
>        return React.createElement(TargetComponent,
> +        { client, target, debugEnabled });

Nit: `debugDisabled`?

::: devtools/client/aboutdebugging/components/target.js
@@ +51,5 @@
>      }
>    },
>  
>    render() {
> +    let { target, debugEnabled } = this.props;

Nit: `debugDisabled`?

@@ +63,5 @@
>        ),
>        React.createElement("button", {
>          className: "debug-button",
>          onClick: this.debug,
> +        disabled: !debugEnabled,

Nit: `debugDisabled`?

::: devtools/client/aboutdebugging/components/workers.js
@@ +31,5 @@
>          service: [],
>          shared: [],
>          other: []
> +      },
> +      debugEnabled: true

Nit: No need to specify anything here if Debug is enabled by default.

@@ +50,5 @@
>    },
>  
>    render() {
> +    let { client } = this.props;
> +    let { workers, debugEnabled } = this.state;

Nit: No need to have a `debug{En,Dis}abled` in the state here.

@@ +53,5 @@
> +    let { client } = this.props;
> +    let { workers, debugEnabled } = this.state;
> +
> +    return React.createElement(
> +      "div", { id: "workers-wrapper" },

Nit: Is this ID used anywhere?

@@ +61,5 @@
> +          "div", { id: "workers", className: "inverted-icons" },
> +          React.createElement(TargetListComponent, {
> +            id: "service-workers",
> +            name: Strings.GetStringFromName("serviceWorkers"),
> +            targets: workers.service, client, debugEnabled }),

Nit: No need to specify debugging status here if Debug is enabled by default.

@@ +65,5 @@
> +            targets: workers.service, client, debugEnabled }),
> +          React.createElement(TargetListComponent, {
> +            id: "shared-workers",
> +            name: Strings.GetStringFromName("sharedWorkers"),
> +            targets: workers.shared, client, debugEnabled }),

Nit: No need to specify debugging status here if Debug is enabled by default.

@@ +69,5 @@
> +            targets: workers.shared, client, debugEnabled }),
> +          React.createElement(TargetListComponent, {
> +            id: "other-workers",
> +            name: Strings.GetStringFromName("otherWorkers"),
> +            targets: workers.other, client, debugEnabled }))

Nit: No need to specify debugging status here if Debug is enabled by default.
Attachment #8715766 - Flags: review?(poirot.alex)
Attachment #8715766 - Flags: review?(janx)
Attachment #8715766 - Flags: review+
Comment on attachment 8715766 [details] [diff] [review]
bug1245029.v1.patch (applied feedback comments)

(Whoops, mid-air collision, restoring ochameau's r+.)
Attachment #8715766 - Flags: review?(poirot.alex) → review+
Thanks for the reviews! I switched to debugDisabled instead of debugEnabled everywhere, and removed it where it was no longer needed.

Carry over r+ from janx & ochameau.
New try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d58511741d5
Attachment #8715766 - Attachment is obsolete: true
Attachment #8716001 - Flags: review+
Attached patch bug1245029.part2.wip.patch (obsolete) — — Splinter Review 
This commit migrates all the remaining UI parts to React. 
(still work in progress)

I did a first try push with a similar patch, and had timeouts on Windows platforms for one the tests. Still need to investigate this before sending to review.

Also need to apply a11y changes.

try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc8c656579e6
Comment on attachment 8716091 [details] [diff] [review]
bug1245029.part2.wip.patch

Review of attachment 8716091 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/aboutdebugging/aboutdebugging.css
@@ +24,5 @@
>    align-content: center;
>  }
>  
> +/* This rule is copied from .category[selected] in in-content/common.css */
> +/* because we cannot generate custom attributes ("selected") from React. */

The React doc says otherwise: https://facebook.github.io/react/docs/tags-and-attributes.html#html-attributes

I read the code in react.js, and selected is considered a boolean attribute, which means that `{ selected: true }` will cause the DOM to have `selected=""` [0], and `{ selected: false }` will cause the DOM not to have that attribute at all.

[0] OK it's not `selected="true"`, but `selected=""` still works with the `.category[selected]` querySelector.
(In reply to Jan Keromnes [:janx] from comment #14)
> Comment on attachment 8716091 [details] [diff] [review]
> bug1245029.part2.wip.patch
> 
> Review of attachment 8716091 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/aboutdebugging/aboutdebugging.css
> @@ +24,5 @@
> >    align-content: center;
> >  }
> >  
> > +/* This rule is copied from .category[selected] in in-content/common.css */
> > +/* because we cannot generate custom attributes ("selected") from React. */
> 
> The React doc says otherwise:
> https://facebook.github.io/react/docs/tags-and-attributes.html#html-
> attributes
> 
> I read the code in react.js, and selected is considered a boolean attribute,
> which means that `{ selected: true }` will cause the DOM to have
> `selected=""` [0], and `{ selected: false }` will cause the DOM not to have
> that attribute at all.
> 
> [0] OK it's not `selected="true"`, but `selected=""` still works with the
> `.category[selected]` querySelector.

I actually tried to use { selected: true/false } before switching to a classname. While it does work correctly the first time the DOM is rendered, subsequent updates are not modifying the attribute. 

I suspect that's because React assumes it can use set the "selected" property of the DOM element instead of setting the attribute. After all selected is supposed to be used for OPTION elements, and not for DIVs.
Isolated the "selected" issue in a fiddle : http://jsfiddle.net/L68wft2p/

Looking at react's code, they treat selected as a boolean attribute, but also as "MUST_USE_PROPERTY", which indeed means DOM operations updating this attribute will directly update the node's property [1]. 

[1] : https://github.com/facebook/react/blob/67e1291ef7d36db56b815ee519b66e26cc5d881b/src/renderers/dom/shared/DOMPropertyOperations.js#L172)

That being said I'm open to alternatives to avoid duplicating the CSS rules, that's definitely one of the parts I want to spend more time on before sending for review.
We discussed the issue with the selected attribute on IRC, I will try to summarize it here.

Having a "selected" attribute is needed for accessibility (an additional aria-selected should also be added).

The selected attribute is treated by React as a boolean (which is fine) and as a property (which is not fine). This means that when updating the DOM, react will actually do domElement.selected = true/false, which does not modify the HTML attribute "selected".

Below I go over the workarounds that have been tried so far.

Treat category div as Custom Element
====================================

As a workaround, we could set the is attribute ("{ is: 'div-selected' }") on the ".category" div. This forces React to treat the element as a custom element, for which attributes are not processed in the same way as for HTML elements.

This allows to use { selected: selected || null } in order to set/remove the attribute.
Since attributes are no longer processed as for a usual DOM element, React no longer translates "className" as the "class" attribute (and will probably not rely on classList methods if modifying the attribute).

Use OPTION instead of DIV
==========================
When I tried, React refused to render any markup inside the option. Also this doesn't solve the styling issue. Since React doesn't update the selected attribute, .category[selected] rule is not applied correctly.

@Jan : What do you think?
Flags: needinfo?(janx)
Try push with an updated version of the patch that should work on windows : https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab193f3826d2
Feel free to change toolkit/themes/shared/in-content/common.inc.css so it excludes [selected="false"]
(In reply to Tim Nguyen [:ntim] (mostly busy until Feb. 12th) from comment #19)
> Feel free to change toolkit/themes/shared/in-content/common.inc.css so it
> excludes [selected="false"]

Ok. Just to be clear, this would allow to use {selected: selected} instead of {selected: selected || null}, but the "custom-element" workaround is still needed.
(In reply to Julian Descottes [:jdescottes] from comment #20)
> (In reply to Tim Nguyen [:ntim] (mostly busy until Feb. 12th) from comment
> #19)
> > Feel free to change toolkit/themes/shared/in-content/common.inc.css so it
> > excludes [selected="false"]
> 
> Ok. Just to be clear, this would allow to use {selected: selected} instead
> of {selected: selected || null}, but the "custom-element" workaround is
> still needed.
How about simply adding support for the 'selected' class name in the CSS ?
(In reply to Tim Nguyen [:ntim] (mostly busy until Feb. 12th) from comment #21)
> (In reply to Julian Descottes [:jdescottes] from comment #20)
> > (In reply to Tim Nguyen [:ntim] (mostly busy until Feb. 12th) from comment
> > #19)
> > > Feel free to change toolkit/themes/shared/in-content/common.inc.css so it
> > > excludes [selected="false"]
> > 
> > Ok. Just to be clear, this would allow to use {selected: selected} instead
> > of {selected: selected || null}, but the "custom-element" workaround is
> > still needed.
> How about simply adding support for the 'selected' class name in the CSS ?

The "selected" attribute is mandatory for accessibility (see comment #17). So if we have to support the attribute, let's reuse the existing CSS selector. I didn't upload a new patch yet (still trying to fix issues with tests on windows), but I'm no longer using the "selected" classname for this reason.

If we decide to deal with the attribute issue in another bug, then we could simply modify the CSS. That's actually what I wanted to discuss (whether it was ok for us to update common.inc.css or not) before I learned that the selected attribute was required.
Attached patch bug1245029.part2.v1.patch (obsolete) — — Splinter Review 
Finally managed to reproduce the test issue on windows. New try push should be ok now : https://treeherder.mozilla.org/#/jobs?repo=try&revision=080201f0d9dc

I feel we still have a few items to discuss, so only marking for feedback for now.

Let's agree on :
- what we should do about the "selected" attribute (used custom-element workaround in this commit)
- how to wait for tests to be ready, cf head.js. I feel that what I am doing is a bit hacky, I would prefer to wait for an event triggered by AboutDebugging. I could use some input here.
- category management. I know categories will ultimately be different depending on the client. For now I just have an array defining them in aboutdebugging.js. I figured we could change this when we actually need to support different clients, but let me know if you want to spend more thoughts on it right now.

After this patch lands I would like to land some additional changes : 
- a11y changes
- use BrowserLoader to have access to window
- remove the "Component" part of component names

Let me know what you think! (removing your ni?)
Attachment #8716091 - Attachment is obsolete: true
Flags: needinfo?(janx)
Attachment #8717414 - Flags: feedback?(janx)
(no idea why my patch got added as a comment here :( )

ochameau : Switching my feedback request to you, as :janx is on PTO. Let me know what you think!
(In reply to Julian Descottes [:jdescottes] from comment #23)
> Let's agree on :
> - what we should do about the "selected" attribute (used custom-element
> workaround in this commit)

It's not really clear if React is wrong here. If it is, we should try to patch it upstream.
Are you sure "selected" attribute is really useful for accessibility?
"aria-selected" is. But is "selected" important? I can't find any explicit mention of that in bug 1210778.
aria-* props seems to be properly handled as attributes.
Then I'm wondering, instead of duplicating selected and aria-selected,
how bad would it be to base the code logic only around aria-selected?
(And tweak the css to support [selected] and [aria-selected])

Anyway, I don't think we should block on this.
We could possibly followup to clean up things if we can and remove unecessary workaround.
Another workaround would be to do this:
    componentDidUpdate: function () {
      // Comment about how bad React is.
      var node = this.getDOMNode();
      if (this.state.selected) {
    	  node.setAttribute("selected", "true");
      } else {
        node.removeAttribute("selected");
      }
    },

> - how to wait for tests to be ready, cf head.js. I feel that what I am doing
> is a bit hacky, I would prefer to wait for an event triggered by
> AboutDebugging. I could use some input here.

I imagine it is as hacky as existing code.
I really like the mutation observer trick as it doesn't force us to introduce any test-specific event.
So that we do not pollute production code, nor use special codepath for tests.

We should see exactly what openAboutDebugging does and what are the expected things to be done/waited for.
 * loads about:debugging document
 * wait for DOMContentLoaded
 * AboutDebugging.init
 * DebuggerClient.connect()
 * AboutDebuggingApp.render()
 * XxxxxCategoryComponent.render() and all the sub renders
I think you got it right. We should wait for tab load via addTab, then wait for AboutDebuggingApp to create its DOM. I agree with you, it feel weak, but at the end that what we are waiting for, we want to wait for the DOM to be ready.
But then openAboutDebugging also open the default or custom category and this is more complex.
I imagine we would need some more specific mutation observers for each category.
We already wait for specific mutation in service workers.
We may move that to head.js this kind of code:
function openWorkers(url) {
  let { tab, document } = yield openAboutDebugging("workers");

  let swTab = yield addTab(url);

  let serviceWorkersElement = document.getElementById("service-workers");
  yield waitForMutation(serviceWorkersElement, { childList: true });
}

> - category management. I know categories will ultimately be different
> depending on the client. For now I just have an array defining them in
> aboutdebugging.js. I figured we could change this when we actually need to
> support different clients, but let me know if you want to spend more
> thoughts on it right now.

I don't think it is necessary. Better see that once we start having different clients.
We would first need to convert Addons to use RDP (bug 1243460).
You current abstraction around categories looks great.

> - use BrowserLoader to have access to window

Note that if we could use only <script> for all JS and it could have access to `require` and `window` (like initializer.js) but scope would be merged and we would miss some lazyness...
:yzen : Linked to your work in Bug 1210778, do you know if the category links of aboutdebugging need both aria-selected and selected attributes for accessibility ?
Flags: needinfo?(yzenevich)
Wow, thanks a lot for the detailed feedback!

(In reply to Alexandre Poirot [:ochameau] from comment #26)
> (In reply to Julian Descottes [:jdescottes] from comment #23)
> > Let's agree on :
> > - what we should do about the "selected" attribute (used custom-element
> > workaround in this commit)
> 
> It's not really clear if React is wrong here. If it is, we should try to
> patch it upstream.
> Are you sure "selected" attribute is really useful for accessibility?
> "aria-selected" is. But is "selected" important? I can't find any explicit
> mention of that in bug 1210778.
> aria-* props seems to be properly handled as attributes.
> Then I'm wondering, instead of duplicating selected and aria-selected,
> how bad would it be to base the code logic only around aria-selected?
> (And tweak the css to support [selected] and [aria-selected])
> 
> Anyway, I don't think we should block on this.
> We could possibly followup to clean up things if we can and remove
> unecessary workaround.
> Another workaround would be to do this:
>     componentDidUpdate: function () {
>       // Comment about how bad React is.
>       var node = this.getDOMNode();
>       if (this.state.selected) {
>     	  node.setAttribute("selected", "true");
>       } else {
>         node.removeAttribute("selected");
>       }
>     },

I think it's worth opening an issue to React to see how they feel. Treating "selected" as a property is only valid when used on an <option>. But using "selected" on a <div> can be seen as invalid as well. Worth a shot.

For now (pending the ni? to :yzen) if we have to go for a workaround, I actually prefer yours. It's nicely isolated, easy to remove if needed. Thanks! 
(and yes, aria-* attributes are handled just fine with React, I actually have this ready in another commit)

> > - how to wait for tests to be ready, cf head.js. I feel that what I am doing
> > is a bit hacky, I would prefer to wait for an event triggered by
> > AboutDebugging. I could use some input here.
> 
> I imagine it is as hacky as existing code.
> I really like the mutation observer trick as it doesn't force us to
> introduce any test-specific event.
> So that we do not pollute production code, nor use special codepath for
> tests.
> 
> We should see exactly what openAboutDebugging does and what are the expected
> things to be done/waited for.
>  * loads about:debugging document
>  * wait for DOMContentLoaded
>  * AboutDebugging.init
>  * DebuggerClient.connect()
>  * AboutDebuggingApp.render()
>  * XxxxxCategoryComponent.render() and all the sub renders
> I think you got it right. We should wait for tab load via addTab, then wait
> for AboutDebuggingApp to create its DOM. I agree with you, it feel weak, but
> at the end that what we are waiting for, we want to wait for the DOM to be
> ready.
> But then openAboutDebugging also open the default or custom category and
> this is more complex.
> I imagine we would need some more specific mutation observers for each
> category.
> We already wait for specific mutation in service workers.
> We may move that to head.js this kind of code:
> function openWorkers(url) {
>   let { tab, document } = yield openAboutDebugging("workers");
> 
>   let swTab = yield addTab(url);
> 
>   let serviceWorkersElement = document.getElementById("service-workers");
>   yield waitForMutation(serviceWorkersElement, { childList: true });
> }

In this case, before starting the test we should wait for #service-workers to be available. I guess we could simply do 
// wait for #workers if loading "workers", for #addons if loading "addons"
while (!document.getElementById("workers")) {
  yield waitForMutation(document.body, {childList:true, subTree: true});
}

This way we now the component we expect has been rendered. Waiting for childList in #service-workers seems more like test-logic than initialization-logic, right ?
(In reply to Julian Descottes [:jdescottes] from comment #27)
> :yzen : Linked to your work in Bug 1210778, do you know if the category
> links of aboutdebugging need both aria-selected and selected attributes for
> accessibility ?

Yes, DOM attributes do not translate into aria ones.
Flags: needinfo?(yzenevich)
(In reply to Yura Zenevich [:yzen] from comment #29)
> (In reply to Julian Descottes [:jdescottes] from comment #27)
> > :yzen : Linked to your work in Bug 1210778, do you know if the category
> > links of aboutdebugging need both aria-selected and selected attributes for
> > accessibility ?
> 
> Yes, DOM attributes do not translate into aria ones.

Sorry, I should have been clearer, I understand "aria-selected" is needed. The question was more: is "selected" also needed for accessibility? We would like to remove it, because it's painful to generate it using React.
Flags: needinfo?(yzenevich)
(In reply to Julian Descottes [:jdescottes] from comment #30)
> (In reply to Yura Zenevich [:yzen] from comment #29)
> > (In reply to Julian Descottes [:jdescottes] from comment #27)
> > > :yzen : Linked to your work in Bug 1210778, do you know if the category
> > > links of aboutdebugging need both aria-selected and selected attributes for
> > > accessibility ?
> > 
> > Yes, DOM attributes do not translate into aria ones.
> 
> Sorry, I should have been clearer, I understand "aria-selected" is needed.
> The question was more: is "selected" also needed for accessibility? We would
> like to remove it, because it's painful to generate it using React.

No, if aria-selected is present, it will be used just fine regardless of the native selected attribute being present.
Flags: needinfo?(yzenevich)
Attached patch bug1245029.part2.v2.patch (obsolete) — — Splinter Review 
Thanks for the info Yura!

Updated the patch to remove the selected attribute, and add support for aria-selected in common.inc.css .

I also merged the pending a11y changes, no need to have a separate patch I guess here.
Attachment #8717414 - Attachment is obsolete: true
Attachment #8717414 - Flags: feedback?(poirot.alex)
(In reply to Julian Descottes [:jdescottes] from comment #32)
> 
> Thanks for the info Yura!
> 

Actually thanks Marco and Yura, did not see :MarcoZ answered for :yzen.
May I also suggest not using ARIA attribute rules in CSS as a general rule. There is a high chance of creating issues down the road because of 2 things:
* people misusing ARIA to get desired CSS styling 
* unexpected CSS styling when people use ARIA
It's sort of similar to an idea advocating using classes for styling in favour of using actual DOM element rules.
Attached patch bug1245029.part2.v3.patch (obsolete) — — Splinter Review 
Good point about not using aria attributes in CSS, makes sense. Changed it back to a "selected" classname.
Attachment #8717810 - Attachment is obsolete: true
Comment on attachment 8717905 [details] [diff] [review]
bug1245029.part2.v3.patch

Review of attachment 8717905 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, but I would really like to give it a try.
Do you think you could rebase it ?
I recently landed changes to require() and I get various conflicts.

::: devtools/client/aboutdebugging/components/category-link.js
@@ +31,5 @@
> +  onKeyUp(event) {
> +    if (event.key === " " || event.key === "Enter") {
> +      this.props.onCategoryClicked(this.props.id);
> +    }
> +  }

Looks like a leftover?

::: devtools/client/aboutdebugging/components/category-list.js
@@ +24,5 @@
> +    });
> +
> +    return React.createElement("div", { id: "categories" }, links);
> +  },
> +});

Is this component really useful?
Would it make AboutDebuggingApp much more complex if we inline it there?

::: devtools/client/aboutdebugging/test/head.js
@@ +20,5 @@
>  registerCleanupFunction(() => {
>    DevToolsUtils.testing = false;
>  });
>  
> +function* openAboutDebugging(page) {

Don't we have to do that to be fully correct?
let openAboutDebugging = Task.async(function *() {
  ...
});

@@ +35,5 @@
> +  if (!document.querySelector(".root")) {
> +    yield waitForMutation(document.body, { childList: true });
> +  }
> +
> +  return { tab, browser, document };

`browser` doesn't seem to be used.
Attachment #8717905 - Flags: review?(poirot.alex)
rebased, carry over r+
Attachment #8716001 - Attachment is obsolete: true
Attachment #8718406 - Flags: review+
Rebased
Attachment #8717905 - Attachment is obsolete: true
Attachment #8717905 - Flags: review?(janx)
Attachment #8718407 - Flags: review?(poirot.alex)
Attachment #8718407 - Flags: review?(janx)
Thanks for the review, I just attached rebased versions of both patches, you should be able to import them now. Let me know if there is any issue!

(In reply to Alexandre Poirot [:ochameau] from comment #37)
> ::: devtools/client/aboutdebugging/components/category-link.js
> @@ +31,5 @@
> > +  onKeyUp(event) {
> > +    if (event.key === " " || event.key === "Enter") {
> > +      this.props.onCategoryClicked(this.props.id);
> > +    }
> > +  }
> 
> Looks like a leftover?

Good catch, I took this from Bug 1210778 (accessibility changes) but there's no way the element can actually get the focus for now. Let's remove it.

> 
> ::: devtools/client/aboutdebugging/components/category-list.js
> @@ +24,5 @@
> > +    });
> > +
> > +    return React.createElement("div", { id: "categories" }, links);
> > +  },
> > +});
> 
> Is this component really useful?
> Would it make AboutDebuggingApp much more complex if we inline it there?
> 

You're right turns out there is no logic performed here, I will inline it.

> ::: devtools/client/aboutdebugging/test/head.js
> @@ +20,5 @@
> >  registerCleanupFunction(() => {
> >    DevToolsUtils.testing = false;
> >  });
> >  
> > +function* openAboutDebugging(page) {
> 
> Don't we have to do that to be fully correct?
> let openAboutDebugging = Task.async(function *() {
>   ...
> });

Since the tests are all wrapped in a Task, it's not mandatory to have another one here. The wrapping task will make sure to call the generator until completion. Maybe I don't get the issue ... feel free to ping me to discuss this, I'm still getting used to using Task.
Comment on attachment 8718407 [details] [diff] [review]
bug1245029.part2.v4.patch (rebased)

Review of attachment 8718407 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

::: devtools/client/aboutdebugging/components/target-list.js
@@ +30,5 @@
>      return (
>        React.createElement("div", { id: this.props.id, className: "targets" },
> +        React.createElement("h2", null, this.props.name),
> +        targets.length > 0 ?
> +          React.createElement("ul", { className: "target-list" }, targets) :

With the usage of ul > li, we get extra left margin/padding.
I'm not sure it is wanted.
It looks different from current about:debugging.
Attachment #8718407 - Flags: review?(poirot.alex) → review+
(In reply to Julian Descottes [:jdescottes] from comment #40)
> > ::: devtools/client/aboutdebugging/test/head.js
> > @@ +20,5 @@
> > >  registerCleanupFunction(() => {
> > >    DevToolsUtils.testing = false;
> > >  });
> > >  
> > > +function* openAboutDebugging(page) {
> > 
> > Don't we have to do that to be fully correct?
> > let openAboutDebugging = Task.async(function *() {
> >   ...
> > });
> 
> Since the tests are all wrapped in a Task, it's not mandatory to have
> another one here. The wrapping task will make sure to call the generator
> until completion. Maybe I don't get the issue ... feel free to ping me to
> discuss this, I'm still getting used to using Task.

Ah. I'm still hesitant when it comes to Task, it always as a part of magic to me.
Looks like you are right.
I removed the ul>li. I prefer to focus on the React refactor here. Let's deal with the accessibility changes in the dedicated bug unless they come for free. 

Also removed an unwanted "inverted-icon" class on the addons container, making the green jigsaw pieces sad-looking.

Try push : https://treeherder.mozilla.org/#/jobs?repo=try&revision=596de1d3f7d6
Attachment #8718407 - Attachment is obsolete: true
Attachment #8718407 - Flags: review?(janx)
Attachment #8718646 - Flags: review?(janx)
Blocks: 1227137
Comment on attachment 8718646 [details] [diff] [review]
bug1245029.part2.v5.patch (applied review comments from :ochameau)

Review of attachment 8718646 [details] [diff] [review]:
-----------------------------------------------------------------

Looking great! And it applies cleanly, works great, and doesn't seem to break any tests (modulo the unrelated-looking oranges on try).

Thanks Julian for making a lot of progress so quickly, and thanks Alex for the review while I was away!

As usual, mostly a lot of nits:

(In reply to Julian Descottes [:jdescottes] from comment #23)
> Let's agree on :
> - what we should do about the "selected" attribute (used custom-element
> workaround in this commit)

Given the interim comments, I agree to drop the "selected" attribute, because it is superseeded by "aria-selected" for accessibility, and the styling can be handled by the ".selected" class.

Just curious, did you end up filing a React issue anyway?

> After this patch lands I would like to land some additional changes : 
> - a11y changes

It's great if you're willing to help with this. Thanks!

> - use BrowserLoader to have access to window

Good idea, and maybe in general move closer to the perf tools' React-style? (e.g. their `.div()` helpers are a lot more readable than our `React.createElement("div")` calls).

> - remove the "Component" part of component names

By doing that, maybe we could also rename "{Addons,Workers}Component" to something like "{Addons,Workers}Tab"?

::: devtools/client/aboutdebugging/aboutdebugging.css
@@ +27,5 @@
>  .category-name {
>    cursor: default;
>  }
>  
> +.root {

Nit: Maybe a name that more closely resembles the "AboutDebugging" component would make more sense? (e.g. ".app"). I don't know, you decide :)

::: devtools/client/aboutdebugging/aboutdebugging.js
@@ +25,5 @@
> +  { id: "workers", name: Strings.GetStringFromName("workers"),
> +    icon: "chrome://devtools/skin/images/debugging-workers.svg",
> +    component: WorkersComponent },
> +];
> +const defaultCategoryId = "addons";

Note: This tab descriptor array is OK for now, but it will probably have to change when we make it more dynamic with remote runtimes.

@@ +71,5 @@
> +    let categoryId = this.props.window.location.hash.substr(1);
> +
> +    let isValid = categories.some(c => c.id == categoryId);
> +    if (!isValid) {
> +      this.selectCategory(defaultCategoryId);

Nit: It seems weird that we use two different ways to effect a category change inside this function: 1) state-change + rerender, 2) hashchange to get called again. Maybe we could have only one, e.g. option 1?

::: devtools/client/aboutdebugging/components/category-link.js
@@ +9,5 @@
> +loader.lazyRequireGetter(this, "React",
> +  "devtools/client/shared/vendor/react");
> +
> +exports.CategoryLinkComponent = React.createClass({
> +  displayName: "CategoryLinkComponent",

Nit: Similar to comment 5, maybe this is more a "TabLink" (or "TabMenuEntry") than a "CategoryLink"?

Nit: Also, why not create a "TabMenu" component directly that creates all the links/entries inline? You could pass it a "tabs" descriptor array prop. Not sure how much more complex that would make the onClick-listener.

::: devtools/client/aboutdebugging/components/target-list.js
@@ +30,5 @@
>      return (
>        React.createElement("div", { id: this.props.id, className: "targets" },
>          React.createElement("h4", null, this.props.name),
> +        targets.length > 0 ?
> +          React.createElement("div", { className: "target-list" }, targets) :

Nit: Why change this to add an extra div? It doesn't seem useful.

::: devtools/client/aboutdebugging/initializer.js
@@ +23,3 @@
>    init() {
>      let telemetry = this._telemetry = new Telemetry();
>      telemetry.toolOpened("aboutdebugging");

Nit: You don't have to do this now, but eventually I think we'll have to pass `telemetry` down to components as a prop, so that we can track usage of the different components. Knowing this, I think it would make more sense to create the object and call `toolOpened()` directly from within the "AboutDebugging" component, but I don't feel too strongly about it (feel free to ignore this nit if you disagree).

::: devtools/client/aboutdebugging/moz.build
@@ +9,5 @@
>  ]
>  
> +DevToolsModules(
> +    'aboutdebugging.js',
> +)

Nit: Since you're making `aboutdebugging.js` a React component, why not simply move it into 'components/' and remove this special entry?
Attachment #8718646 - Flags: review?(janx) → feedback+
Thanks for the review, some additional questions for you below :)

(In reply to Jan Keromnes [:janx] from comment #44)
> Comment on attachment 8718646 [details] [diff] [review]
> bug1245029.part2.v5.patch (applied review comments from :ochameau)
> 
> Review of attachment 8718646 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/aboutdebugging/aboutdebugging.css
> @@ +27,5 @@
> >  .category-name {
> >    cursor: default;
> >  }
> >  
> > +.root {
> 
> Nit: Maybe a name that more closely resembles the "AboutDebugging" component
> would make more sense? (e.g. ".app"). I don't know, you decide :)
> 

Sure, was not happy about "root"!

> @@ +71,5 @@
> > +    let categoryId = this.props.window.location.hash.substr(1);
> > +
> > +    let isValid = categories.some(c => c.id == categoryId);
> > +    if (!isValid) {
> > +      this.selectCategory(defaultCategoryId);
> 
> Nit: It seems weird that we use two different ways to effect a category
> change inside this function: 1) state-change + rerender, 2) hashchange to
> get called again. Maybe we could have only one, e.g. option 1?
> 

I agree, it's weird. The goal is for the address bar to reflect the current view. If the current category is invalid, we change the hash (via selectCategory) in order to go to the default category. In the end we still end in onHashChange and perform a setState, after one more hop. 
Otherwise the addons view would be displayed while still showing invalid URLs. Maybe I am missing an easier solution that would address this though?

Could change the naming to make the intent clearer: rename "selectCategory" as "navigateToCategory" ... add comments. What do you think ? 

> ::: devtools/client/aboutdebugging/components/category-link.js
> @@ +9,5 @@
> > +loader.lazyRequireGetter(this, "React",
> > +  "devtools/client/shared/vendor/react");
> > +
> > +exports.CategoryLinkComponent = React.createClass({
> > +  displayName: "CategoryLinkComponent",
> 
> Nit: Similar to comment 5, maybe this is more a "TabLink" (or
> "TabMenuEntry") than a "CategoryLink"?
> 

sure!

> Nit: Also, why not create a "TabMenu" component directly that creates all
> the links/entries inline? You could pass it a "tabs" descriptor array prop.
> Not sure how much more complex that would make the onClick-listener.
> 

There was an intermediary component called category-list, which was taking the tabs descriptor as a prop. We inlined it in aboutdebugging.js because it had not much value between the app and the tab-entry. The extra complexity makes me unsure about inlining tab-entry in an upper level. Can you confirm?

> ::: devtools/client/aboutdebugging/components/target-list.js
> @@ +30,5 @@
> >      return (
> >        React.createElement("div", { id: this.props.id, className: "targets" },
> >          React.createElement("h4", null, this.props.name),
> > +        targets.length > 0 ?
> > +          React.createElement("div", { className: "target-list" }, targets) :
> 
> Nit: Why change this to add an extra div? It doesn't seem useful.

Good catch!

> 
> ::: devtools/client/aboutdebugging/initializer.js
> @@ +23,3 @@
> >    init() {
> >      let telemetry = this._telemetry = new Telemetry();
> >      telemetry.toolOpened("aboutdebugging");
> 
> Nit: You don't have to do this now, but eventually I think we'll have to
> pass `telemetry` down to components as a prop, so that we can track usage of
> the different components. Knowing this, I think it would make more sense to
> create the object and call `toolOpened()` directly from within the
> "AboutDebugging" component, but I don't feel too strongly about it (feel
> free to ignore this nit if you disagree).
> 

Not sure this matters, but wouldn't it be nice to keep the creation outside of the component for testing (if we ever write component tests for instance). I would pass it as a prop and call toolOpened from the componentDidMount here. Fine with you?

> ::: devtools/client/aboutdebugging/moz.build
> @@ +9,5 @@
> >  ]
> >  
> > +DevToolsModules(
> > +    'aboutdebugging.js',
> > +)
> 
> Nit: Since you're making `aboutdebugging.js` a React component, why not
> simply move it into 'components/' and remove this special entry?

Part of it was to stick to what was done in the memory tool, the rest was me not really knowing how a react app should be organized. I will move it!

Jan : I'll wait for your answers here before submitting another patch for review.
Flags: needinfo?(janx)
> > @@ +71,5 @@
> > > +    let categoryId = this.props.window.location.hash.substr(1);
> > > +
> > > +    let isValid = categories.some(c => c.id == categoryId);
> > > +    if (!isValid) {
> > > +      this.selectCategory(defaultCategoryId);
> > 
> > Nit: It seems weird that we use two different ways to effect a category
> > change inside this function: 1) state-change + rerender, 2) hashchange to
> > get called again. Maybe we could have only one, e.g. option 1?
> > 
> 
> I agree, it's weird. The goal is for the address bar to reflect the current
> view. If the current category is invalid, we change the hash (via
> selectCategory) in order to go to the default category. In the end we still
> end in onHashChange and perform a setState, after one more hop. 
> Otherwise the addons view would be displayed while still showing invalid
> URLs. Maybe I am missing an easier solution that would address this though?
> 
> Could change the naming to make the intent clearer: rename "selectCategory"
> as "navigateToCategory" ... add comments. What do you think ?

Ah, I missed that subtle difference. Maybe just add a comment before the line that got me confused? Something like "// Invalid URL hash, change it to the default tab ID.".

Also maybe a general s/Category/Tab/ could help with clarity (my fault for choosing the wrong word originally).

> > Nit: Also, why not create a "TabMenu" component directly that creates all
> > the links/entries inline? You could pass it a "tabs" descriptor array prop.
> > Not sure how much more complex that would make the onClick-listener.
> > 
> 
> There was an intermediary component called category-list, which was taking
> the tabs descriptor as a prop. We inlined it in aboutdebugging.js because it
> had not much value between the app and the tab-entry. The extra complexity
> makes me unsure about inlining tab-entry in an upper level. Can you confirm?

I did notice that category-list got inlined into aboutdebugging.js, but maybe it would have been better to inline category-link into category-list and rename it tab-menu?

A TabMenu component that controls the hash and just calls back to AboutDebugging to tell it which TabComponent to show doesn't sound more complex to me, but maybe I'm missing something. If you're confident that AboutDebugging should control the hash and the tab links directly itself, then I'll stop arguing about it :)

> > Nit: You don't have to do this now, but eventually I think we'll have to
> > pass `telemetry` down to components as a prop, so that we can track usage of
> > the different components. Knowing this, I think it would make more sense to
> > create the object and call `toolOpened()` directly from within the
> > "AboutDebugging" component, but I don't feel too strongly about it (feel
> > free to ignore this nit if you disagree).
> > 
> 
> Not sure this matters, but wouldn't it be nice to keep the creation outside
> of the component for testing (if we ever write component tests for
> instance). I would pass it as a prop and call toolOpened from the
> componentDidMount here. Fine with you?

I mostly care about components being able to log their own telemetry. Creating the object outside and passing it as a prop might help test the AboutDebugging component itself, so fine with me.

> > > +DevToolsModules(
> > > +    'aboutdebugging.js',
> > > +)
> > 
> > Nit: Since you're making `aboutdebugging.js` a React component, why not
> > simply move it into 'components/' and remove this special entry?
> 
> Part of it was to stick to what was done in the memory tool, the rest was me
> not really knowing how a react app should be organized. I will move it!

Maybe there is a hidden benefit why the memory tool keeps the main component outside the components/ directory, but I don't see it (its name should be enough to indicate that it's the main component). I'm inclined to try and move it, unless we find such a hidden benefit.
Flags: needinfo?(janx)
Jan: this new version should address most of your comments. Regarding the menu/menu-entry split, I reintroduced an intermediary "tab-menu" component. 

I prefer to keep the hash management in aboutdebugging.js because I feel it's more the responsibility of the application to know these things.
Inlining "menu-entry" in "menu" means passing "tabId" to the onClick handler. Keeping a separate component for "menu-entry" allows to use props instead.
Attachment #8718646 - Attachment is obsolete: true
Attachment #8720894 - Flags: review?(janx)
Comment on attachment 8720894 [details] [diff] [review]
bug1245029.part2.v6.patch (rebased & applied review comments)

Review of attachment 8720894 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, thanks! I'm very happy to see this bug getting resolved.

For potential follow-ups, I've identified:
- Additional accessibility fixes
- More general React-improvements, e.g.: BrowserLoader to access `window`; use of helpers like `.div()`; consider wrapping components with `createFactory` (bug 1207997)
- Renaming all "ThingComponent" classes to just "Thing" (and maybe "{Addons,Workers}" to something like "{Addons,Workers}Tab"?).

Please let me know if you want to tackle any of these, or if you have more follow-up ideas.
Attachment #8720894 - Flags: review?(janx) → review+
Rebased. Carry over r+.
Attachment #8718406 - Attachment is obsolete: true
Attachment #8721242 - Flags: review+
Rebased. carry over r+
Attachment #8720894 - Attachment is obsolete: true
Attachment #8721243 - Flags: review+
(In reply to Jan Keromnes [:janx] from comment #48)
> Comment on attachment 8720894 [details] [diff] [review]
> bug1245029.part2.v6.patch (rebased & applied review comments)
> 
> Review of attachment 8720894 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me, thanks! I'm very happy to see this bug getting resolved.
> 
> For potential follow-ups, I've identified:
> - Additional accessibility fixes
> - More general React-improvements, e.g.: BrowserLoader to access `window`;
> use of helpers like `.div()`; consider wrapping components with
> `createFactory` (bug 1207997)
> - Renaming all "ThingComponent" classes to just "Thing" (and maybe
> "{Addons,Workers}" to something like "{Addons,Workers}Tab"?).
> 
> Please let me know if you want to tackle any of these, or if you have more
> follow-up ideas.

Thanks for the review, very happy to have this refactor out of the way as well!

Created Bug 1249585 to rename ThingComponent -> Thing.
I've assigned myself to 1207997 and 1249585 for now.
Blocks: 1249585
Blocks: 1250002
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f6e9b14dc9a3
https://hg.mozilla.org/mozilla-central/rev/7145c3ebe5fd
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: