Closed Bug 1284221 Opened 7 years ago Closed 7 years ago

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

Categories

(DevTools :: Inspector, defect, P2)

49 Branch
defect

Tracking

(firefox50 fixed)

RESOLVED FIXED
Firefox 50
Tracking Status
firefox50 --- fixed

People

(Reporter: pbro, Assigned: pbro)

Details

Attachments

(1 file)

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: 7 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
Component: Inspector: Fonts → Inspector
You need to log in before you can comment on or make changes to this bug.