Closed Bug 1335608 Opened 7 years ago Closed 7 years ago

Add menu button to access tools when toolbar overflows

Categories

(DevTools :: Framework, enhancement, P3)

Product:
DevTools
Component:
Framework
Version:
53 Branch
Type:
enhancement

Tracking

(firefox54 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 54
Tracking Flags:
Tracking Status
firefox54 --- verified

People

(Reporter: jdescottes, Assigned: jdescottes)

References

Details

(Keywords: dev-doc-needed)

Attachments

(3 files)

hidden-tools-side-dock.png (this is extreme, but only two tools are visible here)
193.91 KB, image/png
Details
Bug 1335608 - add a button to select hidden tools when toolbox toolbar overflows;
59 bytes, text/x-review-board-request
gregtatum
: review+
Details
all-tools-menu.gif
1.77 MB, image/gif
Details 
When docked to the side or on small screens, the devtools icons can start overflowing an become unaccessible.

We have several bugs for improving the toolbar (e.g. Bug 1239859) but in the meantime a quick and easy solution could be to display a menu listing all enabled tools when we detect an overflow in the toolbox toolbar.
Attached image all-tools-menu.gif — 
Small demo of the attached patch. The menu button simply lists all available tools (not only the ones that are hidden).
Cleaned up the patch and wrote a test.

Let's see what try has to say: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6dbd22ce5c0fd6c73e42704925c737bf32eb2cdb
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Comment on attachment 8832287 [details]
Bug 1335608 - add a button to select hidden tools when toolbox toolbar overflows;

https://reviewboard.mozilla.org/r/108622/#review110736

::: devtools/client/framework/components/toolbox-toolbar.js:79
(Diff revision 3)
> +
> +  removeFlowEvents() {
> +    let node = findDOMNode(this);
> +    let tabs = node.querySelector(".toolbox-tabs-wrapper");
> +    if (tabs) {
> +      tabs.removeEventListener("overflow", this.onOverflow);

How did I not know about this event? Nice.

::: devtools/client/framework/components/toolbox-toolbar.js:107
(Diff revision 3)
>      return this.props.canRender
>        ? (
>          div(
>            containerProps,
>            renderToolboxButtonsStart(this.props),
> -          renderTabs(this.props),
> +          renderTabs(this.props, this.state.overflow),

It seems like it would make sense to turn renderTabs into it's own stateful component, as the state only matters for that particular component, not for the overall toolbar. Right now the overall component is only concerned with organizing all of the sub-rendering functions, so it is easy to read and understand. I think this would keep the top level component a lot more simple, and keep the added compelxity of maintaining overflow state in the child component. What do you think?

::: devtools/client/framework/components/toolbox-toolbar.js:190
(Diff revision 3)
> +      });
> +
> +      menu.popup(e.screenX, e.screenY, toolbox);
> +      return menu;
> +    }
> +  }, "...");

Should this be the the ellpsis character "…" instead of three dots? Is there anything we should think about for L10N, or is this just considered a glyph?

::: devtools/client/framework/test/browser_toolbox_toolbar_overflow.js:83
(Diff revision 3)
> +  EventUtils.synthesizeMouseAtCenter(allToolsButton, {}, toolbox.win);
> +
> +  let menuPopup = toolbox.doc.querySelector("#all-tools-menu");
> +  ok(menuPopup, "all-tools-menu is available");
> +
> +  info("Waitting for the menu popup to be displayed");

nit: s/Waitting/Waiting/
Nice, thanks for doing this! I wanted to get your feedback on a few thoughts I had before I r+
Comment on attachment 8832287 [details]
Bug 1335608 - add a button to select hidden tools when toolbox toolbar overflows;

https://reviewboard.mozilla.org/r/108622/#review110736

I missed your review! Sorry about that and thanks for the feedback Greg!

> How did I not know about this event? Nice.

Sadly it's Firefox only :)

> It seems like it would make sense to turn renderTabs into it's own stateful component, as the state only matters for that particular component, not for the overall toolbar. Right now the overall component is only concerned with organizing all of the sub-rendering functions, so it is easy to read and understand. I think this would keep the top level component a lot more simple, and keep the added compelxity of maintaining overflow state in the child component. What do you think?

Sounds good I'll try to do that!

> Should this be the the ellpsis character "…" instead of three dots? Is there anything we should think about for L10N, or is this just considered a glyph?

I updated this to reuse the same CSS class as the one we have for a similar menu in the inspector sidebar.
It is now using the dropmarker icon and I moved the shared CSS class to common.css

I missed the fact that you already did a review so I pushed a new version without notifying you, sorry :/
Hi Greg, just to let you know I updated my patch taking your comments into account!
Flags: needinfo?(gtatum)
Comment on attachment 8832287 [details]
Bug 1335608 - add a button to select hidden tools when toolbox toolbar overflows;

https://reviewboard.mozilla.org/r/108622/#review113774

All the code looks great, but I did find two small keyboard navigation issues.

Awkward scrolling behavior STR:
 * Tab focus onto a toolbar tab
 * Hit the right arrow a bunch
 * Once the cursor goes to the obscured elements, the container scrolls over awkwardly and looks really weird.

I would also expect the left/right keyboard events to focus on the down button.

https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/devtools/client/framework/toolbox.js#1030
This method hacks the left/right arrow to work. I would suggest adding `role=button` to the div from the menu, and then update the querySelector in that method. I think the `role` property should be there anyway on the menu for accessibility.

Aria roles for buttons: https://www.w3.org/TR/wai-aria-practices/examples/button/button.html

::: devtools/client/framework/components/toolbox-tabs.js:1
(Diff revision 5)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Glad this is broken out into a separate file, thanks! This was bugging me about my implementation.
Attachment #8832287 - Flags: review?(gtatum)
Flags: needinfo?(gtatum)
Thanks for the review! This should address the weird scrolling behavior and add the expected keyboard navigation feature. 
I ended up using a button directly here, let me know if that's ok with you.
Should I forward the review?
Flags: needinfo?(gtatum)
No, I'll do it today. Thanks for the ping on it.
Comment on attachment 8832287 [details]
Bug 1335608 - add a button to select hidden tools when toolbox toolbar overflows;

https://reviewboard.mozilla.org/r/108622/#review117846

One small issue, if you tab through hitting "tab", then the toolbar should only catch once, while here it is catching on the down arrow as well.
Attachment #8832287 - Flags: review?(gtatum) → review+
Flags: needinfo?(gtatum)
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c865b539abea
add a button to select hidden tools when toolbox toolbar overflows;r=gregtatum
https://hg.mozilla.org/mozilla-central/rev/c865b539abea
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
QA Whiteboard: [good first verify]
I have reproduced this issue with nightly 54.0a1 (2017-02-01) (64-bit) in windows 10 (64bit)

This bug is now verified as fixed in latest beta 54.0b1 (64-bit)

Build ID 	20170420011827
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0

[bugday-20170419]
I have reproduced this bug with Nightly 54.0a1 (2017-01-31) (64-bit) on Ubuntu 16.04 (64  Bit)! 

This bug's fix is now verified with latest Beta 54.0b2!

Build ID   : 20170424215451
User Agent : Mozilla/5.0 (X11; Linux x86_64; rv:54.0) Gecko/20100101 Firefox/54.0

[bugday-20170426]
Marking as verified based on the above comments.
Status: RESOLVED → VERIFIED
QA Whiteboard: [good first verify] → [good first verify][testday-20170428]
status-firefox54: fixed → verified
See Also: → 948456
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: