Should be able to drag/drop to reorder devtools tabs

VERIFIED FIXED in Firefox 61

Status

enhancement
P2
normal
VERIFIED FIXED
4 years ago
5 months ago

People

(Reporter: bgrins, Assigned: daisuke)

Tracking

(Depends on 2 bugs, Blocks 2 bugs, {dev-doc-complete})

unspecified
Firefox 61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 verified)

Details

(Whiteboard: [devtools-ux][devtools-toolbar])

Attachments

(8 attachments, 3 obsolete attachments)

198.27 KB, image/gif
Details
217.60 KB, video/mp4
Details
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
2.46 MB, image/gif
Details
People should be able to choose their tool ordering by drag and dropping the tabs in the toolbox.  It should remember their preferred ordering in their profile so it's restored when closing / reopening toolbox.
Whiteboard: [devtools-ux] → [devtools-ux][devtools-toolbar]
Duplicate of this bug: 1282027
See Also: → 1239859
Duplicate of this bug: 1336711
Severity: normal → enhancement
Priority: -- → P2
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment Bug 1443076 #0)
> +++ This bug was initially created as a clone of Bug #1442531 +++
> 
> We might have to allow to reorder the tab menu of toolbox by using drag and
> drops.
> Another browser can reorder with drag and drop. (i.e. Safari and Chrome)
> 
> And we need to save this order to somewhere(prefs?).
> 
> I guess we can use the Atlassian react-beautiful-dnd library for this
> feature.[1]
> 
> [1] https://github.com/atlassian/react-beautiful-dnd
Blocks: 1444299
Assignee: nobody → dakatsuka
Status: NEW → ASSIGNED
I attach a video of 1st prototype I made.
Victoria, how do you think about this behavior?
Flags: needinfo?(victoria)
Wow, that looks great Daisuke!

The one suggestion I have is maybe we should select the tab upon clicking to drag it, to pattern after the Firefox main tabs (it's also how Chrome DevTools and MacOS Finder work, so might be the more expected behavior). I think this way helps remind the user of the contents of the tab when moving it. However, I'm open to other thoughts on this.
Flags: needinfo?(victoria)
Thank you for the feedback, Victoria!

The reason why I tried such the behavior is, I like the thinking which one action does one thing.
However, indeed, most of other cases are as you said. Also, yeah, it helps remind the content.
This time, I'd like to do along your suggestion.
Also, it seems we need to consider bug 1442531.
I'll inform you when I made 2nd prototype.
Thanks!
Sounds good! (And yeah, I can totally understand your original reasoning as well - there's something a little calmer/neater about the one-action/one-thing workflow. In any case, erring on the side of consistency/quick feedback seems like a good way to proceed for now.)
Depends on: 1442531
Hi Victoria,
I made 2nd prototype.
Could you confirm the behavior?
This includes bug 1442531 changes.
Thanks!
Flags: needinfo?(victoria)
Attachment #8967305 - Attachment is obsolete: true
Attachment #8967306 - Attachment is obsolete: true
Attachment #8967307 - Attachment is obsolete: true
Looks great - thanks Daisuke!
Flags: needinfo?(victoria)
Comment on attachment 8968400 [details]
Bug 1226272 - Part 1: Make devtools tab draggable and reorderable.

https://reviewboard.mozilla.org/r/237110/#review243154

Thanks Daisuke, this looks really good. Clearing the r? flag for now because some of the code needs to be invoked from React and I would prefer to do a final review with this issue addressed.

::: devtools/client/framework/toolbox-tabs-order-manager.js:12
(Diff revision 1)
> +/**
> + * Manage the order of devtools tabs.
> + */
> +class ToolboxTabsOrderManager {
> +  constructor(doc) {
> +    this.tabsElement = doc.querySelector(".toolbox-tabs");

The .toolbox-tabs element is created by React, so we should not add event listeners from a class with a lifecycle not driven by React: 
there is no contract that guarantees that .toolbox-tabs will not be destroyed and recreated. 

Could we try to add the event listeners in the toolbox-tabs React component?
You can still keep the drag and drop code in a separate file and call it from the component.

::: devtools/client/framework/toolbox-tabs-order-manager.js:49
(Diff revision 1)
> +      if (tabElement === this.dragTarget) {
> +        continue;
> +      }
> +
> +      const bounds = tabElement.getBoundingClientRect();
> +      const sx = bounds.left + bounds.width * 0.2;

Could we either use a more descriptive variable name, or add a comment to explain its role?

::: devtools/client/framework/toolbox-tabs-order-manager.js:51
(Diff revision 1)
> +      }
> +
> +      const bounds = tabElement.getBoundingClientRect();
> +      const sx = bounds.left + bounds.width * 0.2;
> +
> +      if (sx < e.pageX && e.pageX < sx + bounds.width * 0.6) {

Could we extract sx + bounds.width * 0.6 as a variable and write it as `bounds.left + bounds.width * 0.8;`. 
It will help the reader to understand that we are trying to see if the mouse pointer is between 20% and 80% of the current tabElement.

::: devtools/client/framework/toolbox-tabs-order-manager.js:70
(Diff revision 1)
> +      }
> +    }
> +
> +    let distance = e.pageX - this.dragStartX;
> +
> +    if ((!this.dragTarget.previousSibling && distance < 0) ||

Could we add a comment here explaining which case is covered by this if statement

::: devtools/client/framework/toolbox-tabs-order-manager.js:75
(Diff revision 1)
> +    if ((!this.dragTarget.previousSibling && distance < 0) ||
> +        (!this.dragTarget.nextSibling && distance > 0)) {
> +      distance = 0;
> +    }
> +
> +    this.toolboxContainer.classList.add("tabs-reordering");

Maybe we should add this class in onMouseDown?
Attachment #8968400 - Flags: review?(jdescottes)
Comment on attachment 8968401 [details]
Bug 1226272 - Part 2: Implement saving and loading the tabs order preference.

https://reviewboard.mozilla.org/r/237112/#review243142

This might be impacted by changes requested for part 1 so clearing for now. But overall it looks good!

::: devtools/client/framework/toolbox-tabs-order-manager.js:7
(Diff revision 1)
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
> +const PREFERENCE_NAME = "devtools.toolbox.tabsOrder";

nit: we usually define our "real" consts after the require. Can you move this below require("Services");

::: devtools/client/framework/toolbox-tabs-order-manager.js:32
(Diff revision 1)
>      this.tabsElement.addEventListener("mousedown", this.onMouseDown);
>    }
>  
>    destroy() {
> +    Services.prefs.removeObserver(PREFERENCE_NAME, this.onOrderUpdatedInternal);
>      this.tabsElement.removeEventListener("mousedown", this.onMouseDown);

this.tabsElement might be null here (see Bug 1454755) now that its assignment was moved to enableReorderDnD. Could we guard this with if (this.tabsElement) ?

::: devtools/client/framework/toolbox-tabs-order-manager.js:41
(Diff revision 1)
> +      definitions.sort(definition => {
> +        return -1 * (definition.ordinal == undefined || definition.ordinal < 0
> +                   ? Number.MAX_VALUE
> +                   : definition.ordinal
> +        );
> +      });

This will not sort the array as expected. It will leave the order unchanged: luckily our definitions array is already sorted by hand in the code. A sort function should always take two arguments and return a value comparing the two (-1, 0, +1)

::: devtools/client/framework/toolbox-tabs-order-manager.js:58
(Diff revision 1)
> +    return definitions.sort((a, b) => {
> +      let orderA = toolIds.indexOf(a.id);
> +      let orderB = toolIds.indexOf(b.id);
> +      orderA = orderA < 0 ? Number.MAX_VALUE : orderA;
> +      orderB = orderB < 0 ? Number.MAX_VALUE : orderB;
> +      return orderA === orderB ? -1 : orderA - orderB;

I would have expected `return orderA - orderB;` here. If this is necessary, can you add a comment explaining why we need something more complex?

::: devtools/client/framework/toolbox-tabs-order-manager.js:140
(Diff revision 1)
> +      Services.prefs.setCharPref(PREFERENCE_NAME, pref);
> +    }
> +  }
> +
> +  /**
> +   * onOrderUpdated is called when the order was updated.

Could we pass a callback as a parameter of the constructor, instead of asking consumers to override onOrderUpdated? This would be more consistent with the current codebase.

::: devtools/client/preferences/devtools-client.js:1
(Diff revision 1)
> +

nit: remove this new line
Attachment #8968401 - Flags: review?(jdescottes)
Comment on attachment 8968402 [details]
Bug 1226272 - Part 3: Handle overflowed tabs.

https://reviewboard.mozilla.org/r/237114/#review243156

I will look in more details tomorrow, but this approach might be problematic for the test at devtools/client/framework/test/browser_toolbox_toolbar_overflow.js
This test resizes the window to display all tabs. It then "waits" for all tabs to be rendered before continuing. Locally the test still passes but I am afraid it might fail on CI.

Maybe we can revert the changes in toolbox-tabs and toolbox-tab, and check that nextSibling is the chevron element: 
`...classList.contains("overflowing")` -> `...classList.contains("tools-chevron-menu")`

Quickly testing it seems to work but I might have missed something.
Comment on attachment 8968403 [details]
Bug 1226272 - Part 4: Change the style which is for while dragging.

https://reviewboard.mozilla.org/r/237116/#review243168

::: devtools/client/themes/toolbox.css:303
(Diff revision 1)
> +#toolbox-container.tabs-reordering .devtools-tab:not(.selected):hover .devtools-tab-line {
> +  background: transparent;
> +  opacity: 0;
> +  transition: none;
> +}
> +
> +#toolbox-container.tabs-reordering .devtools-tab.selected {
> +  background-color: var(--theme-toolbar-hover);

On OSX, only keeping the last rule "z-index: 1" seems to be enough to achieve the disered effect, because at all times, the mouse hovers only the selected item. If the other rules are really needed can you add a comment explaining why?
Attachment #8968403 - Flags: review?(jdescottes) → review+
I have a few comments for you to look at, but I just wanted to say that patch works really well. Great job! We should really try to get this one in Firefox 62 :)
This is a good candidate for a follow up: depending on the position of the cursor relative to the dragged element, the UX can feel a bit clunky. 

We should maybe try to compare the "middle" of the dragged element against the overlapped tab (rather than the cursor position).
Comment on attachment 8968400 [details]
Bug 1226272 - Part 1: Make devtools tab draggable and reorderable.

https://reviewboard.mozilla.org/r/237110/#review243154

Thank you very much for the kindly reviewing!

> The .toolbox-tabs element is created by React, so we should not add event listeners from a class with a lifecycle not driven by React: 
> there is no contract that guarantees that .toolbox-tabs will not be destroyed and recreated. 
> 
> Could we try to add the event listeners in the toolbox-tabs React component?
> You can still keep the drag and drop code in a separate file and call it from the component.

Indeed, will try to move into toolbox-tabs component.
The reason why I added to toolbox.js, at first I thought we might need to change the definitions in definitinos.js[1] so as to reorder.

[1] https://searchfox.org/mozilla-central/source/devtools/client/definitions.js

> Could we extract sx + bounds.width * 0.6 as a variable and write it as `bounds.left + bounds.width * 0.8;`. 
> It will help the reader to understand that we are trying to see if the mouse pointer is between 20% and 80% of the current tabElement.

As you said in comment 24, we should change the logic which detects the reorderable target. So, please let me drop this issue once.
But yes, I agree your advice. Thanks!
Comment on attachment 8968401 [details]
Bug 1226272 - Part 2: Implement saving and loading the tabs order preference.

https://reviewboard.mozilla.org/r/237112/#review243142

> This will not sort the array as expected. It will leave the order unchanged: luckily our definitions array is already sorted by hand in the code. A sort function should always take two arguments and return a value comparing the two (-1, 0, +1)

I discussed with :jdescottes on Slack, we will leave it as it is in this time.
(In reply to Julian Descottes [:jdescottes][:julian] from comment #23)
> I have a few comments for you to look at, but I just wanted to say that
> patch works really well. Great job! We should really try to get this one in
> Firefox 62 :)

Thanks everyone for your work on this. Just to clarify we're hoping to get this into 61. We've told QA we're aiming to land this by 20 April. If that proves too difficult/risky we can defer this to 62.
Comment on attachment 8968401 [details]
Bug 1226272 - Part 2: Implement saving and loading the tabs order preference.

https://reviewboard.mozilla.org/r/237112/#review243250

::: devtools/client/framework/toolbox.js:269
(Diff revision 1)
>     * Combines the built-in panel definitions and the additional tool definitions that
>     * can be set by add-ons.
>     */
>    _combineAndSortPanelDefinitions() {
>      const definitions = [...this._panelDefinitions, ...this.getVisibleAdditionalTools()];
> -    definitions.sort(definition => {
> +    this._tabsOrderManager.sortPanelDefinitions(definitions);

One nit I forgot to post yesterday. It would be great if sortPanelDefinitions could always return the sorted array and if this line could read:

definitions =  this._tabsOrderManager.sortPanelDefinitions(definitions);

Otherwise you have to know that "sortPanelDefinitions" will modify the "definitions" argument to understand what is going on. It's a bit redundant since Array::sort() updates the array in-place, but I think it would help the readability of the block.
(In reply to Brian Birtles (:birtles, travelling 7~18 April) from comment #27)
> (In reply to Julian Descottes [:jdescottes][:julian] from comment #23)
> > I have a few comments for you to look at, but I just wanted to say that
> > patch works really well. Great job! We should really try to get this one in
> > Firefox 62 :)
> 
> Thanks everyone for your work on this. Just to clarify we're hoping to get
> this into 61. We've told QA we're aiming to land this by 20 April. If that
> proves too difficult/risky we can defer this to 62.

I meant 61, sorry :( Also hope we can get this landed in time for the 20th.
Comment on attachment 8968403 [details]
Bug 1226272 - Part 4: Change the style which is for while dragging.

https://reviewboard.mozilla.org/r/237116/#review243366

::: devtools/client/themes/toolbox.css
(Diff revisions 1 - 2)
>    opacity: 0;
>    transition: none;
>  }
>  
>  #toolbox-container.tabs-reordering .devtools-tab.selected {
> -  background-color: var(--theme-toolbar-hover);

My bad

background-color: var(--theme-toolbar-hover);

is needed if your mouse cursor leaves the selected tab (by going above or below)
Comment on attachment 8968402 [details]
Bug 1226272 - Part 3: Handle overflowed tabs.

https://reviewboard.mozilla.org/r/237114/#review243370

Looks good to me!

::: devtools/client/framework/toolbox-tabs-order-manager.js:135
(Diff revision 2)
>      this.tabsElement.ownerDocument.removeEventListener("mouseup", this.onMouseUp);
>  
>      if (this.isOrderUpdated) {
>        const ids =
> -        [...this.tabsElement.childNodes].map(tabElement => tabElement.dataset.id);
> +        [...this.tabsElement.childNodes]
> +          .filter(tabElement => tabElement.classList.contains("devtools-tab"))

nit: we could replace the first two lines by

  [...this.tabsElement.querySelectorAll(".devtools-tab)]
Attachment #8968402 - Flags: review?(jdescottes) → review+
Comment on attachment 8968401 [details]
Bug 1226272 - Part 2: Implement saving and loading the tabs order preference.

https://reviewboard.mozilla.org/r/237112/#review243372

I think we can go with that for now but I would like us to have a follow up to investigate splitting the toolbox-tabs-order-manager.
Right now this class is both responsible for:
- handling the drag and drop (relevant for the react component)
- handling the preference, including translating to/from tool-ids arrays (relevant for the toolbox)

I think we could try having two separate files here because both roles don't have a lot of overlap.

::: devtools/client/framework/toolbox.js:80
(Diff revision 2)
>  loader.lazyRequireGetter(this, "getKnownDeviceFront",
>    "devtools/shared/fronts/device", true);
>  loader.lazyRequireGetter(this, "NetMonitorAPI",
>    "devtools/client/netmonitor/src/api", true);
> +loader.lazyRequireGetter(this, "sortPanelDefinitions",
> +                         "devtools/client/framework/toolbox-tabs-order-manager", true);

nit: can you change the indent to be consistent with the other "lazyRequireGetter" above?
Attachment #8968401 - Flags: review?(jdescottes) → review+
Comment on attachment 8968400 [details]
Bug 1226272 - Part 1: Make devtools tab draggable and reorderable.

https://reviewboard.mozilla.org/r/237110/#review243388

Thanks for the updated patch! I should have been clearer about the issue I had with the previous version.
I added a lot of comments but that's mostly so that you don't need to wait for me to address them. I don't think we are far from a landable version.

::: devtools/client/framework/components/toolbox-tabs.js:56
(Diff revision 2)
>  
>    componentDidMount() {
>      window.addEventListener("resize", this.resizeHandler);
>      this.updateCachedToolTabsWidthMap();
>      this.updateOverflowedTabs();
> +    this._tabsOrderManager =

I don't think this will address the concern raised in the review. 

Between componentDidMount() and componentWillUnmount(), we have no way to be sure that the node we query here will not be destroyed and replaced by another one. 
It looks like React is not doing that "right now" so the current implementation works, but we should treat React as a black box and avoid keeping a reference on this node as much as we can.

I added several comments related to this, have a look at them below!

::: devtools/client/framework/components/toolbox-tabs.js:263
(Diff revision 2)
>        {
>          className: "toolbox-tabs-wrapper"
>        },
>        div(
>          {
>            className: "toolbox-tabs"

we should define onMouseDown here and call tabsOrderManager.onMouseDown():

  {
    className: "toolbox-tabs",
    onMouseDown: (e) => this._tabsOrderManager.onMouseDown(e),
  },

::: devtools/client/framework/toolbox-tabs-order-manager.js:12
(Diff revision 2)
> +/**
> + * Manage the order of devtools tabs.
> + */
> +class ToolboxTabsOrderManager {
> +  constructor(tabsElement) {
> +    this.tabsElement = tabsElement;



::: devtools/client/framework/toolbox-tabs-order-manager.js:12
(Diff revision 2)
> +/**
> + * Manage the order of devtools tabs.
> + */
> +class ToolboxTabsOrderManager {
> +  constructor(tabsElement) {
> +    this.tabsElement = tabsElement;

tabsElement might become invalid, it should not be stored here. Instead you can pass the document of the component and define two getters:

  getTabsElement() {
    return this.doc.querySelector(".toolbox-tabs");
  }

  getToolboxContainer() {
    return this.doc.querySelector("#toolbox-container");
  }
  
Everywhere you use this.tabsElement and this.toolboxContainer, you would use those getters instead. 
Of course using the getters is slower than simply accessing an existing reference, so try to access them at most once per method.
Overall this approach will be slower than your current implementation, but it won't rely on implementation internals from React which is a big plus for me. 
I doubt the performance hit should be noticeable.

::: devtools/client/framework/toolbox-tabs-order-manager.js:20
(Diff revision 2)
> +    this.onMouseDown = this.onMouseDown.bind(this);
> +    this.onMouseMove = this.onMouseMove.bind(this);
> +    this.onMouseOut = this.onMouseOut.bind(this);
> +    this.onMouseUp = this.onMouseUp.bind(this);
> +
> +    this.tabsElement.addEventListener("mousedown", this.onMouseDown);

The mousedown event should be attached by the react component

::: devtools/client/framework/toolbox-tabs-order-manager.js:23
(Diff revision 2)
> +    this.onMouseUp = this.onMouseUp.bind(this);
> +
> +    this.tabsElement.addEventListener("mousedown", this.onMouseDown);
> +  }
> +
> +  destroy() {

If the destroy is called while the user is dragging, it would be nice to perform all the cleanup currently done in onMouseUp()

::: devtools/client/framework/toolbox-tabs-order-manager.js:24
(Diff revision 2)
> +
> +    this.tabsElement.addEventListener("mousedown", this.onMouseDown);
> +  }
> +
> +  destroy() {
> +    this.tabsElement.removeEventListener("mousedown", this.onMouseDown);

Same comment, should be attached by react

::: devtools/client/framework/toolbox-tabs-order-manager.js:38
(Diff revision 2)
> +
> +    this.dragStartX = e.pageX;
> +    this.dragTarget = e.target;
> +    this.previousPageX = e.pageX;
> +
> +    this.tabsElement.ownerDocument.addEventListener("mousemove", this.onMouseMove);

If you pass the document to ToolboxTabsOrderManager (and store it as this.doc for instance) you could replace all `this.tabsElement.ownerDocument` with `this.doc`

::: devtools/client/framework/toolbox-tabs-order-manager.js:47
(Diff revision 2)
> +    this.toolboxContainer.classList.add("tabs-reordering");
> +  }
> +
> +  onMouseMove(e) {
> +    const diffPageX = e.pageX - this.previousPageX;
> +    const drapTargetCenterX =

nit: should this be dragTargetCenterX instead of drapTargetCenterX (p -> g)

::: devtools/client/framework/toolbox-tabs-order-manager.js:50
(Diff revision 2)
> +  onMouseMove(e) {
> +    const diffPageX = e.pageX - this.previousPageX;
> +    const drapTargetCenterX =
> +      this.dragTarget.offsetLeft + diffPageX + this.dragTarget.clientWidth / 2;
> +
> +    for (const tabElement of this.tabsElement.childNodes) {

childNodes would give us text nodes if any. What about getting 
  let tabs = this.tabsElement.querySelectorAll(".devtools-tab");
  
(no need to check for the chevron classname in one of the next commits too!)

::: devtools/client/framework/toolbox-tabs-order-manager.js:55
(Diff revision 2)
> +      const anotherElementCenterX =
> +        tabElement.offsetLeft + tabElement.clientWidth / 2;
> +
> +      if (Math.abs(drapTargetCenterX - anotherElementCenterX) <
> +          tabElement.clientWidth / 2) {

This is much clearer, thanks!

If the dragged tab is smaller than the hovered tab, and you drag slowly, we will see the hovered tab flickering after and before repeatedly. 
That's because after doing the insertBefore, this condition will still be true.
There might be more accurate and clever ways to fix this, but using tabElement.clientWidth / 3 (instead of / 2) makes the flickering disappear for me.

::: devtools/client/framework/toolbox-tabs-order-manager.js:62
(Diff revision 2)
> +
> +      if (Math.abs(drapTargetCenterX - anotherElementCenterX) <
> +          tabElement.clientWidth / 2) {
> +        const xBefore = this.dragTarget.offsetLeft;
> +
> +        if (xBefore < tabElement.offsetLeft) {

nit: we already have the x-center for both elements, we could compare them instead?

::: devtools/client/framework/toolbox-tabs-order-manager.js:78
(Diff revision 2)
> +    if ((!this.dragTarget.previousSibling && distance < 0) ||
> +        (!this.dragTarget.nextSibling && distance > 0)) {
> +      // If the drag target is already edge of the tabs and the mouse will make the
> +      // element to move to same direction more, keep the position.
> +      distance = 0;
> +    }

This logic should be reversed when we are in RTL. Sadly we don't have a great way to check it on the document that owns the tabs.
Other devtools documents have the "dir" attribute defined, but for this one we would have to use getComputedStyle. 
Since this is very costly we should only do it once. See https://searchfox.org/mozilla-central/rev/f65d7528e34ef1a7665b4a1a7b7cdb1388fcd3aa/devtools/client/framework/toolbox.js#199-205 for an example.

I strongly suggest handling this in a followup.

::: devtools/client/framework/toolbox-tabs-order-manager.js:89
(Diff revision 2)
> +  onMouseOut(e) {
> +    if (e.pageX <= 0 || this.tabsElement.ownerDocument.width <= e.pageX ||
> +        e.pageY <= 0 || this.tabsElement.ownerDocument.height <= e.pageY) {
> +      this.onMouseUp();
> +    }
> +  }

If I remove this callback, the tabs follow the cursor even when it leaves the window. 

Was this added to fix a specific issue. I feel like the behavior is more natural without it so I'm curious :)
Attachment #8968400 - Flags: review?(jdescottes)
Comment on attachment 8968404 [details]
Bug 1226272 - Part 5: Add test for reordering the tabs in toolbox.

https://reviewboard.mozilla.org/r/237118/#review243424

Thanks for adding a test Daisuke! I have a few comments we should address before landing. All the comments which are not flagged as issues can be moved to a followup bug.
R+ as long as have a green try for this.

::: devtools/client/framework/test/browser_toolbox_toolbar_reorder_by_dnd.js:9
(Diff revision 2)
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// Test for following reordering operation:
> +// * DnD the target component to back

Comment valid for the whole file:

Could we spell out DragAndDrop rather than using DnD, that's more consistent with what we usually do in devtools

::: devtools/client/framework/test/browser_toolbox_toolbar_reorder_by_dnd.js:45
(Diff revision 2)
> +    expectedOrder: ["inspector", "jsdebugger", "webconsole", "styleeditor",
> +                    "performance", "memory", "netmonitor", "storage"],
> +  },
> +  {
> +    description: "DnD the target component over the starting of the tab",
> +    dragTarget: "toolbox-tab-performance",

The leak issues on try seem linked to the performance panel, let's simply avoid it. Here we can for instance use the netmonitor, starting it doesn't create any leak.

::: devtools/client/framework/test/browser_toolbox_toolbar_reorder_by_dnd.js:54
(Diff revision 2)
> +    expectedOrder: ["performance", "inspector", "webconsole", "jsdebugger",
> +                    "styleeditor", "memory", "netmonitor", "storage"],
> +  },
> +  {
> +    description: "DnD the target component over the ending of the tab",
> +    dragTarget: "toolbox-tab-performance",

similar comment, let's use the webconsole here.

::: devtools/client/framework/test/browser_toolbox_toolbar_reorder_by_dnd.js:72
(Diff revision 2)
> +                    "performance", "memory", "netmonitor", "storage"],
> +  },
> +];
> +
> +add_task(async function() {
> +  const originalPreference = Services.prefs.getCharPref("devtools.toolbox.tabsOrder");

Before starting the test could we check that:
- there is no chevron button displayed (and if there is a chevron menu: log a failure saying the size of the screen being too small)
- the tabs correspond to the expected set (and if they do not: log a failure explaining that the set of default tools does not match the expectation from the test)

For the moment, I checked the devtools-client preferences (https://searchfox.org/mozilla-central/search?q=devtools.%5Ba-z%5D%2B.enabled&case=true&regexp=true&path=devtools-client.js) and they are normally the same on all channels. But we could run the risk to have a tool enabled by default in Nightly but not in Beta/Release and making the test fail surprisingly. We should have a followup to force the tools to match your list.

::: devtools/client/framework/test/browser_toolbox_toolbar_reorder_by_dnd.js:76
(Diff revision 2)
> +add_task(async function() {
> +  const originalPreference = Services.prefs.getCharPref("devtools.toolbox.tabsOrder");
> +  const tab = await addTab("about:blank");
> +  const toolbox = await openToolboxForTab(tab, "webconsole", Toolbox.HostType.BOTTOM);
> +
> +  const testStartingOrder =

nit: this could move outside of the function and be capitalized.

::: devtools/client/framework/test/browser_toolbox_toolbar_reorder_by_dnd.js:82
(Diff revision 2)
> +    ["inspector", "webconsole", "jsdebugger", "styleeditor",
> +     "performance", "memory", "netmonitor", "storage"];
> +
> +  for (const testData of TEST_DATA) {
> +    info(`Test for '${ testData.description }'`);
> +    Services.prefs.setCharPref("devtools.toolbox.tabsOrder", testStartingOrder.join(","));

Here we assume that the tabs are re-rendered synchronously after setting the preference. That feels fragile. If try is green with this even in debug, we can proceed like that but can we log a follow up to improve the test?

Here we would need to wait until the order of the tabs in the DOM matches our expectations.

::: devtools/client/framework/test/browser_toolbox_toolbar_reorder_by_dnd.js:100
(Diff revision 2)
> +
> +  await resizeWindow(toolbox, originalWidth, originalHeight);
> +  Services.prefs.setCharPref("devtools.toolbox.tabsOrder", originalPreference);
> +});
> +
> +function assert(toolbox, dragTarget, expectedOrder) {

could we split this assert in 2:
- one method to check the order
- one method to check the selected tab

Each method should have a clear name (eg. assertTabOrder and assertSelectedTab)

::: devtools/client/framework/test/browser_toolbox_toolbar_reorder_by_dnd.js:111
(Diff revision 2)
> +      .filter(tabElement => tabElement.classList.contains("devtools-tab"))
> +      .map(tabElement => tabElement.dataset.id);
> +    is(currentIds.join(","), expectedOrder.join(","), "The order should be correct");
> +  }
> +
> +  info("Check the order in DevTools preference for tas order");

nit: tas order -> tab order

::: devtools/client/framework/test/browser_toolbox_toolbar_reorder_by_dnd.js:127
(Diff revision 2)
> +function dnd(toolbox, dragTarget, dropTarget, passedTargets = []) {
> +  const dragTargetEl = toolbox.doc.getElementById(dragTarget);
> +  const dragTargetBounds = dragTargetEl.getBoundingClientRect();
> +
> +  info(`Drag ${ dragTarget } to ${ dropTarget }`);
> +  EventUtils.synthesizeMouse(dragTargetEl,

You can use the shortcut:

EventUtils.synthesizeMouseAtCenter(dragTargetEl, { type: "mousedown" }, toolbox.win);

and then drop the dragTargetBounds variable.

(same comment for the other synthesizeMouseAt, except mouseout)

::: devtools/client/framework/test/browser_toolbox_toolbar_reorder_by_dnd.js:136
(Diff revision 2)
> +                             dragTargetEl.ownerGlobal);
> +
> +  for (const passedTarget of passedTargets) {
> +    info(`Via ${ passedTarget }`);
> +    const passedTargetEl = toolbox.doc.getElementById(passedTarget);
> +    const pasedTargetBounds = passedTargetEl.getBoundingClientRect();

nit: pased -> passed

::: devtools/client/framework/test/browser_toolbox_toolbar_reorder_by_dnd.js:171
(Diff revision 2)
> +  const originalWidth = hostWindow.outerWidth;
> +  const originalHeight = hostWindow.outerHeight;
> +  const toWidth = width || originalWidth;
> +  const toHeight = height || originalHeight;
> +
> +  info("Resize devtools window to a width that should trigger an overflow");

I guess this was copied over from another test. This info should have been removed before landing. Can you remove it here?
Attachment #8968404 - Flags: review?(jdescottes) → review+
Comment on attachment 8968400 [details]
Bug 1226272 - Part 1: Make devtools tab draggable and reorderable.

https://reviewboard.mozilla.org/r/237110/#review243388

> This logic should be reversed when we are in RTL. Sadly we don't have a great way to check it on the document that owns the tabs.
> Other devtools documents have the "dir" attribute defined, but for this one we would have to use getComputedStyle. 
> Since this is very costly we should only do it once. See https://searchfox.org/mozilla-central/rev/f65d7528e34ef1a7665b4a1a7b7cdb1388fcd3aa/devtools/client/framework/toolbox.js#199-205 for an example.
> 
> I strongly suggest handling this in a followup.

Thanks! Okay, I'll fix in this patch.

> If I remove this callback, the tabs follow the cursor even when it leaves the window. 
> 
> Was this added to fix a specific issue. I feel like the behavior is more natural without it so I'm curious :)

We can not detect mouse event if the mouse was moved to the browsing document.

So, if we do the following:
1. mouse down on a tab. 
2. move the mouse to the browsing document. 
3. mouse release.
4. move to devtools again.
Dragging is kept even without mouse button.

To resolve this, we need the mouseout event so as to force to finish the dragging.
Or, do you know a way which detects the mouse event on the browsing document from devtool's context??
If we can do, yes, we can remove this event!
(In reply to Daisuke Akatsuka (:daisuke) from comment #40)
> Comment on attachment 8968400 [details]
> Bug 1226272 - Part 1: Make devtools tab draggable and reorderable.
> 
> https://reviewboard.mozilla.org/r/237110/#review243388
> 
> > This logic should be reversed when we are in RTL. Sadly we don't have a great way to check it on the document that owns the tabs.
> > Other devtools documents have the "dir" attribute defined, but for this one we would have to use getComputedStyle. 
> > Since this is very costly we should only do it once. See https://searchfox.org/mozilla-central/rev/f65d7528e34ef1a7665b4a1a7b7cdb1388fcd3aa/devtools/client/framework/toolbox.js#199-205 for an example.
> > 
> > I strongly suggest handling this in a followup.
> 
> Thanks! Okay, I'll fix in this patch.
> 
> > If I remove this callback, the tabs follow the cursor even when it leaves the window. 
> > 
> > Was this added to fix a specific issue. I feel like the behavior is more natural without it so I'm curious :)
> 
> We can not detect mouse event if the mouse was moved to the browsing
> document.
> 
> So, if we do the following:
> 1. mouse down on a tab. 
> 2. move the mouse to the browsing document. 
> 3. mouse release.
> 4. move to devtools again.
> Dragging is kept even without mouse button.
> 
> To resolve this, we need the mouseout event so as to force to finish the
> dragging.
> Or, do you know a way which detects the mouse event on the browsing document
> from devtool's context??
> If we can do, yes, we can remove this event!

That's what I suspected, but I couldn't reproduce it. 

As discussed on Slack, attaching mousemove/mouseup events to the window rather than on the document usually fixes this kind of issues. In this case, we are both on OSX, so I don't know why we see different behaviors. Since you tested with the window instead of the document and it doesn't fix the issue on your machine, let's keep the mouseout event for now and investigate this in a follow up.
The mouseout issue only occurs if the current page is an "about:" page such as about:config. We should be able to fix it by using ownerGlobal.top but we should take care of that in a separate bug. I logged Bug 1455230 for the general case since this impacts several other DevTools components, but we probably need a dedicated follow-up bug for the draggable tabs.
Comment on attachment 8968400 [details]
Bug 1226272 - Part 1: Make devtools tab draggable and reorderable.

https://reviewboard.mozilla.org/r/237110/#review243642

Thanks for the update, I think we can go forward with this as long as all tests are passing. A few comments but nothing big.

As a more general comment, I want to echo something I mentioned in comment 37: splitting toolbox-tabs-order-manager.js between the UI part and the preferences/logic part.
I think extracting the UI part could allow us to reintegrate some code directly in toolbox-tabs.js and this way make it really compatible with the React lifecyle of the component.
The current approach is fragile because we are mixing React with regular DOM manipulation:
- keeping references, even if the last version only keeps them between onMouseDown and onMouseUp, they still might become invalid
- doing DOM manipulations (here calling insertBefore) on DOM elements rendered by React is generally a bad practice
I think we can live with it for now, but we should quickly find a way of doing that in a more "react-friendly" way.
Maybe leveraging some existing libraries such as https://github.com/clauderic/react-sortable-hoc

::: devtools/client/framework/components/toolbox-tabs.js:260
(Diff revision 4)
>          className: "toolbox-tabs-wrapper"
>        },
>        div(
>          {
> -          className: "toolbox-tabs"
> +          className: "toolbox-tabs",
> +          onMouseDown: this._tabsOrderManager.onMouseDown,

I would prefer to call 

(e) => this._tabsOrderManager.onMouseDown(e)

even if we bind it in toolbox-tabs-order-manager.js, the variant above doesn't rely on it.

::: devtools/client/framework/toolbox-tabs-order-manager.js:36
(Diff revision 4)
> +    if (!e.target.classList.contains("devtools-tab")) {
> +      return;
> +    }
> +
> +    this.dragStartX = e.pageX;
> +    this.dragTarget = e.target;

If we are depending on this.dragTarget to get the elements we need, you can skip the getters and directly get all references in onMouseDown:

  this.dragTarget = e.target;
  this.toolboxTabsElement = this.dragTarget.closest(".toolbox-tabs");
  this.toolboxContainerElement = this.dragTarget.closest("#toolbox-container");
  
and nullify them in onMouseUp.
Attachment #8968400 - Flags: review?(jdescottes) → review+
Comment on attachment 8968400 [details]
Bug 1226272 - Part 1: Make devtools tab draggable and reorderable.

Yulia, it would be great to have your opinion here. This patch adds drag-and-drop on our toolbox-tabs React component. If you look at the patch, you will see it does that by doing DOM manipulations on elements normally managed by React (which is bad). Somehow in my first reviews I missed that, and despite this underlying issue, we have a patch that works well with the current version of React we use in DevTools.

As I said in my previous comment, we really should rework this to use something which is more React-friendly, but I am not sure what to do with current patch series. Since it works nicely, has a test - and is a very nice feature -, I am tempted to let this version land in time for FF61 (soft freeze starting tomorrow) and then re-work it later. But I don't feel comfortable enough with React to assess if this is a bad decision or not. 

What is your take on this?
Attachment #8968400 - Flags: feedback?(ystartsev)
(Yulia: switching the f? to a ni?, less likely to be erased by mozreview :) )
Flags: needinfo?(ystartsev)
Hey!

TL;DR -- this looks ok to me, given the state of drag and drop in react applications

Reordering is something that react famously has problems with, because it combines two requirements -- animation and dom order. There have been a few attempts to solve this, and Dan Abramov has written a lot about it. https://medium.com/@dan_abramov/the-future-of-drag-and-drop-apis-249dfea7a15f

Some context: There are two libraries that (when last I checked) were the go to libraries for dragging and dropping. Dragula, a lightweight option, and react dnd.  

Dragula takes the same approach as we are taking here - it uses dom manipulations to move the elements around, outside of the react lifecycle. You can see that here: https://github.com/bevacqua/dragula/blob/master/dragula.js#L253. The key difference is that it emits an event, and allows you to update your react data structure as you see fit. 

React dnd does something much more complex. it creates a new element which is a screenshot of the element being dragged, so it can be dragged freely. This is stored in a separate back-end and cleaned up as necessary. Once it is dropped, the new order is inserted via redux. https://github.com/react-dnd/react-dnd/blob/master/packages/dnd-core/src/reducers/dragOperation.js#L52

Both of these libraries are quite complex, and I am not sure we are gaining much from implementing them. In fact I prefer what we currently have, since the problem being addressed here has a more controlled scope than what those libraries try to solve. I also think this does a good job of what it is meant to do.

There is one drawback of this current solution. It does not update the react data. If we rerender the toolbar and it gets its initial order from react, this reordering will disappear. If this is a significant concern, I would suggest keeping the code as it is, and having a reducer that updates the order of tabs on drop. There is a drawback to this as it will cause the tab to re-render. I do not think this is a major concern and probably this can be left as it is.
Flags: needinfo?(ystartsev)
Oh, just realized what the event listeners are doing! ok, so we are doing something very similar to dragula in all cases. This looks good to me
Thanks a lot for having a look Yulia! 

I agree that react-dnd or similar libraries seem too heavy for our simple problem, and if the alternative is doing something similar to Daisuke's current approach we'll be better using our custom implementation for now.

Daisuke: With this concern out of the way, I see that the last try push seems green except for 2 failures on Windows ccov. If those are unrelated to your change (I think it's the case?), I think you should be good to land this now.

Let's synchronize tomorrow to log the followup bugs we mentioned during the review.
Thank you very much for your kindly feedback, Yulia!
I can make these patches to land with confidence :)

Julian, yeah, I don't think that failures were caused by this change.
And yes, let's file the followup bugs! 

I'll make them to land soon.
Thanks!
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dd92dd2d5ae7
Part 1: Make devtools tab draggable and reorderable. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/59a62840bad1
Part 2: Implement saving and loading the tabs order preference. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/a30819ceb734
Part 3: Handle overflowed tabs. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/918f4ce27e41
Part 4: Change the style which is for while dragging. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/f9f1ea8029d2
Part 5: Add test for reordering the tabs in toolbox. r=jdescottes
Depends on: 1455573
I have reproduced this issue using Firefox  61.0a1 (2018.04.12) on Ubuntu 14.04 x64.
I can confirm this issue is fixed, I verified using Firefox 61.0b9 on Win 10 x64, Mac OS X 10.13 and Ubuntu 14.04 x64.
Status: RESOLVED → VERIFIED
Depends on: 1460584
Depends on: 1467000
Product: Firefox → DevTools
I have documented this by adding a note to the toolbox article:
https://developer.mozilla.org/en-US/docs/Tools/Tools_Toolbox

And adding a note to the Fx61 rel notes:
https://developer.mozilla.org/en-US/Firefox/Releases/61#Developer_tools

Let me know if this looks OK. Thanks!
Flags: needinfo?(dakatsuka)
Looks good!
Thank you for the note, Chiris!
Flags: needinfo?(dakatsuka)
Depends on: 1508234
You need to log in before you can comment on or make changes to this bug.