1284221 - Move the font inspector panel as the last tab in the inspector sidebar

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Patrick Brosset <:pbro>]

It's one of the lesser used tools, and we don't want the sidebar to look too crowded, so let's move it last.

Comment 1

[Patrick Brosset <:pbro>]

Attachment: Bug 1284221 - Move the font panel to the last position in the sidebar;

Review commit: https://reviewboard.mozilla.org/r/62092/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62092/

Comment 2

[Julian Descottes [:jdescottes]]

Comment on attachment 8767640 [details]
Bug 1284221 - Move the font panel to the last position in the sidebar;

https://reviewboard.mozilla.org/r/62092/#review58798

Looks good to me, thanks for updating the implementation!

::: devtools/client/framework/sidebar.js:202
(Diff revision 1)
>      // The auto-checking of menuitems in this menu doesn't work, so let's do
>      // it manually
>      item.setAttribute("autocheck", false);
>  
> -    this._allTabsBtn.querySelector("menupopup").appendChild(item);
> +    let menu = this._allTabsBtn.querySelector("menupopup");
> +    let referenceItem = menu.querySelector(`#${idPrefix}${options.insertBefore}`);

options.insertBefore defaults to undefined. This means we will actually query "#sidebar-alltabs-item-undefined". It is unlikely to clash with an actual id, but it could.

To be on the safe side I would structure this code as follows:

if (options.insertBefore) {
  let referenceItem = menu.querySelector(`#${idPrefix}${options.insertBefore}`);
  menu.insertBefore(item, referenceItem);
} else {
  menu.appendChild(item);
}

If referenceItem is falsy, insertBefore will behave in the same way as appendChild, so we are safe even if an invalid id is provided.

::: devtools/client/framework/sidebar.js:226
(Diff revision 1)
>     *
> -   * @param {string} tab uniq id
> -   * @param {string} url
> +   * @param {string} id The unique id for this tab.
> +   * @param {string} url The URL of the document to load in this new tab.
> +   * @param {Object} options A set of options for this new tab:
> +   * - {Boolean} selected Set to true to make this new tab selected by default.
> +   * - {Number} insertBefore By default, the new tab is appended at the end of the

nit: s/{Number}/{String}

::: devtools/client/framework/sidebar.js:244
(Diff revision 1)
>      tab.setAttribute("id", this.TAB_ID_PREFIX + id);
>      tab.setAttribute("crop", "end");
> +    // Avoid showing "undefined" while the tab is loading
> +    tab.setAttribute("label", "");
> +
> +    let referenceTab = this.getTab(options.insertBefore);

same remark as for the other if statements

::: devtools/client/framework/sidebar.js:276
(Diff revision 1)
>  
>      let tabpanel = this._panelDoc.createElementNS(XULNS, "tabpanel");
>      tabpanel.setAttribute("id", this.TABPANEL_ID_PREFIX + id);
>      tabpanel.appendChild(iframe);
> +
> +    let referenceTabpanel = this.getTabPanel(options.insertBefore);

same remark as for the other if statements

Comment 3

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4499814e5e69&group_state=expanded

Comment 4

[Patrick Brosset <:pbro>]

Thanks for the review. I have made the changes requested in comment 2.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/76746a3c04de
Move the font panel to the last position in the sidebar; r=jdescottes

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/76746a3c04de

Comment 7

[Rezaul Huque Nayeem]

Reproduced this bug in firefox nightly 50.0a1 (2016-07-04) with windows 10 (64 bit)

Verified this bug as fixed with latest firefox nightly 50.0a1 (Build ID: 20160728030208)
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:50.0) Gecko/20100101 Firefox/50.0

Comment 8

[Mohammad Maruf Rahman]

This bug was about to Move the font inspector panel as the last tab in the inspector sidebar and I have seen it being implemented with Nightly 50.0a1 in Ubuntu 16.04 LTS 64 Bit !

Build   ID   20160801030227
User Agent   Mozilla/5.0 (X11; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0

[bugday-20170208]