Add menu button to access tools when toolbar overflows

VERIFIED FIXED in Firefox 54

Status

()

Firefox
Developer Tools: Framework
P3
enhancement
VERIFIED FIXED
5 months ago
12 days ago

People

(Reporter: jdescottes, Assigned: jdescottes)

Tracking

({dev-doc-needed})

53 Branch
Firefox 54
dev-doc-needed
Points:
---

Firefox Tracking Flags

(firefox54 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

5 months ago
Created attachment 8832286 [details]
hidden-tools-side-dock.png (this is extreme, but only two tools are visible here)

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.
Comment hidden (mozreview-request)
(Assignee)

Comment 2

5 months ago
Created attachment 8832289 [details]
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).
(Assignee)

Comment 3

5 months ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 6

5 months ago
mozreview-review
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 hidden (mozreview-request)
(Assignee)

Comment 9

5 months ago
mozreview-review-reply
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 :/
Comment hidden (mozreview-request)
(Assignee)

Comment 11

4 months ago
Hi Greg, just to let you know I updated my patch taking your comments into account!
Flags: needinfo?(gtatum)

Comment 12

4 months ago
mozreview-review
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)
Comment hidden (mozreview-request)
(Assignee)

Comment 14

4 months ago
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.
(Assignee)

Comment 15

4 months ago
Should I forward the review?
Flags: needinfo?(gtatum)
No, I'll do it today. Thanks for the ping on it.

Comment 17

4 months ago
mozreview-review
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)
Comment hidden (mozreview-request)

Comment 19

4 months ago
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

Comment 20

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c865b539abea
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Keywords: dev-doc-needed
QA Whiteboard: [good first verify]

Comment 21

2 months ago
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
(Assignee)

Updated

12 days ago
See Also: → bug 948456
You need to log in before you can comment on or make changes to this bug.