Closed Bug 1281789 Opened 4 years ago Closed 4 years ago

Create an HTML replacement for the sidebar all tabs menu

Categories

(DevTools :: Framework, defect, P1)

defect

Tracking

(firefox51 verified)

VERIFIED FIXED
Firefox 51
Iteration:
51.2 - Aug 29
Tracking Status
firefox51 --- verified

People

(Reporter: Honza, Assigned: Honza)

References

Details

(Whiteboard: [devtools-html])

Attachments

(2 files, 3 obsolete files)

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
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Iteration: --- → 50.2
Flags: qe-verify+
Priority: -- → P1
QA Contact: alexandra.lucinet
Whiteboard: [devtools-html]
Attached patch bug1281789.patch (obsolete) — Splinter Review
First patch
Iteration: 50.2 - Jul 4 → 50.3 - Jul 18
Iteration: 50.3 - Jul 18 → 50.4 - Aug 1
Iteration: 50.4 - Aug 1 → 51.1 - Aug 15
QA Contact: adalucin → cristian.comorasu
Iteration: 51.1 - Aug 15 → 51.2 - Aug 29
Attached patch bug1281789.patch (obsolete) — Splinter Review
Patrick, you've been already doing review related to the tab bar. Here is related patch, support for the all-tabs-menu.

Honza
Attachment #8764873 - Attachment is obsolete: true
Attachment #8784023 - Flags: review?(pbrosset)
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
Attachment #8784023 - Flags: review?(pbrosset)
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?
Attached patch bug1281789.patch (obsolete) — Splinter Review
(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
Attachment #8784023 - Attachment is obsolete: true
Attachment #8785220 - Flags: review?(pbrosset)
> ::: 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
Flags: needinfo?(bgrinstead)
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.
Attachment #8785220 - Flags: review?(pbrosset) → review+
(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
Attachment #8785283 - Flags: review?(fredrik.broman)
Attachment #8785283 - Flags: review?(fredrik.broman) → review?(pbrosset)
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
See Comment 10 - you shouldn't create the <popupset> or append the menu into the inspector frame but rather the toolbox
Flags: needinfo?(bgrinstead)
Attachment #8785283 - Flags: review?(pbrosset) → review+
Attached patch bug1281789.patchSplinter Review
(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
Attachment #8785220 - Attachment is obsolete: true
Attachment #8785928 - Flags: review+
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a358a87c0938
https://hg.mozilla.org/mozilla-central/rev/aac1b365c55c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Depends on: 1300291
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.