Display the icon and label always in the toolbox.

VERIFIED FIXED in Firefox 61

Status

enhancement
VERIFIED FIXED
a year ago
10 months ago

People

(Reporter: mantaroh, Assigned: mantaroh)

Tracking

(Blocks 1 bug)

unspecified
Firefox 61
Dependency tree / graph

Firefox Tracking Flags

(firefox61 verified)

Details

Attachments

(10 attachments, 2 obsolete attachments)

(Assignee)

Description

a year ago
Current developer tool tab menu will be iconization and show menu list button if tab button width is small. Related bug is as follow:

Bug 1338106 - Improve tab display when toolbox width is too small.
Bug 1335608 - Add menu button to access tools when toolbar overflows

This feature is so useful, however, I confused toolbar's icon sometimes. For example, I couldn't distinguish the icons of Debugger and Style. Icon of style editor uses '{}', this icon has evoked the image of javascript syntax to me.  I think this toolbox might have to display the set of icon and label. (i.e. the tab menu width will be fixed.) 

I thought two toolbox behavior pattern if tab menu width will be fixed.

Plan 1) Hide overflowed tab menu and add it to the menu list button.
  Toolbox hides the tab menu and display menu list button if tab menu is overflowed.
  Another browser(Edge/Chrome/Safari) does this behavior.
  User access the tab by using menu list button.

Plan 2) Add the horizontal scroll and display menu list button.
  Like the browser's tab, this plan will make toolbox to be scrollable.
  A user accesses the tab by using the scroll or menu list button.

Julian,
How do you think about this behavior?
I think that plan 1) will easy to use for the user of using another browser.
Flags: needinfo?(jdescottes)
(Assignee)

Comment 1

a year ago
This attachment is behavior of another browser.
Chrome 64 / Edge (Windows 10 Fall Creators Update 1709) / Safari TP(50).
(adding Victoria in cc, DevTool's UX designer)

Thanks for the feedback!

I don't know if we already have bugs filed about this, but that is something that has been mentioned in the past. We have some old bugs filed under [devtools-ux][devtools-toolbar], but nothing recent, and nothing really about the issues you mention here. 
https://bugzilla.mozilla.org/buglist.cgi?quicksearch=%5Bdevtools-ux%5D%5Bdevtools-toolbar%5D&list_id=14035743

Victoria, did we decide on a specific solution for the devtools toolbar overflow issue? Are there other bugs to track this?
Flags: needinfo?(jdescottes) → needinfo?(victoria)
Video of the current toolbar, as we resize the toolbox. Since I have a lot of tools enabled and some extensions, there is a point where you can almost only see the icons. The logic introduced in Bug 1338106 works well when there are only a few tools enabled, but not here.
Posted video fixed-tab-width.mp4
I think we can get closer to your Plan 1) if we stop shrinking the tabs' width. We can modify https://searchfox.org/mozilla-central/rev/61d400da1c692453c2dc2c1cf37b616ce13dea5b/devtools/client/themes/toolbox.css#90-103 and just add flex-shrink: 0 and remove the min-width. I think this would already make the devtools toolbar more usable. 

This is a video of a quick attempt at doing that. Let me know if you think this would be an improvement.
(Assignee)

Comment 5

a year ago
(In reply to Julian Descottes [:jdescottes][:julian] from comment #4)
> Created attachment 8955436 [details]
> fixed-tab-width.mp4
> 
> I think we can get closer to your Plan 1) if we stop shrinking the tabs'
> width. We can modify
> https://searchfox.org/mozilla-central/rev/
> 61d400da1c692453c2dc2c1cf37b616ce13dea5b/devtools/client/themes/toolbox.
> css#90-103 and just add flex-shrink: 0 and remove the min-width. I think
> this would already make the devtools toolbar more usable. 
> 
> This is a video of a quick attempt at doing that. Let me know if you think
> this would be an improvement.

Thanks, Julian.

Your behavior is pretty good to me. I think the toolbox might need the following menu features.

 * An overflowed tab menu goes to list menu button.
 * A selected tab menu which is from list menu button goes shown toolbox tabs.
 * Make tab menu to be draggable move. (Maybe another bug?)

The Safari / Chrome can move with dragging, and shown tab menu and menu list button are exclusive(i.e. the way to access tool is either clicking tool tab or selecting list menu.).

Maybe, I think that we can detect overflow/underflow by using intersection observer and notify via React's property.
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #5)
>  * Make tab menu to be draggable move. (Maybe another bug?)

(As discussed, this refers to re-ordering tabs.)

And yes I definitely think this should be a separate bug.
(Assignee)

Comment 7

