Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Description • 8 years ago

This is a follow up for bug 1259819

The sidebar implements a menu with list of all tabs, which should be converted to HTML.

This bug is also blocking bug 1278984 - Add the font panel as an optional sidebar tab.

The menu should be implemented on top of API introduced in bug 1257613

Honza

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 1 • 8 years ago

First patch

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 2 • 8 years ago

Patrick, you've been already doing review related to the tab bar. Here is related patch, support for the all-tabs-menu.

Honza

	Patrick Brosset <:pbro>
	Comment 3 • 8 years ago

Comment on attachment 8784023 [details] [diff] [review]
bug1281789.patch

Review of attachment 8784023 [details] [diff] [review]:
-----------------------------------------------------------------

Great work. Simple and easy to understand.
I just have a question about <popupset> which I'd like answered before I R+.
Maybe Brian needs to weight in on this?

::: devtools/client/inspector/inspector.xul
@@ +31,5 @@
>            src="chrome://devtools/content/shared/theme-switching.js"/>
> +
> +  <!-- Popupset needed by `Menu` component. This should be removed
> +    as soon as bug 1297412 is fixed. -->
> +  <popupset />

Doesn't this need an ID so we can find it from JS? Is this done automatically in the Menu component and you have no control over it? It seems weird that the Menu component doesn't do that on its own.
Also, I think Brian's plan was to move all the remaining necessary XUL things like popupsets into toolbox.xul, so that we can have a completely XUL-free inspector frame. This prevents it. Can we move it to the toolbox frame instead?

::: devtools/client/shared/components/tabs/tabbar.js
@@ +26,5 @@
>      onSelect: PropTypes.func,
> +    showAllTabsMenu: PropTypes.bool,
> +  },
> +
> +

nit: there should only be 1 newline between blocks.

@@ +158,5 @@
> +    // https://bugzilla.mozilla.org/show_bug.cgi?id=1274551
> +    let rect = target.getBoundingClientRect();
> +    let screenX = target.ownerDocument.defaultView.mozInnerScreenX;
> +    let screenY = target.ownerDocument.defaultView.mozInnerScreenY;
> +    menu.popup(rect.left + screenX, rect.bottom + screenY, {

Probably a minor issue, but rect.left might need to be rect.right when in RTL.

::: devtools/client/shared/components/tabs/tabs.css
@@ +41,5 @@
>  
> +.tabs .all-tabs-menu  {
> +  position: absolute;
> +  top: 0;
> +  right: 0;

For RTL, the menu should be all the way to the left instead.
So 2 solutions:

top: 0;
offset-inline-end: 0;

But that's only if offset-inline-end has shipped, which I'm not sure it did. It might be only available in nightly and not riding the trains.

Other solution:

.tabs .all-tabs-menu {
  top: 0;
  right: 0;
  ...
}

.tabs .all-tabs-menu:-moz-dir(rtl) {
  right: unset;
  left: 0;
}

::: devtools/client/shared/components/tabs/tabs.js
@@ +120,5 @@
> +        node.removeEventListener("underflow", this.onUnderflow, false);
> +      }
> +    },
> +
> +    // Overflow

Why this comment? Looks like a bit further down you have a // DOM Events comment below which are all the event handlers. Maybe move these 2 functions down there instead and remove the // Overflow comment ?

@@ +122,5 @@
> +    },
> +
> +    // Overflow
> +
> +    onOverflow: function(event) {

Missing space between function and (
Same for the other functions you added. You should make sure to run eslint locally.
It runs relatively quickly on devtools, so `mach eslint devtools` is quick and useful. The plan was to have it integrated into mozreview too, so it would perform an automated review for you, but that hasn't happened yet. Also, very soon we should be able to do `mach eslint -r .` to lint only the files changed in a given hg revision, making it really fast.

@@ +263,5 @@
>              )
>            );
>          });
>  
> +      // Display the menu only if there is no enough horizontal

s/no/not

	Patrick Brosset <:pbro>
	Comment 4 • 8 years ago

Oh, and I forgot to mention, we do have some tests today for the "old" sidebar widget, and in particular we have one for the overflow/underflow mechanism:
devtools\client\framework\test\browser_toolbox_sidebar_overflow_menu.js
Can you add a test for the "new" sidebar widget?

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 5 • 8 years ago

(In reply to Patrick Brosset <:pbro> from comment #3)
> Comment on attachment 8784023 [details] [diff] [review]
> bug1281789.patch
> 
> Review of attachment 8784023 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great work. Simple and easy to understand.
> I just have a question about <popupset> which I'd like answered before I R+.
> Maybe Brian needs to weight in on this?
> 
> ::: devtools/client/inspector/inspector.xul
> @@ +31,5 @@
> >            src="chrome://devtools/content/shared/theme-switching.js"/>
> > +
> > +  <!-- Popupset needed by `Menu` component. This should be removed
> > +    as soon as bug 1297412 is fixed. -->
> > +  <popupset />
> 
> Doesn't this need an ID so we can find it from JS? Is this done
> automatically in the Menu component and you have no control over it? It
> seems weird that the Menu component doesn't do that on its own.
Here is the related code:
https://dxr.mozilla.org/mozilla-central/rev/7963ebdd52b93f96b812eff2eab8d94097147b9c/devtools/client/framework/menu.js#67

The menu component creates <menupopup> inside the set and reuses it for all menus (also making sure that just one menu is opened at a time).

> Also, I think Brian's plan was to move all the remaining necessary XUL
> things like popupsets into toolbox.xul, so that we can have a completely
> XUL-free inspector frame. This prevents it. Can we move it to the toolbox
> frame instead?
This is more question for Brian. My feeling is that we need a simple solution for bug 1297412.

> 
> ::: devtools/client/shared/components/tabs/tabbar.js
> @@ +26,5 @@
> >      onSelect: PropTypes.func,
> > +    showAllTabsMenu: PropTypes.bool,
> > +  },
> > +
> > +
> 
> nit: there should only be 1 newline between blocks.
Fixed

> 
> @@ +158,5 @@
> > +    // https://bugzilla.mozilla.org/show_bug.cgi?id=1274551
> > +    let rect = target.getBoundingClientRect();
> > +    let screenX = target.ownerDocument.defaultView.mozInnerScreenX;
> > +    let screenY = target.ownerDocument.defaultView.mozInnerScreenY;
> > +    menu.popup(rect.left + screenX, rect.bottom + screenY, {
> 
> Probably a minor issue, but rect.left might need to be rect.right when in
> RTL.
Note that there is no much difference between using `rect.left` or `rect.right`. This only aligns the left corner of the popup to left or right corner of the button (the button's width is 15px).

Better solution (not sure if the right solution) could be to change popup alignment relative to the target (the button). E.g. `topleft` to align top left corner of the popup to the bottom left corner of the target (button) or `topright` align top right corner of the popup to the bottom right corner of the target.

Problem is that this is a context menu (not real popup) opened using `openPopupAtScreen` and this API doesn't support custom alignment.

> 
> ::: devtools/client/shared/components/tabs/tabs.css
> @@ +41,5 @@
> >  
> > +.tabs .all-tabs-menu  {
> > +  position: absolute;
> > +  top: 0;
> > +  right: 0;
> 
> For RTL, the menu should be all the way to the left instead.
> So 2 solutions:
> 
> top: 0;
> offset-inline-end: 0;
> 
> But that's only if offset-inline-end has shipped, which I'm not sure it did.
> It might be only available in nightly and not riding the trains.
> 
> Other solution:
> 
> .tabs .all-tabs-menu {
>   top: 0;
>   right: 0;
>   ...
> }
> 
> .tabs .all-tabs-menu:-moz-dir(rtl) {
>   right: unset;
>   left: 0;
> }
Done

> 
> ::: devtools/client/shared/components/tabs/tabs.js
> @@ +120,5 @@
> > +        node.removeEventListener("underflow", this.onUnderflow, false);
> > +      }
> > +    },
> > +
> > +    // Overflow
> 
> Why this comment? Looks like a bit further down you have a // DOM Events
> comment below which are all the event handlers. Maybe move these 2 functions
> down there instead and remove the // Overflow comment ?
Done

> 
> @@ +122,5 @@
> > +    },
> > +
> > +    // Overflow
> > +
> > +    onOverflow: function(event) {
> 
> Missing space between function and (
> Same for the other functions you added. You should make sure to run eslint
> locally.
Yeah, I got it not, sorry.

> It runs relatively quickly on devtools, so `mach eslint devtools` is quick
> and useful. The plan was to have it integrated into mozreview too, so it
> would perform an automated review for you, but that hasn't happened yet.
> Also, very soon we should be able to do `mach eslint -r .` to lint only the
> files changed in a given hg revision, making it really fast.
> 
> @@ +263,5 @@
> >              )
> >            );
> >          });
> >  
> > +      // Display the menu only if there is no enough horizontal
> 
> s/no/not

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 6 • 8 years ago

