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

RESOLVED FIXED in Firefox 50

Status

defect
P2
normal
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: pbro, Assigned: pbro)

Tracking

49 Branch
Firefox 50

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(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
Status: NEW → ASSIGNED
Priority: -- → P2
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
Attachment #8767640 - Flags: review?(jdescottes) → review+
Thanks for the review. I have made the changes requested in comment 2.
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/76746a3c04de
Move the font panel to the last position in the sidebar; r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/76746a3c04de
Status: ASSIGNED → RESOLVED
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

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