a year ago
(In reply to Brian Birtles (:birtles) from comment #6)
> (In reply to Mantaroh Yoshinaga[:mantaroh] from comment #5)
> >  * Make tab menu to be draggable move. (Maybe another bug?)
> 
> (As discussed, this refers to re-ordering tabs.)
> 
> And yes I definitely think this should be a separate bug.

Thanks, Brian.
I filed the bug 1443076.
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #5)
> 
> Thanks, Julian.
> 
> Your behavior is pretty good to me. I think the toolbox might need the
> following menu features.
> 
>  * An overflowed tab menu goes to list menu button.
>  * A selected tab menu which is from list menu button goes shown toolbox
> tabs.
>  * Make tab menu to be draggable move. (Maybe another bug?)
> 
> The Safari / Chrome can move with dragging, and shown tab menu and menu list
> button are exclusive(i.e. the way to access tool is either clicking tool tab
> or selecting list menu.).
> 
> Maybe, I think that we can detect overflow/underflow by using intersection
> observer and notify via React's property.

If I understand correctly you would like to improve two things here:
- only show the overflowed tabs in the menu (today we always show everything)
- when a tab is selected, scroll the toolbar to make sure it is visible

I would still like to get feedback from :victoria here, but those improvements make sense to me and I think they are a natural evolution of our current approach.

Is this something you would like to work on? The implementation for the menu is at https://searchfox.org/mozilla-central/source/devtools/client/framework/components/toolbox-tabs.js in the method renderAllToolsButton(). When building the menu here we need to know which tabs are not visible. I don't know if we should use intersection observers, or check each tab only when creating the menu.

In order to keep the selected tab visible, the toolbox emits a "select" event at https://searchfox.org/mozilla-central/rev/d2b4b40901c15614fad2fa34718eea428774306e/devtools/client/framework/toolbox.js#1922 and "toolbox" is a prop of the toolbox-tabs component.
Ah yes! This is something I wanted to do during the Photon redesign, but we ran out of time.

- We need better breakpoints for collapsing tabs - squish down the padding, then hide the icons, but don't hide any of the text, just overflow into the overflow menu. I agree the icons shouldn't be used without text.
- Agreed that the overflow menu should show only hidden tabs. The overflow icon should be a smaller version of the Firefox >> icon.
- As Chrome does, the hidden tabs should "reorder" into view upon being selected from the menu.
- Related: put the icons on the right into into a meatball menu, so there's just RDM/meatball/close button. That will help save some space.

I see resorting of tabs to be lower priority, so a separate bug sounds good. 

For DevTools, I'd rather avoid Firefox's horizontal scrolling of tabs behavior.
Flags: needinfo?(victoria)
This is also a duplicate of Bug 1417134 - Implement the photon styles for the toolbox toolbar or perhaps this should become the main bug for it.
(Assignee)

Comment 11

a year ago
Thank, Julian, Victoria, Gabriel

(In reply to Victoria Wang [:victoria] from comment #9)
> Ah yes! This is something I wanted to do during the Photon redesign, but we
> ran out of time.
> 
> - We need better breakpoints for collapsing tabs - squish down the padding,
> then hide the icons, but don't hide any of the text, just overflow into the
> overflow menu. I agree the icons shouldn't be used without text.
> - Agreed that the overflow menu should show only hidden tabs. The overflow
> icon should be a smaller version of the Firefox >> icon.
> - As Chrome does, the hidden tabs should "reorder" into view upon being
> selected from the menu.
> - Related: put the icons on the right into into a meatball menu, so there's
> just RDM/meatball/close button. That will help save some space.
> 
> I see resorting of tabs to be lower priority, so a separate bug sounds good. 
> 
> For DevTools, I'd rather avoid Firefox's horizontal scrolling of tabs
> behavior.

I think this behavior is reasonable. This attachment is experimental screen capture of detecting the overflow and hiding to the meatball menu.

For this feature, we need to:
 * Change the toolbox-tab styles (As mentioned by julian on comment 4).
 * ToolboxTab should detect intersection and notify parent component(to ToolboxTabs).
 * ToolboxTabs should manage hidden menu.
 * ToolboxTabs should hide tab menu and add this menu to meatball menu when notifying intersection from ToolboxTab.
 * ToolboxTabs should reorder the tab menus if select hidden menu from meatball menu.
I would prefer the chrome approach to the tabs where we don't show the tab name or icon at all when the name is cut off and just overflows into the tab menu. This is different from the overflowing behaviour you are showing in the video.
This should also be done for the sidebar tabs.
Thanks Mantaroh, this is already looking great! I agree with gl, ideally the tab text is either fully visible or overflowed.
(Assignee)

Comment 15

a year ago
(In reply to Victoria Wang [:victoria] from comment #14)
> Thanks Mantaroh, this is already looking great! I agree with gl, ideally the
> tab text is either fully visible or overflowed.

Thanks for the feedback.
I agree with this behavior. This attachment is the screen capture after changing this behavior.
Attachment #8956353 - Attachment is obsolete: true
(Assignee)

Comment 16

a year ago
Julian,
I have a question about tab menu ordinal. This tab ordinal is defined in definition.js[1]. Toolbox register this definition when loading the Toolbox or changing to the enable setting from the options panel. I think that we need to remain this tab order into somewhere(e.g. devtools preference) in order to reorder tab menu when selected tab menu will be hidden menu. Maybe, I think that other feature like the bug 1226272 uses these ordinal settings too. Can I add this ordinal setting to devtool's preference?

[1] https://searchfox.org/mozilla-central/rev/33cc9e0331da8d9ff3750f1e68d72d61201176cb/devtools/client/definitions.js#46
Flags: needinfo?(jdescottes)
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #15)
> Created attachment 8957059 [details]
> experimental-implementation-of-toolbox-tabs.mp4
> 
> (In reply to Victoria Wang [:victoria] from comment #14)
> > Thanks Mantaroh, this is already looking great! I agree with gl, ideally the
> > tab text is either fully visible or overflowed.
> 
> Thanks for the feedback.
> I agree with this behavior. This attachment is the screen capture after
> changing this behavior.

This does look better but I would expect the dropdown arrow or chervon to follow the last tab like in Chrome's tabs than statically positioned. I would also expect when I select the tab inside the dropdown menu that it would reappear in the tab menu. So, in some sense we do need the ability to reorder the last tab at least.
First thanks for working on this, the last version seems really nice! I agree with gl's suggestion to have the chevron next to the tab, but other than this it looks good.

Regarding the ordinal, it depends on how you want to handle "showing a hidden tab". 

Let's say we have 

  [tab1, tab2, tab3] visible and [tab4, tab5, tab6, tab7] hidden 

  If the user selects tab6 in the menu, what should happen? 

What Chrome does is that they only make sure that the selected tab is visible. If the selected tab is not visible, it is swapped with the last visible element. In my example, tab6 would replace tab3 and we would have

  [tab1, tab2, tab6] visible and [tab3, tab4, tab5, tab7] hidden

But this order should not really be persisted. If you increase the toolbox width we should have:

  [tab1, tab2, tab3, tab6] visible and [tab4, tab5, tab7] hidden (and so on...)

Also if you happen to select another tool when tab6 is selected, tab6 would move back to the hidden tab. 

The advantages of the solution is that it's only based on information we already persist: the selected tab and the default order. As you mentioned, ordering will be added in bug 1226272, but I think that it should remain separated from the logic that will be introduced here, which reorders tabs on the fly to ensure the selected tab is always visible.

Hope that makes sense! If I misunderstood the intent, could you share your patch? I guess it will be easier to understand if we can look at the current approach.
Flags: needinfo?(jdescottes)
Btw, you can find the Photon chevron icon here: https://design.firefox.com/icons/viewer/#overflow

I would suggest displaying it at 12x12px Gray-50, spaced evenly from the last tab, so that it looks like this:

https://mozilla.invisionapp.com/share/M5G8OO1ZVE4#/screens/283871189
Related is bug 1417727 . This seems to cover a similar issue, with preferring icons over text. If we think this is included in this work, I can close it as dupe.

If I read correctly, this is a significant work and possibly part of a larger phased redesign work, please fill out a PRD [1] and track the work in the backlog. This also solves that tracking eng/product/design decisions through bugzilla comments hits a limit at some point.

[1]: https://docs.google.com/document/d/18I0YSU9agomMWuyBgM2reeplJx21U_ZiAjZ_RigsS7E/edit#
Taking a step back I will work with y'all to start documenting work and milestones for polish work; as we have a few items in our backlog that are connected and need a place for tracking decisions.

The work scoped here to improve overflow looks great! Let me know when product can help or if there is user research/testing we can do to validate ideas.
Thanks Harald, I'll fill out the PRD later today.
See Also: → 1417134
See Also: → 1417727
(Assignee)

Comment 23

a year ago
I applied the comment of gl and julian and victoria to my local code. This video is experimental implementation.
I thought that we can use intersection observer, however, we can't detect the underflow if the target element is hidden. So this experimental implementation detects underflow/overflow by using the target window resizing event.

I will submit the code after refactoring it since current implementation is too rough :|
Attachment #8957059 - Attachment is obsolete: true
The stretching behavior is looking good! I'd suggest removing the vertical lines from the overflow button - also, it looks like it needs a little more side-margin to match spacing between the tabs. Here's the icon link again, can be displayed at 12x12px Gray-50 https://design.firefox.com/icons/viewer/#overflow
(Assignee)

Comment 25

a year ago
This is WIP Patch for this bug.

I need the following work yet.
 * Hide the overflowed tab when opening the dev tool window.
 * Update the tab menu when enabling or disabling from the options panel.
 * Reduce the exceeded render.
 * Create the browser mochi test for resizing dev tool window.

I think that I can implement this more simply. So I will brush up this patch before reviewing this.
Assignee: nobody → mantaroh
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8962556 - Flags: review?(jdescottes)

Comment 27

a year ago
mozreview-review
Comment on attachment 8962556 [details]
Bug 1442531 - Part 1. Make chevron button of devtool to be exclusive and apply the photon design.

https://reviewboard.mozilla.org/r/231360/#review236908

I had a chance to play with it and wanted to quickly provide a comment on the svg and css.

::: devtools/client/jar.mn:154
(Diff revision 1)
>      skin/images/command-console.svg (themes/images/command-console.svg)
>      skin/images/command-eyedropper.svg (themes/images/command-eyedropper.svg)
>      skin/images/command-rulers.svg (themes/images/command-rulers.svg)
>      skin/images/command-measure.svg (themes/images/command-measure.svg)
>      skin/images/command-noautohide.svg (themes/images/command-noautohide.svg)
> +    skin/images/command-meatball.svg (themes/images/command-meatball.svg)

This isn't really a meatball icon, and should be renamed to be chevron.

::: devtools/client/themes/common.css
(Diff revision 1)
>      transform: rotate(360deg);
>    }
>  }
>  
>  /* Common tabs styles */
> -

These new lines are intentional and shouldn't be removed.

::: devtools/client/themes/images/command-meatball.svg:4
(Diff revision 1)
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - 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/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16">

Victoria spec's indicated this should be 12 x 12 px. I would either resize here or in the CSS.

::: devtools/client/themes/toolbox.css:61
(Diff revision 1)
>    display: flex;
>    flex: 1;
>  }
>  
> -.toolbox-tabs-wrapper .all-tools-menu {
> +.toolbox-tabs-wrapper .tools-meatballmenu {
>    border-inline-end: 1px solid var(--theme-splitter-color);

We should probably remove this border completely.
Comment hidden (mozreview-request)
(Assignee)

Comment 29

a year ago
mozreview-review-reply
Comment on attachment 8962556 [details]
Bug 1442531 - Part 1. Make chevron button of devtool to be exclusive and apply the photon design.

https://reviewboard.mozilla.org/r/231360/#review236908

Thanks, Gabriel!

I addressed it.

> Victoria spec's indicated this should be 12 x 12 px. I would either resize here or in the CSS.

I addressed it by changing the view port size of svg.

Comment 30

a year ago
mozreview-review
Comment on attachment 8962556 [details]
Bug 1442531 - Part 1. Make chevron button of devtool to be exclusive and apply the photon design.

https://reviewboard.mozilla.org/r/231360/#review237330

::: devtools/client/framework/components/toolbox-tabs.js:35
(Diff revision 2)
>  
>    constructor(props) {
>      super(props);
>  
>      this.state = {
> -      overflow: false,
> +      hiddenTabIds: [],  // Array of hidden tool id.

We don't want to use inline comments here.

These state comments should be placed in the line before it.

::: devtools/client/framework/components/toolbox-tabs.js:104
(Diff revision 2)
> -      node.addEventListener("underflow", this.onUnderflow);
> +      }
>      }
>    }
>  
> -  removeFlowEvents() {
> +  // Detect overflow/underflow
> +  // selected          : If change the selected Tabs, specify it.

This doesn't follow our JSDoc convention.

See an example here https://searchfox.org/mozilla-central/source/devtools/client/aboutdebugging/modules/addon.js#50

::: devtools/client/jar.mn:154
(Diff revision 2)
>      skin/images/command-console.svg (themes/images/command-console.svg)
>      skin/images/command-eyedropper.svg (themes/images/command-eyedropper.svg)
>      skin/images/command-rulers.svg (themes/images/command-rulers.svg)
>      skin/images/command-measure.svg (themes/images/command-measure.svg)
>      skin/images/command-noautohide.svg (themes/images/command-noautohide.svg)
> +    skin/images/command-chevron.svg (themes/images/command-chevron.svg)

This shouldn't really need the command prefix, and should just be named chevron.svg or overflow.svg.

The command prefix is if its a command button, but this is a generic svg that I think we will use in a number of places.

Comment 31

a year ago
mozreview-review
Comment on attachment 8962556 [details]
Bug 1442531 - Part 1. Make chevron button of devtool to be exclusive and apply the photon design.

https://reviewboard.mozilla.org/r/231360/#review237668

Thanks for the patch! I quickly tested it and it works well. This is a really nice start. 

I found two small bugs with the current implementation:

bug 1: 
  - resize until the toolbar cannot even display one tab
  - resize again to make the toolbar big enough for all tabs
  -> chevron button remains displayed

bug 2:
  - in devtools options, add or remove any button in "Available Toolbox Buttons"
  - resize the toolbox to overflow some tools
  -> chevron button does not appear

Before starting the review, I see that there are some ESLint failures on your patch.
We are using eslint to make our syntax consistant. Follow the instructions at 
http://docs.firefox-dev.tools/contributing/eslint.html to setup eslint for your local 
development environment. This will help make your code more consistent with our existing
codebase, and it will be easier to review.

As Gabriel said, we should also try to follow the style of existing files for comments and 
JSDoc (which sadly are not checked with eslint).

The general idea for this patch is to measure the available width for the 
toolbox-tabs-wrapper, measure each tab, and based on this and on the selected tab, display
tabs. If some tabs are hidden, a button is displayed.

One of the big technical challenges here is that we need to perform a render in order to 
measure elements, and then we might rerender based on that. I had some prior ideas on how
to do that, and it heavily influenced my review. Feel free to push back if it looks wrong.

I think this can be simplified, see my comments for some pointers and feel free to reach 
out directly to me if you want to discuss this. I'm not really used to React so I might
be overlooking a few things :) 

Also I think we should focus on the implementation in toolbox-tabs before looking at the 
tests.

::: devtools/client/framework/components/toolbox-tabs.js:36
(Diff revision 2)
>    constructor(props) {
>      super(props);
>  
>      this.state = {
> -      overflow: false,
> +      hiddenTabIds: [],  // Array of hidden tool id.
> +      allTabWidth: null, // Map with toolId and its width size.

I am not sure if this should be in the state. The tab of a specific tool will always have the same width. I think we could replace this with a cache that maps a tool-id to a tab width.

::: devtools/client/framework/components/toolbox-tabs.js:43
(Diff revision 2)
>  
> -    this.addFlowEvents = this.addFlowEvents.bind(this);
> -    this.removeFlowEvents = this.removeFlowEvents.bind(this);
> -    this.onOverflow = this.onOverflow.bind(this);
> -    this.onUnderflow = this.onUnderflow.bind(this);
> -  }
> +    this.getHiddenMenu = this.getHiddenMenu.bind(this);
> +    this.resizeHandler = this.resizeHandler.bind(this);
> +    this.updateTabs = this.updateTabs.bind(this);
> +    this.selectToolAndReorderTab = this.selectToolAndReorderTab.bind(this);
> +    this.collectAllTabInforamtion = this.collectAllTabInformation.bind(this);

typo: collectAllTabInforamtion -> collectAllTabInformation

but the bind is not needed here since this is never passed as a callback where the context would be lost.

::: devtools/client/framework/components/toolbox-tabs.js:51
(Diff revision 2)
> +  selectToolAndReorderTab(id) {
> +    this.props.selectTool(id);
> +    this.updateTabs(id);
> +  }

We should not have a specific selection logic here. A tool can be selected by the toolbar, but also by keyboard shorcuts, or programmatically by other tools.

To select a tool, we should always only call this.props.selectTool, and expct to update with a change of the "currentToolId" props

::: devtools/client/framework/components/toolbox-tabs.js:60
(Diff revision 2)
> +
> +  // Detect the regist and unregist tools.
> +  // ToolboxTabs need to calculate tabs width since the managed tab width
> +  // is not fixed width.
> +  componentDidUpdate(prevProps, prevState) {
> +    if ((this.props.panelDefinitions.length !=

I imagined the following pattern for componentDidUpdate:

  if (panelDefinitionsHaveChanged) {
    this.computeAllTabWidth(); // <- update the cache of tab width
    let hiddenTools = computeHiddenTools()
    if (hiddenTools != state.hiddenTools) {
      this.setState({ hiddenTools });
    }
  }

in conjunction with a componentWillUpdate:

  if (panelDefinitionsWillChange) {
    // force next render to show all tabs to compute tab width
    // in componentDidUpdate
    nextState.hiddenTools = []; 
  }

The current approach is trying to handle differently cases were tools are added or removed. It is nice but I find it harder to follow.

::: devtools/client/framework/components/toolbox-tabs.js:79
(Diff revision 2)
> -  componentDidUpdate() {
> -    this.addFlowEvents();
> +      if (prevPanelDefinitions.length > currPanelDefinitions.length) {
> +        // Remove tools
> +        let removeToolId;
> +        for (const id of prevPanelDefinitions) {
> +          if (!currPanelDefinitions.includes(id)){
> +            removeToolId = id;

If I understand correctly, this assumes only one tool is removed at a time. While this sounds reasonable, programmatically there is no guarantee this will always be the case.

Similarly, comparing the length of the definitions is not enough to know if they are identical. We should compare the ids contained in each one of them.

::: devtools/client/framework/components/toolbox-tabs.js:107
(Diff revision 2)
>  
> -  removeFlowEvents() {
> +  // Detect overflow/underflow
> +  // selected          : If change the selected Tabs, specify it.
> +  // tabs              : If change the panel definition, specify it.
> +  // currentHiddenTabs : If change the hidden tool, specify it.
> +  updateTabs(selectedId, tabs, currentHiddenTabs) {

The main purpose of this method is to compute the hidden and visible tabs. We should normally have everything we need in the props and state to compute this information. Could we rewrite it as a method that takes no argument and returns an array of visible or hidden tool ids ?

  getHiddenToolIds() {}
  
for instance.

The current implementation using optional parameters that fallback to the state is hard to follow.

::: devtools/client/framework/components/toolbox-tabs.js:131
(Diff revision 2)
>      });
> +    let willHideTabs = [];
> +
> +    // If the chevron menu is not rendered yet, we use fixed width(18px).
> +    let chevronButton = node.getElementsByClassName("tools-chevronmenu");
> +    let chevronButtonWidth =

The button has a fixed width so I think it is fine to hardcode it as a constant rather than measuring it. The current value is 26px.

::: devtools/client/framework/components/toolbox-tabs.js:182
(Diff revision 2)
> +    let node = findDOMNode(this);
> +    let tempTabs = this.collectAllTabInformation(node);
> +    this.updateTabs(null, tempTabs);
> +  }
> +
> +  willComponentUnmount() {

the correct name should be componentWillUnmount

::: devtools/client/framework/components/toolbox-tabs.js:250
(Diff revision 2)
>  
> -/**
> +  /**
> - * Render a button to access all tools, displayed only when the toolbar presents an
> +   * Render a button to access all tools, displayed only when the toolbar presents an
> - * overflow.
> +   * overflow.
> - */
> +   */
> -function renderAllToolsButton(props) {
> +  renderToolsChevronButton(props, getMenusFunc, selectTool) {

Since renderToolsChevronButton is part of the Component's prototype, we don't have to pass all those arguments. They can all be retrieved on the instance.
Attachment #8962556 - Flags: review?(jdescottes)
Harald, Mantaroh has a nice first version up for review, I wanted to check if it was OK from a product perspective to proceed here?
Flags: needinfo?(hkirschner)
Status: NEW → ASSIGNED
(Assignee)

Comment 34

a year ago
Gabriel, Julian,
Thank you guys for reviewing this patch. I'll modify this patch.

(In reply to Julian Descottes [:jdescottes][:julian] from comment #31)
> Comment on attachment 8962556 [details]
> Bug 1442531 - Make chevron button of devtool to be exclusive and apply the
> photon design.
> 
> https://reviewboard.mozilla.org/r/231360/#review237668
> 
> The general idea for this patch is to measure the available width for the 
> toolbox-tabs-wrapper, measure each tab, and based on this and on the
> selected tab, display
> tabs. If some tabs are hidden, a button is displayed.
> 
> One of the big technical challenges here is that we need to perform a render
> in order to 
> measure elements, and then we might rerender based on that. I had some prior
> ideas on how
> to do that, and it heavily influenced my review. Feel free to push back if
> it looks wrong.
> 
> I think this can be simplified, see my comments for some pointers and feel
> free to reach 
> out directly to me if you want to discuss this. I'm not really used to React
> so I might
> be overlooking a few things :) 
> 

I think that we can use the "tool-registered/tool-unregistered" event of gDevTools. This event will dispatch from the options panel.
And I will add the cache of current sum of tab width size and tab width which should be displayed. If we have these cache, I think that we can decrease the performance problem since we don't need to calculate element's size when resizing the window.
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #34)
> Gabriel, Julian,
> Thank you guys for reviewing this patch. I'll modify this patch.
> 

Great! 

> I think that we can use the "tool-registered/tool-unregistered" event of
> gDevTools. This event will dispatch from the options panel.

You shouldn't have to add anything for registration/unregistration, the toolbox-tabs has panelDefinitions in its props, and you can trust toolbox.js to call setPanelDefinitions() on toolbox-controller.js which will then update toolbox-tabs. Try to only use the lifecycle methods in the toolbox-tabs component and the current props.

> And I will add the cache of current sum of tab width size and tab width
> which should be displayed. If we have these cache, I think that we can
> decrease the performance problem since we don't need to calculate element's
> size when resizing the window.

Sounds good, it would be enough to store only individual tab width in the cache, doing the sum is inexpensive.
(Assignee)

Comment 36

a year ago
(In reply to Julian Descottes [:jdescottes][:julian] from comment #31)
> Comment on attachment 8962556 [details]
> Bug 1442531 - Make chevron button of devtool to be exclusive and apply the
> photon design.
> 
> https://reviewboard.mozilla.org/r/231360/#review237668
> 
> Before starting the review, I see that there are some ESLint failures on
> your patch.
> We are using eslint to make our syntax consistant. Follow the instructions
> at 
> http://docs.firefox-dev.tools/contributing/eslint.html to setup eslint for
> your local 
> development environment. This will help make your code more consistent with
> our existing
> codebase, and it will be easier to review.
> 

Ah, a current this folder looks like ignored by eslintignore file. If I removed the framework directory definition of ignoring file, a test has started.[1]

julian,
I checked the addressed patch, the setState of componentDidUpdate and componentDidMount is error[2]. I think that I need this implementation in this case, can I skip this error type?

[1] https://searchfox.org/mozilla-central/rev/57bbc1ac58816dc054df242948f3ecf75e12df5f/.eslintignore#107-112
[2] https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/no-did-update-set-state.md

Try: https://hg.mozilla.org/try/rev/6103895413387b0a033ca241b437b2ef8ad8cc8e
Flags: needinfo?(jdescottes)
(Assignee)

Comment 37

a year ago
mozreview-review
Comment on attachment 8962556 [details]
Bug 1442531 - Part 1. Make chevron button of devtool to be exclusive and apply the photon design.

https://reviewboard.mozilla.org/r/231360/#review237772

::: devtools/client/framework/components/toolbox-tabs.js:35
(Diff revision 2)
>  
>    constructor(props) {
>      super(props);
>  
>      this.state = {
> -      overflow: false,
> +      hiddenTabIds: [],  // Array of hidden tool id.

OK. I addressed it.

::: devtools/client/framework/components/toolbox-tabs.js:43
(Diff revision 2)
>  
> -    this.addFlowEvents = this.addFlowEvents.bind(this);
> -    this.removeFlowEvents = this.removeFlowEvents.bind(this);
> -    this.onOverflow = this.onOverflow.bind(this);
> -    this.onUnderflow = this.onUnderflow.bind(this);
> -  }
> +    this.getHiddenMenu = this.getHiddenMenu.bind(this);
> +    this.resizeHandler = this.resizeHandler.bind(this);
> +    this.updateTabs = this.updateTabs.bind(this);
> +    this.selectToolAndReorderTab = this.selectToolAndReorderTab.bind(this);
> +    this.collectAllTabInforamtion = this.collectAllTabInformation.bind(this);

OK. I removed it.

::: devtools/client/framework/components/toolbox-tabs.js:51
(Diff revision 2)
> +  selectToolAndReorderTab(id) {
> +    this.props.selectTool(id);
> +    this.updateTabs(id);
> +  }

Oh, If we select tool with shortcut key, this patch will distinguish current tool correctly.

::: devtools/client/framework/components/toolbox-tabs.js:104
(Diff revision 2)
> -      node.addEventListener("underflow", this.onUnderflow);
> +      }
>      }
>    }
>  
> -  removeFlowEvents() {
> +  // Detect overflow/underflow
> +  // selected          : If change the selected Tabs, specify it.

Thanks!

I addressed this comment.

::: devtools/client/framework/components/toolbox-tabs.js:131
(Diff revision 2)
>      });
> +    let willHideTabs = [];
> +
> +    // If the chevron menu is not rendered yet, we use fixed width(18px).
> +    let chevronButton = node.getElementsByClassName("tools-chevronmenu");
> +    let chevronButtonWidth =

OK. I use the fixed chevron menu size.

::: devtools/client/framework/components/toolbox-tabs.js:250
(Diff revision 2)
>  
> -/**
> +  /**
> - * Render a button to access all tools, displayed only when the toolbar presents an
> +   * Render a button to access all tools, displayed only when the toolbar presents an
> - * overflow.
> +   * overflow.
> - */
> +   */
> -function renderAllToolsButton(props) {
> +  renderToolsChevronButton(props, getMenusFunc, selectTool) {

Ah, yes!
I addressed it.
(Assignee)

Comment 38

a year ago
mozreview-review-reply
Comment on attachment 8962556 [details]
Bug 1442531 - Part 1. Make chevron button of devtool to be exclusive and apply the photon design.

https://reviewboard.mozilla.org/r/231360/#review237668

Thanks for review.
I changed the way of detecting to remove/add tool.

> I imagined the following pattern for componentDidUpdate:
> 
>   if (panelDefinitionsHaveChanged) {
>     this.computeAllTabWidth(); // <- update the cache of tab width
>     let hiddenTools = computeHiddenTools()
>     if (hiddenTools != state.hiddenTools) {
>       this.setState({ hiddenTools });
>     }
>   }
> 
> in conjunction with a componentWillUpdate:
> 
>   if (panelDefinitionsWillChange) {
>     // force next render to show all tabs to compute tab width
>     // in componentDidUpdate
>     nextState.hiddenTools = []; 
>   }
> 
> The current approach is trying to handle differently cases were tools are added or removed. It is nice but I find it harder to follow.

I added the logic which is "shouldRecalculateAndRender". This will judge that we should re-calculate the tab width or not and it is used at componentWillUpdate and componentDidUpdate.
Comment hidden (mozreview-request)
Thanks for the recording, :jdescottes! This looks like a good first shot at solving the overflow issue.

In follow up work I'd like to explore other aspects of responsiveness, like hiding the icons before tabs are starting to flow into the overflow. Let's see what users say to this change and if it changes engagement with panels that quickly go into overflow in vertical mode.
Flags: needinfo?(hkirschner) → needinfo?(victoria)

Comment 41

a year ago
mozreview-review
Comment on attachment 8962556 [details]
Bug 1442531 - Part 1. Make chevron button of devtool to be exclusive and apply the photon design.

https://reviewboard.mozilla.org/r/231360/#review238926

::: devtools/client/framework/components/toolbox-tabs.js:167
(Diff revision 3)
> -      selectTool,
> -    }));
>  
> -    // A wrapper is needed to get flex sizing correct in XUL.
> -    return div(
> -      {
> +  resizeHandler(evt) {
> +    let node = findDOMNode(this);
> +    const toolboxWidth = parseInt(getComputedStyle(node).width, 10);

"resize" dispatches _lots_ of events.
And `getComputedStyle` here, introduces a synchronous reflow in order to compute the current size of the node.

This is a common slow pattern (listening for resize and compute an element size).

A common mitigation is the aggregate the resize events like this:
https://searchfox.org/mozilla-central/rev/f5fb323246bf22a3a3b4185882a1c5d8a2c02996/devtools/client/netmonitor/src/components/RequestListHeader.js#97-99
  requestIdleCallback will aggregate all resize events until the user is done resizing as it will only fire the callback once the user stop interacting with the UI.
If this is too infrequent, this function accepts a `timeout` parameter to ensure it fires at least after X ms.

Also, you are calling getComputedStyle(node) from: calculateOverflowedTabs, updateAllTabsWidth and here. And I think it will reflow everytime you call it, even if you call it from the same event loop. So you may try to call it only once and pass the toolbox width as an argument.

Comment 42

a year ago
mozreview-review
Comment on attachment 8962556 [details]
Bug 1442531 - Part 1. Make chevron button of devtool to be exclusive and apply the photon design.

https://reviewboard.mozilla.org/r/231360/#review238974

Thanks for updating the patch mantaroh, this looks much better! 
I still have a few suggestions to make the implementation simpler, let me know what you think.

::: devtools/client/framework/components/toolbox-tabs.js:11
(Diff revision 3)
>  const { Component, createFactory } = require("devtools/client/shared/vendor/react");
>  const dom = require("devtools/client/shared/vendor/react-dom-factories");
>  const PropTypes = require("devtools/client/shared/vendor/react-prop-types");
>  const {findDOMNode} = require("devtools/client/shared/vendor/react-dom");
>  const {button, div} = dom;
> +const {gDevTools} = require("devtools/client/framework/devtools");

We should remove this, it is not used here.

::: devtools/client/framework/components/toolbox-tabs.js:54
(Diff revision 3)
> +
> +    this.resizeHandler = this.resizeHandler.bind(this);
> +  }
> +
> +  componentDidMount() {
> +    window.removeEventListener("resize", this.resizeHandler);

Now that componentWillUnmount has the correct name, is it still necessary to remove the event listener here?

::: devtools/client/framework/components/toolbox-tabs.js:76
(Diff revision 3)
> +    }
>    }
>  
> -  componentWillUnmount() {
> -    this.removeFlowEvents();
> +  componentDidUpdate(prevProps, prevState) {
> +    if (this.shouldRecalculateAndRender(prevProps, this.props)) {
> +      this._registeredToolTabsWidthMap = this.updateAllTabsWidth();

Another optimization here is to always keep the same cache. If we know that Inspector is 120px, there is no need to measure this tab again when the list of tool changes.

::: devtools/client/framework/components/toolbox-tabs.js:91
(Diff revision 3)
> -      node.addEventListener("overflow", this.onOverflow);
> -      node.addEventListener("underflow", this.onUnderflow);
> -    }
> +  }
> +
> +  shouldRecalculateAndRender(prevProps, nextProps) {
> +    return prevProps.panelDefinitions.length !== nextProps.panelDefinitions.length ||

We can not rely only on the length here. Different arrays of tools with the same length can have tabs of different sizes.

::: devtools/client/framework/components/toolbox-tabs.js:117
(Diff revision 3)
> -  onOverflow() {
> -    this.setState({
> -      overflow: true
> +  /**
> +   * Return the overflowed tab array from currently displayed tab buttons.
> +   * If calculated result is same to a current overflowed tab array, this
> +   * function will return undefined.
> +   */
> +  calculateOverflowedTabs() {

The implementation of the method can be simpler, I think we can avoid using alreadyOverflowed and prevTabId. 

1: get the information needed: 
- size of the toolbar (getComputedStyle(node).width) - list of tools enabled (props.panelDefinitions)
- selected tool (props.currentToolId) 

2: fill an array of visible tool ids by looping on panelDefinitions and increasing the sumWidth until you reach toolboxWidth

3: if the selected tool is not in the visible tools, replace the last tool of the array by the selected tool. Be careful that the selectedTool can also be something that is not in panelDefinitions, such as "options". In this case you should ignore this step. 

4: finally create the overflowed array by filtering panelDefinitions (for instance `panelDefinitions.filter(d => visibleTools.includes(d.id));`)

Note: I find it easier to reason with visible tools rather than hidden tools, and you can probably do this differently if you focus on hidden tools. The important part is just to avoid the "previous/already" variables, which make the code a bit hard to understand.

::: devtools/client/framework/components/toolbox-tabs.js:120
(Diff revision 3)
> +   * function will return undefined.
> +   */
> +  calculateOverflowedTabs() {
> +    let node = findDOMNode(this);
> +    const toolboxWidth = parseInt(getComputedStyle(node).width, 10);
> +    let selectedId = this.props.toolbox.currentToolId;

currentToolId is already in the props for this component, so we should be able to retrieve it via:

let { currentToolId } = this.props;

::: devtools/client/framework/components/toolbox-tabs.js:131
(Diff revision 3)
>  
> -  onUnderflow() {
> -    this.setState({
> -      overflow: false
> +    // 26px is chevron button width
> +    let sumWidth = this.state.overflowedTabIds.length > 0 ? 26 : 0;
> +    let prevTabId;
> +
> +    this._registeredToolTabsWidthMap.forEach((val, key, map) => {

If we change the cache to keep all information, you can not rely on it for looping here. Instead, loop on this.props.panelDefinitions.

::: devtools/client/framework/components/toolbox-tabs.js:157
(Diff revision 3)
> +          prevTabId = key;
> +        }
> +      }
> +    });
>  
> -  /**
> +    if (this.state.overflowedTabIds.length ===

I think it would be nicer to keep this logic out of this function. This method is complex enough to only return the list of hidden ids.

Maybe another method e.g. didOverflowTabIdsChange(tabIds) ? 

Also, we cannot rely on the length to know if the lists are identical, you need to compare the keys and the order. You could for instance compare the output of .join("-") between the two arrays.

::: devtools/client/framework/components/toolbox-tabs.js:166
(Diff revision 3)
> -      panelDefinition,
> -      selectTool,
> -    }));
>  
> -    // A wrapper is needed to get flex sizing correct in XUL.
> -    return div(
> +  resizeHandler(evt) {
> +    let node = findDOMNode(this);

What about simplifying the whole function to just

  let overflowedTabs = this.calculateOverflowedTabs();
  if (overflowedTabs) {
    this.setState({
      overflowedTabIds: overflowedTabs
    });
  }

We already measured the size of the tabs (it is in the cache) and we need to measure the size of the toolbox anyway. So we are not going to have a significant performance win by skipping a call to calculateOverflowedTabs.

::: devtools/client/framework/components/toolbox-tabs.js:211
(Diff revision 3)
>  
> -/**
> +  /**
> - * Render a button to access all tools, displayed only when the toolbar presents an
> +   * Render a button to access all tools, displayed only when the toolbar presents an
> - * overflow.
> +   * overflow.
> - */
> +   */
> -function renderAllToolsButton(props) {
> +  renderToolsChevronButton(props) {

We should not pass props here, since the method is now on the prototype of the component, you can use this.props.

::: devtools/client/framework/components/toolbox-tabs.js:293
(Diff revision 3)
> +        {
> +          className: "toolbox-tabs"
> +        },
> +        tabs,
> +        (this.state.overflowedTabIds.length > 0)
> +          ? this.renderToolsChevronButton(this.props)

no need to pass this.props here, renderToolsChevronButton also has access to this.props
Attachment #8962556 - Flags: review?(jdescottes)
> I checked the addressed patch, the setState of componentDidUpdate and componentDidMount is error[2]. 
> I think that I need this implementation in this case, can I skip this error type?

I don't see any other solution here. We have to perform a render to get information that will change the state of the component. Let's leave it as is, we can either add `// eslint-disable-next-line` comments or extract the code that does the setState in a separate method.
Flags: needinfo?(jdescottes)
Comment hidden (mozreview-request)
(Assignee)

Comment 45

a year ago
mozreview-review-reply
Comment on attachment 8962556 [details]
Bug 1442531 - Part 1. Make chevron button of devtool to be exclusive and apply the photon design.

https://reviewboard.mozilla.org/r/231360/#review238926

> "resize" dispatches _lots_ of events.
> And `getComputedStyle` here, introduces a synchronous reflow in order to compute the current size of the node.
> 
> This is a common slow pattern (listening for resize and compute an element size).
> 
> A common mitigation is the aggregate the resize events like this:
> https://searchfox.org/mozilla-central/rev/f5fb323246bf22a3a3b4185882a1c5d8a2c02996/devtools/client/netmonitor/src/components/RequestListHeader.js#97-99
>   requestIdleCallback will aggregate all resize events until the user is done resizing as it will only fire the callback once the user stop interacting with the UI.
> If this is too infrequent, this function accepts a `timeout` parameter to ensure it fires at least after X ms.
> 
> Also, you are calling getComputedStyle(node) from: calculateOverflowedTabs, updateAllTabsWidth and here. And I think it will reflow everytime you call it, even if you call it from the same event loop. So you may try to call it only once and pass the toolbox width as an argument.

Thanks, ochameau,

I tried the requestIdleCallback with 300ms timeout, I think that the behavior looks OK.

This patch uses the getComputedStyle in three functions:


1. resizeHandler - calculate the **wrapped tab width**. called this function when resizing the window.


2. calculateOverflowedTabs -  calculate the **wrapped tab width**. called this function when
      * mounting this component at the first time.
      * finishing the component update if selected tool or num of registered tabs is changed.
      * detecting overflow or underflow tab.


3. updateAllTabsWidth -calculate **each tabs width**. called this function when:
      * mounting this component at the first time.
      * finishing the component update if selected tool or num of registered tabs is changed.

So I think that this patch will not call the 'getComputedStyle' of the same element.
(Assignee)

Comment 46

a year ago
mozreview-review-reply
Comment on attachment 8962556 [details]
Bug 1442531 - Part 1. Make chevron button of devtool to be exclusive and apply the photon design.

https://reviewboard.mozilla.org/r/231360/#review238974

Thanks julian!
I updated the patch, this update changed the mostrly logic of detecting overflowed tab.

> The implementation of the method can be simpler, I think we can avoid using alreadyOverflowed and prevTabId. 
> 
> 1: get the information needed: 
> - size of the toolbar (getComputedStyle(node).width) - list of tools enabled (props.panelDefinitions)
> - selected tool (props.currentToolId) 
> 
> 2: fill an array of visible tool ids by looping on panelDefinitions and increasing the sumWidth until you reach toolboxWidth
> 
> 3: if the selected tool is not in the visible tools, replace the last tool of the array by the selected tool. Be careful that the selectedTool can also be something that is not in panelDefinitions, such as "options". In this case you should ignore this step. 
> 
> 4: finally create the overflowed array by filtering panelDefinitions (for instance `panelDefinitions.filter(d => visibleTools.includes(d.id));`)
> 
> Note: I find it easier to reason with visible tools rather than hidden tools, and you can probably do this differently if you focus on hidden tools. The important part is just to avoid the "previous/already" variables, which make the code a bit hard to understand.

OK. I addressed it.
In step 3, I considered that total tab width is not smaller than toolbox width after replaced current tool tab and last visible tab. So we need to recursively call this shuffling process.

Comment 47

a year ago
mozreview-review
Comment on attachment 8962556 [details]
Bug 1442531 - Part 1. Make chevron button of devtool to be exclusive and apply the photon design.

https://reviewboard.mozilla.org/r/231360/#review239586

Thanks for the updated patch Mantaroh! There are still some unaddressed items from the previous review, I tried to be more precise here. 
Please contact me if anything is unclear!

::: devtools/client/framework/components/toolbox-tabs.js:45
(Diff revision 4)
> -    this.removeFlowEvents = this.removeFlowEvents.bind(this);
> -    this.onOverflow = this.onOverflow.bind(this);
> -    this.onUnderflow = this.onUnderflow.bind(this);
> +    // React's lifecycle. If tool is registered, ToolboxTabs will add/remove
> +    // target tool id to/from this map.
> +    this._registeredToolTabsWidthMap = new Map();
> +
> +    // This is to store the sum of currently displayed tool tab size due to
> +    // the performance problem.

I would like to see profiles highlighting the gains for this performance improvement, see my comment on the resizeHandler method.

Here are two profiles I made:
- with the optimization (+ a fix for infinite recursion); https://perfht.ml/2Jm5l0G
- without the optimization https://perfht.ml/2Jns6B9

My scenario was:
- enable all possible tools
- increase window size to show everything
- start profiling
- resize to smallest width
- resize to original width
- stop profiling

I don't see a real difference between the two profiles.

::: devtools/client/framework/components/toolbox-tabs.js:54
(Diff revision 4)
> +
> +    this.resizeHandler = this.resizeHandler.bind(this);
> +  }
> +
> +  componentDidMount() {
> +    window.removeEventListener("resize", this.resizeHandler);

From previous review: I don't think this is needed anymore. If it is, we should add a comment explaining why.

::: devtools/client/framework/components/toolbox-tabs.js:58
(Diff revision 4)
> +    let overflowedTabs = this.calculateOverflowedTabs();
> +    if (overflowedTabs) {
> +      // eslint-disable-next-line
> +      this.setState({
> +        overflowedTabIds: overflowedTabs
> +      });
> +    }

Can we extract all of this to a dedicated method, since it's called from componentDidMount, componentDidUpdate (and most likely from the resize handler)

::: devtools/client/framework/components/toolbox-tabs.js:119
(Diff revision 4)
> +    let tabs = new Map();
> +    let cachedTabs = this._registeredToolTabsWidthMap;
> +
> +    for (let child of thisNode.querySelectorAll(".devtools-tab")) {
> +      let childId = child.id.replace("toolbox-tab-", "");
> +      if (cachedTabs && cachedTabs.has(childId)) {
> +        tabs.set(childId, cachedTabs.get(childId));
> +      } else {
> +        let cs = getComputedStyle(child);
> +        tabs.set(childId, parseInt(cs.width, 10));
> +      }
> +    }
> +    this._registeredToolTabsWidthMap = tabs;

no need to create a new map here, just add more information to the existing one. 

updateRegisteredToolTabsWidthMap() {
  let thisNode = findDOMNode(this);
  for (let tab of thisNode.querySelectorAll(".devtools-tab")) {
    let tabId = tab.id.replace("toolbox-tab-", "");
    if (!cachedTabs.has(tabId)) {
      let cs = getComputedStyle(tab);
      tabs.set(tabId, parseInt(cs.width, 10));
    }
  }
}

This way if we disable tools, the map will still keep the size in the cache

::: devtools/client/framework/components/toolbox-tabs.js:144
(Diff revision 4)
> +  insertToolTabToToolbox(maxWidth,
> +                         toolId,
> +                         toolIdWidth,
> +                         currentVisibleTabs,
> +                         currentSumWidth) {
> +    if ((currentSumWidth + toolIdWidth) < maxWidth) {

If the toolbox width (ie maxWidth) is lower than toolIdWidth (ie the width of the select tool tab), this goes into infinite recursion and freezes firefox. A loop approach would be less prone to this kind of issues.

::: devtools/client/framework/components/toolbox-tabs.js:170
(Diff revision 4)
> -      currentToolId,
> -      focusButton,
> -      focusedButton,
> -      highlightedTools,
> -      panelDefinitions,
> -      selectTool,
> +    const toolboxWidth = parseInt(getComputedStyle(node).width, 10);
> +    let { currentToolId } = this.props;
> +    let enabledTabs = this.props.panelDefinitions.map(def => def.id);
> +    let willOverflowTabs = [];
> +
> +    // 26px is chevron button width

Move this comment and the variable to a const on top of the file:

const CHEVRON_BUTTON_WIDTH = 26px;

In the comment it would be good to mention which css class and css file corresponds to this 26px, so that in the future we can check they are still in sync.

::: devtools/client/framework/components/toolbox-tabs.js:171
(Diff revision 4)
> -      focusButton,
> -      focusedButton,
> -      highlightedTools,
> -      panelDefinitions,
> -      selectTool,
> -    } = this.props;
> +    let { currentToolId } = this.props;
> +    let enabledTabs = this.props.panelDefinitions.map(def => def.id);
> +    let willOverflowTabs = [];
> +
> +    // 26px is chevron button width
> +    let sumWidth = this.state.overflowedTabIds.length > 0 ? 26 : 0;

Here we read the current state to know if there is a chevron. This is wrong because we should only add a chevron if the next state will have overflow tabs.

For now can we just always add the chevron width, and improve this in a follow-up bug ?

::: devtools/client/framework/components/toolbox-tabs.js:185
(Diff revision 4)
> -    let tabs = panelDefinitions.map(panelDefinition => ToolboxTab({
> -      key: panelDefinition.id,
> +    if (!visibleTabs.includes(currentToolId) &&
> +        enabledTabs.includes(currentToolId)) {
> +      let selectedToolWidth = this._registeredToolTabsWidthMap.get(currentToolId);
> +      visibleTabs = this.insertToolTabToToolbox(toolboxWidth,
> -      currentToolId,
> +                                                currentToolId,
> -      focusButton,
> -      focusedButton,
> -      highlightedTools,
> -      panelDefinition,
> +                                                selectedToolWidth,
> +                                                visibleTabs,
> +                                                sumWidth);
> +    }

In the first version we could have simply swapped the last tool with the selected tool, but I like your approach trying to make sure we really have enough room for the selected tool. 

I am wondering if this would be easier to understand without recursion? Or maybe define your function inside calculateOverflowedTabs so that you don't have to pass around that many arguments.

::: devtools/client/framework/components/toolbox-tabs.js:195
(Diff revision 4)
> -      highlightedTools,
> -      panelDefinition,
> +                                                sumWidth);
> +    }
> -      selectTool,
> -    }));
>  
> -    // A wrapper is needed to get flex sizing correct in XUL.
> +    for (const id of enabledTabs) {

nit: you can swap this for

let willOverflowTabs = enabledTabs.filter(id => !visibleTabs.includes(id));

::: devtools/client/framework/components/toolbox-tabs.js:201
(Diff revision 4)
> -module.exports = ToolboxTabs;
> +    if (!this.equalToolIdArray(this.state.overflowedTabIds, willOverflowTabs)) {
> +      return willOverflowTabs;
> +    }
> +
> +    return undefined;

From previous review: I am still not confortable with this method returning either an array or undefined. If you prefer, repurpose the method to always update the state and not return an array of tab ids. For instance rename it to updateOverflowTabIds:

udpateOverflowTabIds() {
  // ... current code of the method
  let willOverflowTabs = enabledTabs.filter(/* ... */);
  if (!this.equalToolIdArray(this.state.overflowedTabIds, willOverflowTabs)) {
    this.setState({overflowTabIds: willOverflowTabs});
  } 
}

::: devtools/client/framework/components/toolbox-tabs.js:217
(Diff revision 4)
> +  shouldUpdateToolboxTabs(toolboxWidth) {
> +    // Use the cache for detecting overflow/underflow.
> +    if (this._sumToolTabsWidth != 0 &&
> +        this._nextToolTabSize != 0) {
> +      let space = toolboxWidth - this._sumToolTabsWidth;
> +      if (space > 0 && space < this._nextToolTabSize) {
> +        return false;
> +      }
> +      let displayedTabNum =
> +          this.props.panelDefinitions.length -
> +          this.state.overflowedTabIds.length;
> +      // Skip underflow calculation if the tab which is displayed
> +      // is current tool only.
> +      if (space < 0 && displayedTabNum == 1) {
> +        return false;
> +      }
> +    }
> +    return true;
> +  }
> +
> +  updateToolboxTabs(toolboxWidth) {
> +    if (!this.shouldUpdateToolboxTabs(toolboxWidth)) {
> +      return;
> +    }
> +    let overflowtabs = this.calculateOverflowedTabs();
> +    if (overflowtabs) {
> +      this.setState({
> +        overflowedTabIds: overflowtabs
> +      });
> +
> +      // Refresh the tab width caches.
> +      let displayedWidth;
> +      let _nextToolTabSize;
> +      this._registeredToolTabsWidthMap.forEach((val, key, map) => {
> +        if (!overflowtabs.includes(key)) {
> +          displayedWidth += val;
> +        } else if (!_nextToolTabSize) {
> +          _nextToolTabSize = val;
> +        }
> +      });
> +      this._sumToolTabsWidth = displayedWidth;
> +      this._nextToolTabSize = _nextToolTabSize;
> +    }
> +  }

From previous review: I still think that on resize we should only call 

  let overflowtabs = this.calculateOverflowedTabs();
  if (overflowtabs) {
    this.setState({
      overflowedTabIds: overflowtabs
    });
  }
  
Performance wise, what is really costly here is to access the DOM, compute styles etc... Here the optimization only concerns looping on small arrays and doing basic operations. Happy to discuss this in more details if you have profiles showing that this optimization is needed.
Attachment #8962556 - Flags: review?(jdescottes)
Comment hidden (mozreview-request)
(Assignee)

Comment 49

a year ago
mozreview-review-reply
Comment on attachment 8962556 [details]
Bug 1442531 - Part 1. Make chevron button of devtool to be exclusive and apply the photon design.

https://reviewboard.mozilla.org/r/231360/#review239586

Thank you for taking your time to review this.

> I would like to see profiles highlighting the gains for this performance improvement, see my comment on the resizeHandler method.
> 
> Here are two profiles I made:
> - with the optimization (+ a fix for infinite recursion); https://perfht.ml/2Jm5l0G
> - without the optimization https://perfht.ml/2Jns6B9
> 
> My scenario was:
> - enable all possible tools
> - increase window size to show everything
> - start profiling
> - resize to smallest width
> - resize to original width
> - stop profiling
> 
> I don't see a real difference between the two profiles.

I discussed with julian over the IRC, and I dropped this cached variable(i.e. _sumToolTabsWidth / _nextToolTabSize) since the num of tools is limited and not big. As julian said in comment 47, this values doesn't affect the performance.

> From previous review: I don't think this is needed anymore. If it is, we should add a comment explaining why.

Yes, it is not needed. I dropped this.

> Can we extract all of this to a dedicated method, since it's called from componentDidMount, componentDidUpdate (and most likely from the resize handler)

OK. I created the new function for this.

> If the toolbox width (ie maxWidth) is lower than toolIdWidth (ie the width of the select tool tab), this goes into infinite recursion and freezes firefox. A loop approach would be less prone to this kind of issues.

I changed this logic by using for-loop.

> Here we read the current state to know if there is a chevron. This is wrong because we should only add a chevron if the next state will have overflow tabs.
> 
> For now can we just always add the chevron width, and improve this in a follow-up bug ?

I got it now. For example, ToolboxTabs can't distinguish that should display chevron or not in this step when mounting the component. So I added the code of considering chevron when detecting the overflowed tab.

> From previous review: I still think that on resize we should only call 
> 
>   let overflowtabs = this.calculateOverflowedTabs();
>   if (overflowtabs) {
>     this.setState({
>       overflowedTabIds: overflowtabs
>     });
>   }
>   
> Performance wise, what is really costly here is to access the DOM, compute styles etc... Here the optimization only concerns looping on small arrays and doing basic operations. Happy to discuss this in more details if you have profiles showing that this optimization is needed.

OK. I made event handler be simple.

Comment 50

a year ago
mozreview-review
Comment on attachment 8962556 [details]
Bug 1442531 - Part 1. Make chevron button of devtool to be exclusive and apply the photon design.

https://reviewboard.mozilla.org/r/231360/#review240042

Thanks a lot for the update, it looks much better. I still have a few comments, with edge cases that are not covered by the current implementation.
I am just sending a review for toolbox-tabs.js. I don't expect the changes to be significant anymore, so I will have a look at the other files (especially the tests) later today.

::: devtools/client/framework/components/toolbox-tabs.js:70
(Diff revision 5)
> -  removeFlowEvents() {
> -    let node = findDOMNode(this);
> -    if (node) {
> -      node.removeEventListener("overflow", this.onOverflow);
> -      node.removeEventListener("underflow", this.onUnderflow);
> +  /**
> +   * Update cached tool tab width and overflowed tab, then update the state of
> +   * this component.
> +   */
> +  updateToolboxTabs(prevProps, nextProps) {
> +    this.updateCachedToolTabsWidthMap();
> +    this.updateOverflowedTabs();
> -    }
> +  }

I made two conflicting comments in my last review. Since updateOverflowedTabs now updates the state, I think there is no need to have just the 2 lines isolated in a method anymore. You can remove this method and put the two lines everywhere you used to call it. Sorry about that!

::: devtools/client/framework/components/toolbox-tabs.js:106
(Diff revision 5)
> +    let nextPanels = nextProps.panelDefinitions.map(def => def.id);
> +    return !this.equalToolIdArray(prevPanels, nextPanels);
>    }
>  
>    /**
> -   * Render all of the tabs, based on the panel definitions and builds out
> +   * Updatae the Map of tool id and tool tab width.

nit: Updatae -> Update

::: devtools/client/framework/components/toolbox-tabs.js:129
(Diff revision 5)
> -      highlightedTools,
> -      panelDefinition,
> -      selectTool,
> -    }));
> +  updateOverflowedTabs() {
> +    let node = findDOMNode(this);
> +    const toolboxWidth = parseInt(getComputedStyle(node).width, 10);
> +    let { currentToolId } = this.props;
> +    let enabledTabs = this.props.panelDefinitions.map(def => def.id);
> +    let sumWidth = this.state.overflowedTabIds.length > 0 ? CHEVRON_BUTTON_WIDTH

The current approach is slightly wrong. 

When only one tool is hidden (this.state.overflowedTabIds.length > 0), this code will assume that the chevron will be displayed. If you try to resize the window to successively show & overflow the last tool, you should see the issue (the chevron appears and disappears for different window widths, it should not)

To fix this we can simplify the logic:
- assume that chevron is not displayed: let sumWidth = 0;
- if we have any overflowed tool (the else if your for loop), check if we need to pop() another tool because of the chevron

::: devtools/client/framework/components/toolbox-tabs.js:143
(Diff revision 5)
> -      div(
> -        {
> -          className: "toolbox-tabs"
> -        },
> -        tabs
> -      ),
> +      } else {
> +        sumWidth -= width;
> +
> +        // After mounting component, state.overflowedTabIds is zero size.
> +        // In this case, ToolboxTabs doesn't consider the chevron size.
> +        if (this.state.overflowedTabIds.length === 0 &&

Related to my previous comment, this if should only check

if ((sumWidth + CHEVRON_BUTTON_WIDTH) > toolboxWidth)

The previous state is not important, what is important is the information we are calculating right now.

::: devtools/client/framework/components/toolbox-tabs.js:145
(Diff revision 5)
> -          className: "toolbox-tabs"
> -        },
> -        tabs
> -      ),
> -      this.state.overflow ? renderAllToolsButton(this.props) : null
> -    );
> +
> +        // After mounting component, state.overflowedTabIds is zero size.
> +        // In this case, ToolboxTabs doesn't consider the chevron size.
> +        if (this.state.overflowedTabIds.length === 0 &&
> +            (sumWidth + CHEVRON_BUTTON_WIDTH) > toolboxWidth) {
> +          sumWidth -= this._cachedToolTabsWidthMap.get(visibleTabs.pop());

Can we move visibleTabs.pop() to its own line? It is important to make clear that we are removing a tool from the array here.

::: devtools/client/framework/components/toolbox-tabs.js:151
(Diff revision 5)
> -module.exports = ToolboxTabs;
> +    if (visibleTabs.length === 0) {
> +      visibleTabs = [currentToolId];
> +    }

We should handle this case in the next if block, because we also need to check that currentToolId is in enabledTabs (see my last comment for more details)

::: devtools/client/framework/components/toolbox-tabs.js:160
(Diff revision 5)
> +    if (!visibleTabs.includes(currentToolId) &&
> +        enabledTabs.includes(currentToolId)) {
> +      let selectedToolWidth = this._cachedToolTabsWidthMap.get(currentToolId);
> +      while (visibleTabs.length >= 1) {
> +        let lastToolWidth =
> +            this._cachedToolTabsWidthMap.get(visibleTabs.pop());

Same comment: can we move visibleTabs.pop() in its own line:

let lastTool = visibleTabs.pop();
let lastToolWidth = this._cachedToolTabsWidthMap.get(lastTool);

::: devtools/client/framework/components/toolbox-tabs.js:163
(Diff revision 5)
> +        if ((sumWidth + selectedToolWidth) < toolboxWidth) {
> +          visibleTabs.push(currentToolId);
> +          break;
> +        }

I think we always want to show the current tool, even if the chevron button slightly overflows it. 

With the current approach there are edge cases where nothing might be displayed: if the current tool tab is bigger than the first tool (eg. "Inspector" vs "Accessiblity" or "Performance"). In this case when you resize the window to the smallest width possible you will only see the chevron.

The logic for this loop should be:

if (!visibleTabs.includes(currentToolId) && enabledTabs.includes(currentToolId)) {
  // get selected tool with
  while sumWidth + currentToolWidth is too big _and_ visibleTabs is not empty {
    // remove last tool and update sumWidth
  }
  // push the current tool in visibleTabs in _any_ case
}

This way you no longer need the previous if block, and the edge case I mentioned is now covered.
Attachment #8962556 - Flags: review?(jdescottes)
Comment hidden (mozreview-request)
(Assignee)

Comment 52

a year ago
mozreview-review-reply
Comment on attachment 8962556 [details]
Bug 1442531 - Part 1. Make chevron button of devtool to be exclusive and apply the photon design.

https://reviewboard.mozilla.org/r/231360/#review240042

Thank you. I updated this patch.
My patch was so complicated, but this will be simple thanks to your review.

> The current approach is slightly wrong. 
> 
> When only one tool is hidden (this.state.overflowedTabIds.length > 0), this code will assume that the chevron will be displayed. If you try to resize the window to successively show & overflow the last tool, you should see the issue (the chevron appears and disappears for different window widths, it should not)
> 
> To fix this we can simplify the logic:
> - assume that chevron is not displayed: let sumWidth = 0;
> - if we have any overflowed tool (the else if your for loop), check if we need to pop() another tool because of the chevron

I assumed that ToolboxTabs can distinguish that Chevron will be displayed or not in this step, however, ToolboxTabs couldn't calculate correctly this condition after mounting component. So your logic is simpler than before it.

Comment 53

a year ago
mozreview-review
Comment on attachment 8962556 [details]
Bug 1442531 - Part 1. Make chevron button of devtool to be exclusive and apply the photon design.

https://reviewboard.mozilla.org/r/231360/#review240780

This looks great! I have a bunch of nits and some comments but they should be very straightforward. 
Some open questions for the tests but they can be addressed in follow up bugs.

I sent the patch to try https://treeherder.mozilla.org/#/jobs?repo=try&revision=e704f0e7d680e3a5408ee3bfd6f9913ea03b611e 
It looks like browser_toolbox_options.js still fails on some platforms. See my comment about this test.

The next version should be ready to land I think, just holding on the r+ for the test failure here. Almost there :)

::: devtools/client/framework/components/toolbox-tabs.js:106
(Diff revision 6)
> -  render() {
> -    let {
> -      currentToolId,
> -      focusButton,
> -      focusedButton,
> -      highlightedTools,
> +  updateCachedToolTabsWidthMap() {
> +    let thisNode = findDOMNode(this);
> +    for (let tab of thisNode.querySelectorAll(".devtools-tab")) {
> +      let tabId = tab.id.replace("toolbox-tab-", "");
> +      if (!this._cachedToolTabsWidthMap.has(tabId)) {
> +        let cs = getComputedStyle(tab);

nit: Maybe we should use getBoundingClientRect().width here? This way if we ever have padding/borders modifying the width of the tabs, this would still work. Could be handled in a follow up bug, maybe this needs more research.

::: devtools/client/framework/components/toolbox-tabs.js:114
(Diff revision 6)
> -    } = this.props;
> +    }
> +  }
>  
> -    let tabs = panelDefinitions.map(panelDefinition => ToolboxTab({
> -      key: panelDefinition.id,
> -      currentToolId,
> +  /**
> +   * Update the overflowed tab array from currently displayed tool tab.
> +   * If calculated result is same to a current overflowed tab array, this

nit: is same to a current -> is the same as the current

::: devtools/client/framework/components/toolbox-tabs.js:132
(Diff revision 6)
> -      {
> -        className: "toolbox-tabs-wrapper"
> -      },
> -      div(
> -        {
> -          className: "toolbox-tabs"
> +      sumWidth += width;
> +      if (sumWidth <= toolboxWidth) {
> +        visibleTabs.push(id);
> +      } else {
> +        sumWidth -= width;
> +

At this point we know we will display the chevron so we can immediately do:
sumWidth = sumWidth + CHEVRON_BUTTON_WIDTH;

This will be useful when we have to insert the selected tool back in the visible tools.

::: devtools/client/framework/components/toolbox-tabs.js:148
(Diff revision 6)
> +    // toolbox.
> +    if (!visibleTabs.includes(currentToolId) &&
> +        enabledTabs.includes(currentToolId)) {
> +      let selectedToolWidth = this._cachedToolTabsWidthMap.get(currentToolId);
> +      while ((sumWidth + selectedToolWidth) > toolboxWidth &&
> +             visibleTabs.length >= 0) {

This should be visibleTabs.length > 0 (strictly greater than). If visibleTabs.length === 0 you will get undefined with visibleTabs.pop()

::: devtools/client/framework/components/toolbox-tabs.js:157
(Diff revision 6)
> +      }
> +      visibleTabs.push(currentToolId);
> +    }
> +
> +    if (visibleTabs.length === 0) {
> +      visibleTabs = [currentToolId];

At first I thought we should remove this, but this is actually a very interesting case!

At this point, is visibleTabs.length === 0, then it means that the currentTool is not in "enabledTabs" (otherwise it would have been added by the if () block just above).

So we cannot do `visibleTabs = [currentToolId];`, because the current tool is not at tab, but we could do 

  visibleTabs = [enabledTabs[0]];

This could happen if:
- user is on the "settings" tool (which is not a tab)
- tab for first tool is bigger than available width
  ("Inspector" is probably much longer in some locales)

::: devtools/client/framework/components/toolbox-tabs.js:186
(Diff revision 6)
> -    toolbox,
> +      toolbox,
> -    L10N,
> +      L10N,
> -  } = props;
> +    } = this.props;
>  
> -  return button({
> +    return button({
> -    className: "all-tools-menu all-tabs-menu",
> +      className: "devtools-button tools-chevronmenu",

could we use tools-chevron-menu instead of tools-chevronmenu ? This way we are consistent with tools-chevron-menupopup

::: devtools/client/framework/components/toolbox-tabs.js:220
(Diff revision 6)
> -  });
> +    });
> -}
> +  }
> +
> +  /**
> +   * Render all of the tabs, based on the panel definitions and builds out
> +   * a toolbox tab for each of them. Will render an all-tabs button if the

nit: "all-tabs button" should probably be chevron button now

::: devtools/client/framework/test/browser_toolbox_options.js:24
(Diff revision 6)
>    registerNewTool();
>    let tab = await addTab(URL);
>    let target = TargetFactory.forTab(tab);
>    toolbox = await gDevTools.showToolbox(target);
> +
> +  info("In order to ensure display all tab menu, increase the width of devtool.");

nit: all tab menu -> "all tabs" or "all tools"
nit: increase the width of devtool -> "increase the width of the toolbox"

::: devtools/client/framework/test/browser_toolbox_options.js:27
(Diff revision 6)
>    toolbox = await gDevTools.showToolbox(target);
> +
> +  info("In order to ensure display all tab menu, increase the width of devtool.");
> +  let hostWindow = toolbox.win.parent;
> +  let onResize = once(hostWindow, "resize");
> +  hostWindow.resizeTo(1300, hostWindow.outerHeight);

We should restore the original size in the cleanup() method.

But I realize that 1300 is not even enough for this test, which keeps failing with "FAIL Tab added back for storage - ". It passes with a bigger width (for instance I tried with 1700px) but I am afraid this might be too big for some platforms on CI, and in the longterm it does not scale. Ideally, to check if a tool is registered we should check the tabs and the chevron menu (same to check if it is unregistered). 

Here, can you try to find a bigger width to make the test pass, and log a follow up bug so that we can fix this.

::: devtools/client/framework/test/browser_toolbox_toolbar_overflow.js:58
(Diff revision 6)
> +
> +  info("Open the tools-chevron-menupopup and verify that the inspector button is checked");
> +  let menuPopup = await openChevronMenu(toolbox);
>  
> -  info("Wait until the all tools button is available");
> -  await waitUntil(() => toolbox.doc.querySelector(".all-tools-menu"));
> +  let inspectorButton = toolbox.doc.querySelector("#tools-chevron-menupopup-inspector");
> +  ok(!inspectorButton, "A chevron menu doesn't have the inspector button.");

nit: A chevron menu -> The chevron menu (same comments for lines below)

::: devtools/client/framework/test/browser_toolbox_toolbar_reorder_by_width.js:13
(Diff revision 6)
> +
> +// This test will:
> +//
> +// * Confirm that currently selected button to access tools will not hide due to overflow.
> +//   In this case, a button which is located on the left of a currently selected will hide.
> +// * Confirm that a button to access tool will hide when registering a new panel.

It would be great to clearly separate the two tests. You can use two separated add_task() or create a second dedicated test file.

::: devtools/client/framework/test/browser_toolbox_toolbar_reorder_by_width.js:24
(Diff revision 6)
> +
> +add_task(async function () {
> +  let tab = await addTab("about:blank");
> +
> +  info("Open devtools on the Storage in a sidebar.");
> +  let toolbox = await openToolboxForTab(tab, "storage", Toolbox.HostType.SIDE);

You should use either WINDOW or BOTTOM here. With SIDE, resizing the window doesn't change the width of devtools.

::: devtools/client/framework/test/browser_toolbox_toolbar_reorder_by_width.js:30
(Diff revision 6)
> +
> +  let hostWindow = toolbox.win.parent;
> +  let originalWidth = hostWindow.outerWidth;
> +  let originalHeight = hostWindow.outerHeight;
> +
> +  info("Resize devtools window to a width that should not trigger any overflow");

nit: "that should not trigger any overflow" -> "that should trigger an overflow"

::: devtools/client/framework/test/browser_toolbox_toolbar_reorder_by_width.js:32
(Diff revision 6)
> +  let originalWidth = hostWindow.outerWidth;
> +  let originalHeight = hostWindow.outerHeight;
> +
> +  info("Resize devtools window to a width that should not trigger any overflow");
> +  let onResize = once(hostWindow, "resize");
> +  hostWindow.resizeTo(800, originalHeight);

Ideally, this test should show that "storage" is displayed instead of another tool. Here we simply know that for 800px width "storage" is displayed. 

After we land this Bug, can you log another one so that we can add some more coverage?

::: devtools/client/framework/test/browser_toolbox_toolbar_reorder_by_width.js:41
(Diff revision 6)
> +  await waitUntil(() => toolbox.doc.querySelector(".tools-chevronmenu"));
> +
> +  let chevronMenuButton = toolbox.doc.querySelector(".tools-chevronmenu");
> +  ok(chevronMenuButton, "The chevron menu button is displayed");
> +
> +  info("Confirm that selected menu doesn't hidden.");

nit: We should call this a "tab" rather than a "menu" 
nit: doesn't hidden -> is not hidden

::: devtools/client/framework/test/browser_toolbox_toolbar_reorder_by_width.js:43
(Diff revision 6)
> +  let chevronMenuButton = toolbox.doc.querySelector(".tools-chevronmenu");
> +  ok(chevronMenuButton, "The chevron menu button is displayed");
> +
> +  info("Confirm that selected menu doesn't hidden.");
> +  let storageButton = toolbox.doc.querySelector("#toolbox-tab-storage");
> +  ok(storageButton, "A chevron menu has the storage button.");

nit: "storage" is not supposed to be in the menu, so maybe "The storage tab is rendered" or something like that.

::: devtools/client/framework/test/browser_toolbox_toolbar_reorder_by_width.js:59
(Diff revision 6)
> +  await onRegistered;
> +
> +  info("Open the chevron menu button.");
> +  let popup = await openChevronMenu(toolbox);
> +
> +  info("A registered new tool menu should be in the chevron menu.");

Same comment about renaming "menu" to "tab" in this context (for "tool menu", not for "chevron menu")

::: devtools/client/framework/test/browser_toolbox_toolbar_reorder_by_width.js:61
(Diff revision 6)
> +  info("Open the chevron menu button.");
> +  let popup = await openChevronMenu(toolbox);
> +
> +  info("A registered new tool menu should be in the chevron menu.");
> +  let testToolsButton = toolbox.doc.querySelector("#tools-chevron-menupopup-test-tools");
> +  ok(testToolsButton, "A chevron menu has a registered new tool button.");

nit: A chevron menu -> The chevron menu

::: devtools/client/framework/test/browser_toolbox_toolbar_reorder_by_width.js:78
(Diff revision 6)
> +  info("Open the chevron menu button.");
> +  popup = await openChevronMenu(toolbox);
> +
> +  info("A unregistered new tool menu should not be in the chevron menu.");
> +  testToolsButton = toolbox.doc.querySelector("#tools-chevron-menupopup-test-tools");
> +  ok(!testToolsButton, "A chevron menu doesn't have a unregistered new tool button.");

nit: A chevron menu -> The chevron menu

::: devtools/client/framework/test/browser_toolbox_toolbar_reorder_by_width.js:91
(Diff revision 6)
> +  onResize = once(hostWindow, "resize");
> +  hostWindow.resizeTo(originalWidth, originalHeight);
> +  await onResize;
> +});
> +
> +async function openChevronMenu(toolbox) {

Since this method is used in both test files now, you could define it in the head.js for the same folder. This is a good candidate for a follow up bug.
Attachment #8962556 - Flags: review?(jdescottes)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 56

a year ago
mozreview-review-reply
Comment on attachment 8962556 [details]
Bug 1442531 - Part 1. Make chevron button of devtool to be exclusive and apply the photon design.

https://reviewboard.mozilla.org/r/231360/#review240780

Thank you so much!
I updated the patch, and added second patch of test.

> nit: Maybe we should use getBoundingClientRect().width here? This way if we ever have padding/borders modifying the width of the tabs, this would still work. Could be handled in a follow up bug, maybe this needs more research.

OK. I'll file the bug.

> At this point we know we will display the chevron so we can immediately do:
> sumWidth = sumWidth + CHEVRON_BUTTON_WIDTH;
> 
> This will be useful when we have to insert the selected tool back in the visible tools.

Yes. And I think that above subtraction is not needed. I added target tool width to sumWidth before this if condition, but I think it have better to move this addition to the if statement. I changed this logic.

> We should restore the original size in the cleanup() method.
> 
> But I realize that 1300 is not even enough for this test, which keeps failing with "FAIL Tab added back for storage - ". It passes with a bigger width (for instance I tried with 1700px) but I am afraid this might be too big for some platforms on CI, and in the longterm it does not scale. Ideally, to check if a tool is registered we should check the tabs and the chevron menu (same to check if it is unregistered). 
> 
> Here, can you try to find a bigger width to make the test pass, and log a follow up bug so that we can fix this.

Sorry, Now I inspected this test, the size of fitting toolbox is 1313px. I just changed to 1350px. And I added second patch which is checking tool tabs without resizing the window.

> Since this method is used in both test files now, you could define it in the head.js for the same folder. This is a good candidate for a follow up bug.

OK. I moved this function to head.js.
Thanks.

Comment 57

a year ago
mozreview-review
Comment on attachment 8966489 [details]
Bug 1442531 - Part 2. Check tool tab in tools menu as well.

https://reviewboard.mozilla.org/r/235190/#review241156

Looks good to me! This way the test should be much more stable.

::: devtools/client/framework/test/browser_toolbox_options.js:450
(Diff revision 1)
>      default:
>        throw new Error("Unknown type");
>    }
>  }
>  
> -async function cleanup(win, winWidth) {
> +async function exploreToolbox(toolId) {

nit: exploreToolbox is a bit vague as a name. Maybe getButtonForToolId/findButtonForToolId. "button" is slightly inaccurate because we can either return a button or a menu-item, but that's acceptable. 

Can we add a small JS doc for this function to explain the purpose (eg "Find the button for the provided tool id, either in the toolbox tabs or in the chevron menu in case the tab was hidden by the overflow")
Attachment #8966489 - Flags: review?(jdescottes) → review+

Comment 58

a year ago
mozreview-review
Comment on attachment 8962556 [details]
Bug 1442531 - Part 1. Make chevron button of devtool to be exclusive and apply the photon design.

https://reviewboard.mozilla.org/r/231360/#review241148

Great work Mantaroh, this looks good to me! Few nits and one small issue with "import-globals-from", but no need for other round of reviews from me here.
Thanks for being patient with the reviews :) I pushed the changes to try https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0a827768b28df9e698091e41eb54bd5ba12c677

I would like to get a final check from Victoria for UX review before we land.

::: devtools/client/framework/components/toolbox-tabs.js:73
(Diff revision 7)
> -      node.addEventListener("underflow", this.onUnderflow);
>      }
>    }
>  
> -  removeFlowEvents() {
> -    let node = findDOMNode(this);
> +  /**
> +   * Return a specified two panel id array is same or not.

nit: Maybe rephrase to "Check if two arrays of ids are the same or not."

::: devtools/client/framework/test/browser_toolbox_options.js:24
(Diff revision 7)
>    registerNewTool();
>    let tab = await addTab(URL);
>    let target = TargetFactory.forTab(tab);
>    toolbox = await gDevTools.showToolbox(target);
> +
> +  info("In order to ensure display the chevron menu, increase the width of " +

I think this is the opposite? We increase the width in order to show all tabs (or hide the chevron menu)

::: devtools/client/framework/test/browser_toolbox_toolbar_reorder_by_width.js:6
(Diff revision 7)
> +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +/* vim: set ft=javascript ts=2 et sw=2 tw=80: */
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +/* import-globals-from shared-head.js */

This is not needed and would only work if there was a "shared-head.js" file in the same folder.

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

The info should be generic here "Waiting for the window to be resized" (since this method will be called both to trigger overflows or to restore the original size)
Attachment #8962556 - Flags: review?(jdescottes) → review+
Comment on attachment 8962556 [details]
Bug 1442531 - Part 1. Make chevron button of devtool to be exclusive and apply the photon design.

Victoria: can you check the UI and UX for this before we proceed? There should be a build for OSX at https://queue.taskcluster.net/v1/task/TVAGMr4bSeONUiDZBMRtGg/runs/0/artifacts/public/build/target.dmg 

We can make a small gif/video if the DMG doesn't work as expected.
Flags: needinfo?(victoria)
Attachment #8962556 - Flags: ui-review?(victoria)

Comment 60

a year ago
mozreview-review-reply
Comment on attachment 8962556 [details]
Bug 1442531 - Part 1. Make chevron button of devtool to be exclusive and apply the photon design.

https://reviewboard.mozilla.org/r/231360/#review241148

> I think this is the opposite? We increase the width in order to show all tabs (or hide the chevron menu)

Ignore this since the code is removed in the second changeset.

Comment 61

a year ago
mozreview-review
Comment on attachment 8962556 [details]
Bug 1442531 - Part 1. Make chevron button of devtool to be exclusive and apply the photon design.

https://reviewboard.mozilla.org/r/231360/#review241164

A few eslint failures I forgot to mention!

::: devtools/client/framework/test/browser_toolbox_toolbar_reorder_by_width.js:20
(Diff revision 7)
> +// Note that this test is based on the tab ordinal is fixed.
> +// i.e. After changed by Bug 1226272, this test might fail.
> +
> +let { Toolbox } = require("devtools/client/framework/toolbox");
> +
> +add_task(async function () {

eslint: remove space before parenthesis

::: devtools/client/framework/test/browser_toolbox_toolbar_reorder_by_width.js:42
(Diff revision 7)
> +  ok(storageButton, "The storage tab is on toolbox.");
> +
> +  await resizeWindow(toolbox, originalWidth, originalHeight);
> +});
> +
> +add_task(async function () {

eslint: remove space before parenthesis

::: devtools/client/framework/test/browser_toolbox_toolbar_reorder_by_width.js:99
(Diff revision 7)
> +async function resizeWindow(toolbox, width, height) {
> +  let hostWindow = toolbox.win.parent;
> +  let originalWidth = hostWindow.outerWidth;
> +  let originalHeight = hostWindow.outerHeight;
> +  let toWidth = width || originalWidth;
> +  let toHeight = height || originalHeight

eslint: missing semi colon
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 64

a year ago
mozreview-review-reply
Comment on attachment 8962556 [details]
Bug 1442531 - Part 1. Make chevron button of devtool to be exclusive and apply the photon design.

https://reviewboard.mozilla.org/r/231360/#review241164

I'm sorry for bothering you again 
I fixed it.
(Assignee)

Comment 65

a year ago
mozreview-review-reply
Comment on attachment 8966489 [details]
Bug 1442531 - Part 2. Check tool tab in tools menu as well.

https://reviewboard.mozilla.org/r/235190/#review241156

Thank you so much.
I fixed patch 1 and patch 2, then the try is as follow:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c27442b4652382bd13e899618e33f64236d65b19

I'll wait to land it until the victoria's review.

> nit: exploreToolbox is a bit vague as a name. Maybe getButtonForToolId/findButtonForToolId. "button" is slightly inaccurate because we can either return a button or a menu-item, but that's acceptable. 
> 
> Can we add a small JS doc for this function to explain the purpose (eg "Find the button for the provided tool id, either in the toolbox tabs or in the chevron menu in case the tab was hidden by the overflow")

Added the JS doc and I changed this function name to "lookupButtonForToolId".
Thanks!
(Assignee)

Comment 66

a year ago
(In reply to Julian Descottes [:jdescottes][:julian] from comment #59)
> Comment on attachment 8962556 [details]
> Bug 1442531 - Part 1. Make chevron button of devtool to be exclusive and
> apply the photon design.
> 
> Victoria: can you check the UI and UX for this before we proceed? There
> should be a build for OSX at
> https://queue.taskcluster.net/v1/task/TVAGMr4bSeONUiDZBMRtGg/runs/0/
> artifacts/public/build/target.dmg 
> 
> We can make a small gif/video if the DMG doesn't work as expected.

I attached the screen capture of this behavior. Please see this video if you want to glance at this behavioir.
Thanks Mantaroh for the video! I also checked out the try build to verify a few things, thanks Julian! This is looking great - I love the functionality of the selected tab disappearing last.

I just have a couple visual tweaks for the chevron icon: 

1. it looks a bit too big - if you could change the background-size from 16px to 12px it will fit among the small DevTools icons better

2. It would be great to style the color as Gray 90 opacity .8 (we figured out in a different bug (https://bugzilla.mozilla.org/show_bug.cgi?id=1444301#c62) that these new chunky photon icons like the meatball/close button look best this way)
Here's how the chevron icon should look after making the changes I mentioned in my last comment
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 71

a year ago
Thanks, victoria.

(In reply to Victoria Wang [:victoria] from comment #67)
> Thanks Mantaroh for the video! I also checked out the try build to verify a
> few things, thanks Julian! This is looking great - I love the functionality
> of the selected tab disappearing last.
> 
> I just have a couple visual tweaks for the chevron icon: 
> 
> 1. it looks a bit too big - if you could change the background-size from
> 16px to 12px it will fit among the small DevTools icons better
> 

My patch specified the background-size:cover style. So I removed it.

> 2. It would be great to style the color as Gray 90 opacity .8 (we figured
> out in a different bug
> (https://bugzilla.mozilla.org/show_bug.cgi?id=1444301#c62) that these new
> chunky photon icons like the meatball/close button look best this way)

OK. I apply this style to chevron button:
https://screenshots.firefox.com/0Dv4bFFSZ76xbCnW/null
That looks great - thanks Mantaroh!
Attachment #8962556 - Flags: ui-review?(victoria) → ui-review+

Comment 73

a year ago
mozreview-review
Comment on attachment 8962556 [details]
Bug 1442531 - Part 1. Make chevron button of devtool to be exclusive and apply the photon design.

https://reviewboard.mozilla.org/r/231360/#review241954

Still looks good to me :) Two additional comments to address.

After that we can push this to autoland. I don't know if you have the permissions to do so?
If you don't, let me know I'll push it for you.

Kicked off two try runs:
- current patch https://treeherder.mozilla.org/#/jobs?repo=try&revision=24f57c44e8df4ecd89c11bf0910238484da8819b
- current patch + suggested test fix https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ae8f207da325952188dbe232620982a62ba07c0

::: devtools/client/framework/test/browser_toolbox_toolbar_overflow.js:37
(Diff revision 9)
> +
>    info("Resize devtools window to a width that should not trigger any overflow");
>    let onResize = once(hostWindow, "resize");
> -  hostWindow.resizeTo(640, 300);
> +  hostWindow.resizeTo(1350, 300);
>    await onResize;
> +  await raf();

It looks like this test is failing frequently on TRY in debug builds. If we land like that we will most likely get backed out. 
Just waiting one request animation frame seemed a bit fragile but I hoped to moved this to a follow up.
Ideally, the toolbar should fire an event that we could catch in the test. But I am not sure when to fire this. In the meantime can we add a 

  waitUntil(() => !toolbox.doc.querySelector(".tools-chevron-menu"));

::: devtools/client/themes/toolbox.css:47
(Diff revision 9)
>  }
>  
> +.tools-chevron-menu::before {
> +  top: 0;
> +  offset-inline-end: 0;
> +  width: 12px;

This is not applied because the rule from .devtools-button:empty::before in common.css has higher priority.

You could merge this with your #devtools-chevron-menu-button::before around line 150. It targets the same element, but has higher priority than the one in common.css 

This new icon is no longer centered. In theory I guess using height: 12px; width: 12px; should fix it, but it still seems off (on my machine). height: 13px; looks better (still on my machine). We can use 12px for now and have a follow up to fix the centering.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 76

a year ago
mozreview-review-reply
Comment on attachment 8962556 [details]
Bug 1442531 - Part 1. Make chevron button of devtool to be exclusive and apply the photon design.

https://reviewboard.mozilla.org/r/231360/#review241954

Thanks a lot, Julian!

I changed the test without requestAnimationFrame and requestIdleCallback. And I think we can use "background-poisition:center" for resolving the centering behavior.

> This is not applied because the rule from .devtools-button:empty::before in common.css has higher priority.
> 
> You could merge this with your #devtools-chevron-menu-button::before around line 150. It targets the same element, but has higher priority than the one in common.css 
> 
> This new icon is no longer centered. In theory I guess using height: 12px; width: 12px; should fix it, but it still seems off (on my machine). height: 13px; looks better (still on my machine). We can use 12px for now and have a follow up to fix the centering.

I confirmed this behavior. The size of other devtools button is 16px, so it will be fitting to toolbox tabs height. However, this chevron icon is 12px, so I think that width and height of this pseudo element should be 16px and background should be centering. I changed this style to using 16px size and "background-position:center".

Screenshot:
https://screenshots.firefox.com/AuiuqjXmijsiKv5X/screenshots.firefox.com

Comment 77

a year ago
mozreview-review
Comment on attachment 8966489 [details]
Bug 1442531 - Part 2. Check tool tab in tools menu as well.

https://reviewboard.mozilla.org/r/235190/#review242150


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/framework/components/toolbox-tabs.js:190
(Diff revision 4)
>      return button({
> +      id: "devtools-chevron-menu-button",
>        className: "devtools-button tools-chevron-menu",
>        tabIndex: -1,
>        title: L10N.getStr("toolbox.allToolsButton.tooltip"),
>        id: "tools-chevron-menu-button",

Error: Duplicate key 'id'. [eslint: no-dupe-keys]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 82

a year ago
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #80)
> Comment on attachment 8962556 [details]
> Bug 1442531 - Part 1. Make chevron button of devtool to be exclusive and
> apply the photon design.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/231360/diff/11-12/

Updated the patch in order to test fail. This changes will wait for all buttons are displayed when maximizing the window.
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6db21902dc5ac91c3e045d1fb40e2ee4d94b7cc
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 85

a year ago
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #83)
> Comment on attachment 8962556 [details]
> Bug 1442531 - Part 1. Make chevron button of devtool to be exclusive and
> apply the photon design.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/231360/diff/12-13/

Sorry, a previous patch is my mistake.
I should be changed "toolbox.panelDefinitions.length !=== toolbox.doc.querySelectorAll("...").length".

I updated the test:
 * Comparing the num of panel definitions and the displayed tool tab in order to wait for that all tabs are displayed or not.
 * Using the HostType.BOTTOM for improvements the test performance(In my local and try, to use the bottom dock is little faster than undocked).
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 88

a year ago
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #86)
> Comment on attachment 8962556 [details]
> Bug 1442531 - Part 1. Make chevron button of devtool to be exclusive and
> apply the photon design.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/231360/diff/13-14/

fixed typo.

All target tests are succeeded.(dt5)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c82951ff3bf92a4f887a343a552e80192ecc5c58&group_state=expanded

Comment 89

a year ago
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ee1ecd82bda4
Part 1. Make chevron button of devtool to be exclusive and apply the photon design. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/519b64233026
Part 2. Check tool tab in tools menu as well. r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/ee1ecd82bda4
https://hg.mozilla.org/mozilla-central/rev/519b64233026
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
I have reproduced this bug with Nightly 60.0a1 (2018-03-02) on ManjaroLinux 17.1.8, 64 bit!

The fix is now verified on Latest Nightly.

Build ID 	20180428100112
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:61.0) Gecko/20100101 Firefox/61.0
QA Whiteboard: [bugday-20180425]
I have successfully reproduce this bug Nightly 61.0a1 (2018-03-02) on windows 10(32-bit)

this bug is fix now on Latest Nightly 61.0a1 (2018-04-28) (32-bit)

Build ID : 20180428220114
Mozilla/5.0 (Windows NT 10.0; rv:61.0) Gecko/20100101 Firefox/61.0

[bugday-20180425]
I have reproduced this issue using Firefox  60.0a1 (2018.03.02) 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: 1465941

Updated

10 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.