> ::: devtools/client/inspector/inspector.xul
> @@ +31,5 @@
> >            src="chrome://devtools/content/shared/theme-switching.js"/>
> > +
> > +  <!-- Popupset needed by `Menu` component. This should be removed
> > +    as soon as bug 1297412 is fixed. -->
> > +  <popupset />
> 
> Doesn't this need an ID so we can find it from JS? Is this done
> automatically in the Menu component and you have no control over it? It
> seems weird that the Menu component doesn't do that on its own.
Here is the related code:
https://dxr.mozilla.org/mozilla-central/rev/7963ebdd52b93f96b812eff2eab8d94097147b9c/devtools/client/framework/menu.js#67

The menu component creates <menupopup> inside the set and reuses it for all menus (also making sure that just one menu is opened at a time).

> Also, I think Brian's plan was to move all the remaining necessary XUL
> things like popupsets into toolbox.xul, so that we can have a completely
> XUL-free inspector frame. This prevents it. Can we move it to the toolbox
> frame instead?
This is more question for Brian. My feeling is that we need a simple solution for bug 1297412.


Honza

	Patrick Brosset <:pbro>
	Comment 7 • 8 years ago

Comment on attachment 8785220 [details] [diff] [review]
bug1281789.patch

Review of attachment 8785220 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, then R+ from me, pending a discussion with Brian.

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 8 • 8 years ago

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd776f81e577

Honza

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 9 • 8 years ago

(In reply to Patrick Brosset <:pbro> from comment #4)
> Oh, and I forgot to mention, we do have some tests today for the "old"
> sidebar widget, and in particular we have one for the overflow/underflow
> mechanism:
> devtools\client\framework\test\browser_toolbox_sidebar_overflow_menu.js
> Can you add a test for the "new" sidebar widget?

Patch attached.

Honza

	Brian Grinstead [:bgrins]
	Comment 10 • 8 years ago

Comment on attachment 8785220 [details] [diff] [review]
bug1281789.patch

Review of attachment 8785220 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/shared/components/tabs/tabbar.js
@@ +158,5 @@
> +    let rect = target.getBoundingClientRect();
> +    let screenX = target.ownerDocument.defaultView.mozInnerScreenX;
> +    let screenY = target.ownerDocument.defaultView.mozInnerScreenY;
> +    menu.popup(rect.left + screenX, rect.bottom + screenY, {
> +      doc: target.ownerDocument

This is meant to take a toolbox object instead of an options arg like this.  See https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/shared/style-inspector-menu.js#254.  This way, it will append the menu into the toolbox's document and you don't need to create the <popupset> in inspector.xul

	Brian Grinstead [:bgrins]
	Comment 11 • 8 years ago

See Comment 10 - you shouldn't create the <popupset> or append the menu into the inspector frame but rather the toolbox

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 12 • 8 years ago

(In reply to Brian Grinstead [:bgrins] from comment #11)
> See Comment 10 - you shouldn't create the <popupset> or append the menu into
> the inspector frame but rather the toolbox
Done, thanks!

Try 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fe0ba5a4c45

Honza

	Pulsebot
	Comment 13 • 8 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/a358a87c0938
Implement AllTabsMenu component. r=pbro
https://hg.mozilla.org/integration/fx-team/rev/aac1b365c55c
Test for all tabs menu. r=pbro

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 14 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/a358a87c0938
https://hg.mozilla.org/mozilla-central/rev/aac1b365c55c

	Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact mboldan@mozilla.com
	Comment 15 • 8 years ago

I didn't find any new issues while verifying this bug. I used the latest Nightly 51.0a1 on Windows 10 64-bit, Ubuntu 14.04 64-bit and Mac OS X 10.11.