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

RESOLVED FIXED in Firefox 50


3 years ago
Last year


(Reporter: pbro, Assigned: pbro)


49 Branch
Firefox 50

Firefox Tracking Flags

(firefox50 fixed)



(1 attachment)

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.
Assignee: nobody → pbrosset
Priority: -- → P2
Comment on attachment 8767640 [details]
Bug 1284221 - Move the font panel to the last position in the sidebar;

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 {

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
Attachment #8767640 - Flags: review?(jdescottes) → review+
Thanks for the review. I have made the changes requested in comment 2.
Pushed by
Move the font panel to the last position in the sidebar; r=jdescottes
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
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
QA Whiteboard: [bugday-20160727]
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

Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.