Closed Bug 1426041 Opened 6 years ago Closed 6 years ago

Fix netmonitor toolbar items layout in Portrait mode

Categories

(DevTools :: Netmonitor, defect, P3)

defect

Tracking

(firefox-esr52 unaffected, firefox57 wontfix, firefox58 wontfix, firefox59 wontfix, firefox60 wontfix, firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: magicp.jp, Assigned: Honza)

References

Details

Attachments

(4 files, 3 obsolete files)

Steps to reproduce:
1. Launch Nightly
2. Open Netmonitor (Ctrl+Shift+E)
3. Dock to side of browser window
4. Switch to each DevTools themes

Actual results:
devtools-toolbar items are wrapped and require much height. Some items overlap with request headers in the Firebug theme.

Expected results:
In portrait mode, Filter buttons are nowrap, checkboxes move to next line and eliminate overlapping. Please find the attached file.
@magicp: what Firefox version the 'expected' screenshots are coming from?

Note that we are planning toolbar redesign in Q1/2018
https://mozilla.invisionapp.com/share/2XEEY0RYA#/screens/263401584_Network_Toolbar

Honza
Flags: needinfo?(magicp.jp)
Priority: -- → P3
(In reply to Jan Honza Odvarko [:Honza] from comment #1)
> @magicp: what Firefox version the 'expected' screenshots are coming from?

This is my design. Sorry for your confusion. 

> Note that we are planning toolbar redesign in Q1/2018
> https://mozilla.invisionapp.com/share/2XEEY0RYA#/screens/263401584_Network_Toolbar

Good design!
Flags: needinfo?(magicp.jp)
(In reply to magicp from comment #2)
> (In reply to Jan Honza Odvarko [:Honza] from comment #1)
> > @magicp: what Firefox version the 'expected' screenshots are coming from?
> 
> This is my design. Sorry for your confusion. 
Ah, I see, looks so real! :-)

Honza
Hi Victoria,

We'd like to work on the new UI layout and fix issues mentioned in this bug.

Is https://mozilla.invisionapp.com/share/2XEEY0RYA#/screens/263401584_Network_Toolbar the latest design or could we get a reference version somewhere?
Flags: needinfo?(victoria)
Cool to see magicp's mockups! We were on the same page basically :). I didn't know the bugzilla one looked so broken though, oof!

Fred: Yep, that's the latest design. Some specs on new colors are in this mockup: https://mozilla.invisionapp.com/share/2XEEY0RYA#/screens/263398480_Console-Network_-_Input_Field_Focus_Ring

(I'm still using this doc as my source of latest truth for all netmonitor/console toolbar changes https://docs.google.com/document/d/1HG2mJi8jNklgUnIMND-iTnoPByI7HnAexmUXG-lyyCI/)
Flags: needinfo?(victoria)
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
Here's the gif for the new toolbar layout that render differently in bottom and side dock mode.

The UX spec does not cover `if there's more width(space)` case, so here are some difference from the spec:

1. netmonitor has a `detail button` at right side, I keep it align right in side-dock mode if there's more space.
2. I expand the searchbox in bottom dock mode when there's more space.

Victoria, is it looks right?

(I will add the separators before send review)
Flags: needinfo?(victoria)
Attached image 2-columns-layout.gif
new layout adds the separators and will change to 2 columns when screen size < 920px (the width is chosen because it can contain all text in en), maybe we should pick a larger one (such as 960px) for language tolerance.
Attachment #8939726 - Attachment is obsolete: true
Victoria, current patch followed https://mozilla.invisionapp.com/share/2XEEY0RYA#/screens/263401584_Network_Toolbar version,

but I also saw different variants (Network Vertical) in your doc https://docs.google.com/document/d/1HG2mJi8jNklgUnIMND-iTnoPByI7HnAexmUXG-lyyCI/ could you point which is the preferred one?


For webconsole, current webconsole has a filter icon button to toggle those filter buttons in a separate line. According to the doc, does the webconsole design heads to remove the filter icon button and use something like current network monitor does (list all filter buttons on toolbar)?
Comment on attachment 8939721 [details]
Bug 1426041 - rearrange and add 2 columns toolbar layout to support bottom and side dock mode;

https://reviewboard.mozilla.org/r/210028/#review215724

::: devtools/client/netmonitor/src/components/MonitorPanel.js:56
(Diff revision 2)
>  
>    constructor(props) {
>      super(props);
>  
>      this.state = {
> -      isVerticalSpliter: MediaQueryList.matches,
> +      isSingleRow: MediaQuerySingleRow,

this should be `MediaQuerySingleRow.matches`

::: devtools/client/netmonitor/src/components/Toolbar.js:273
(Diff revision 2)
> +/**
> + * Render a separator.
> + */
> +function renderSeparator() {
> +  return span({className: "devtools-separator"});
> +}
> +
> +/**
> + * Render a clear button.
> + */
> +function renderClearButton(clearRequests) {
> +  return button({
> +    className: "devtools-button devtools-clear-icon requests-list-clear-button",
> +    title: TOOLBAR_CLEAR,
> +    onClick: clearRequests,
> +  });
> +}
> +
> +/**
> + * Render a Detail button.
> + */
> +function renderDetailButton(networkDetailsOpen,
> +  networkDetailsToggleDisabled, toggleNetworkDetails) {
> +  // Detail toggle button
> +  let toggleDetailButtonClassList = [
> +    "network-details-panel-toggle",
> +    "devtools-button",
> +  ];
> +
> +  if (!networkDetailsOpen) {
> +    toggleDetailButtonClassList.push("pane-collapsed");
> +  }
> +  let toggleDetailButtonClass = toggleDetailButtonClassList.join(" ");
> +  let toggleDetailButtonTitle = networkDetailsOpen ? COLLAPSE_DETAILS_PANE :
> +    EXPAND_DETAILS_PANE;
> +
> +  return button({
> +    className: toggleDetailButtonClass,
> +    title: toggleDetailButtonTitle,
> +    disabled: networkDetailsToggleDisabled,
> +    tabIndex: "0",
> +    onClick: toggleNetworkDetails,
> +  });
> +}
> +
> +/**
> + * Render a ToggleRecording button.
> + */
> +function renderToggleRecordingButton(recording, toggleRecording) {
> +  // Calcula[te class-list for toggle recording button.
> +  // The button has two states: pause/play.
> +  let toggleRecordingButtonClass = [
> +    "devtools-button",
> +    "requests-list-pause-button",
> +    recording ? "devtools-pause-icon" : "devtools-play-icon",
> +  ].join(" ");
> +
> +  return button({
> +    className: toggleRecordingButtonClass,
> +    title: TOOLBAR_TOGGLE_RECORDING,
> +    onClick: toggleRecording,
> +  });
> +}
> +
> +/**
> + * Render a Persistlog checkbox.
> + */
> +function renderPersistlogCheckbox(persistentLogsEnabled, togglePersistentLogs) {
> +  return label(
> +    {
> +      className: "devtools-checkbox-label",
> +      title: ENABLE_PERSISTENT_LOGS_TOOLTIP,
> +    },
> +    input({
> +      id: "devtools-persistlog-checkbox",
> +      className: "devtools-checkbox",
> +      type: "checkbox",
> +      checked: persistentLogsEnabled,
> +      onChange: togglePersistentLogs,
> +    }),
> +    ENABLE_PERSISTENT_LOGS_LABEL
> +  );
> +}
> +
> +/**

I really like this way to construct reusable components.

For coding style consistency, please do not define new method out of Class component. And remember `module.exports = ...` should be the last item in a file.

::: devtools/client/netmonitor/src/components/Toolbar.js:277
(Diff revision 2)
> +
> +/**
> + * Render a separator.
> + */
> +function renderSeparator() {
> +  return span({className: "devtools-separator"});

nit: adding whitespace between objecgt properties

{ className: "devtools-separator" }

::: devtools/client/netmonitor/src/components/Toolbar.js:284
(Diff revision 2)
> +
> +/**
> + * Render a clear button.
> + */
> +function renderClearButton(clearRequests) {
> +  return button({

nit: wrapping () around multi-lines components.

return (
  button(
    ...
  )
)

::: devtools/client/netmonitor/src/components/Toolbar.js:309
(Diff revision 2)
> +  }
> +  let toggleDetailButtonClass = toggleDetailButtonClassList.join(" ");
> +  let toggleDetailButtonTitle = networkDetailsOpen ? COLLAPSE_DETAILS_PANE :
> +    EXPAND_DETAILS_PANE;
> +
> +  return button({

nit: wrapping () around multi-lines components.

return (
  button(
    ...
  )
)

::: devtools/client/netmonitor/src/components/Toolbar.js:330
(Diff revision 2)
> +    "devtools-button",
> +    "requests-list-pause-button",
> +    recording ? "devtools-pause-icon" : "devtools-play-icon",
> +  ].join(" ");
> +
> +  return button({

nit: wrapping () around multi-lines components.

return (
  button(
    ...
  )
)

::: devtools/client/netmonitor/src/components/Toolbar.js:341
(Diff revision 2)
> +  return label(
> +    {
> +      className: "devtools-checkbox-label",
> +      title: ENABLE_PERSISTENT_LOGS_TOOLTIP,
> +    },

nit: wrapping () around multi-lines components & move { to previous line.

return (
  label({
    className: "devtools-checkbox-label",
    title: DISABLE_CACHE_TOOLTIP,
    ...
  })
)

::: devtools/client/netmonitor/src/components/Toolbar.js:353
(Diff revision 2)
> +      className: "devtools-checkbox",
> +      type: "checkbox",
> +      checked: persistentLogsEnabled,
> +      onChange: togglePersistentLogs,
> +    }),
> +    ENABLE_PERSISTENT_LOGS_LABEL

nit: missing tailing comma

::: devtools/client/netmonitor/src/components/Toolbar.js:361
(Diff revision 2)
> +
> +/**
> + * Render a Cache checkbox.
> + */
> +function renderCacheCheckbox(browserCacheDisabled, toggleBrowserCache) {
> +  return label(

nit: wrapping () around multi-lines components & move { to previous line.

return (
  label({
    className: "devtools-checkbox-label",
    title: DISABLE_CACHE_TOOLTIP,
    ...
  })
)
Comment on attachment 8939721 [details]
Bug 1426041 - rearrange and add 2 columns toolbar layout to support bottom and side dock mode;

https://reviewboard.mozilla.org/r/210028/#review215724

> I really like this way to construct reusable components.
> 
> For coding style consistency, please do not define new method out of Class component. And remember `module.exports = ...` should be the last item in a file.

thank for review, the previous style is aligned with https://searchfox.org/mozilla-central/source/devtools/client/framework/components/toolbox-toolbar.js#167

I've updated the patch to align with our coding style
Summary: Fix netmontor toolbar items layout in Portrait mode → Fix netmonitor toolbar items layout in Portrait mode
Comment on attachment 8939721 [details]
Bug 1426041 - rearrange and add 2 columns toolbar layout to support bottom and side dock mode;

https://reviewboard.mozilla.org/r/210028/#review215778

Looks good to me!

Two comments:

1) The details button is not properly centered, it should be moved 1-2 pixels up
2) Can we make the filter bar style the same as in the Console panel?
Something like as follows:

element {
  background-color: transparent;
  border: none;
}

Honza
Comment on attachment 8939721 [details]
Bug 1426041 - rearrange and add 2 columns toolbar layout to support bottom and side dock mode;

https://reviewboard.mozilla.org/r/210028/#review216132

Yep, it goes in the right direction:

Couple of more comments:
1) The Console filter box doesn't have blue border when focused. The Net panel should do the same for consistency.
2) The details button is still not v-centered. Screenshot coming...

Thanks!
Honza
Attachment #8939721 - Flags: review?(odvarko) → review-
Attached image vcentering.png
See the details button in portrait mode. The checkboxes would deserve better centering too.

Honza
I'd be curious to see if there are any perf regressions to this.
This is looking great, Fred and everyone! Some comments:

(In reply to Fred Lin [:gasolin] from comment #10)
> Victoria, current patch followed
> https://mozilla.invisionapp.com/share/2XEEY0RYA#/screens/
> 263401584_Network_Toolbar version,
> but I also saw different variants (Network Vertical) in your doc
> https://docs.google.com/document/d/1HG2mJi8jNklgUnIMND-iTnoPByI7HnAexmUXG-
> lyyCI/ could you point which is the preferred one?

Sorry about the confusion here. Only the top section called "Development Needed" is the final spec. The rest of the doc is just there for record-keeping/future UX work. I updated the doc to make this more clear.

> For webconsole, current webconsole has a filter icon button to toggle those
> filter buttons in a separate line. According to the doc, does the webconsole
> design heads to remove the filter icon button and use something like current
> network monitor does (list all filter buttons on toolbar)?

Correct, the Console will be removing that filter toggle functionality and showing all filter buttons by default, but there will be an optional minimal toolbar setting as well. (I don't know when this will be happening exactly - hopefully also in Q1! I'll bring it up with Harald next week.)

> 1. netmonitor has a `detail button` at right side, I keep it align right in 
> side-dock mode if there's more space.
> 2. I expand the searchbox in bottom dock mode when there's more space.

Yes, sounds good! I'm not sure how I totally forgot about the detail button.

(In reply to Jan Honza Odvarko [:Honza] from comment #16)
> Comment on attachment 8939721 [details]
> 1) The Console filter box doesn't have blue border when focused. The Net
> panel should do the same for consistency.

- The Console team is also adding the blue border in this bug, so this should match it https://bugzilla.mozilla.org/show_bug.cgi?id=1421395
- The border should be 1px blue 50 - no shadow needed for this style of minimal input

Regarding the latest screenshot I see from Honza:
- Looks like the checkboxes could use a little less spacing on the left. 
- Could we use the bolder, denser-looking pause icon from bug? https://bugzilla.mozilla.org/show_bug.cgi?id=1410351
-
Flags: needinfo?(victoria)
Also - for your next patch, if you send me a try build for MacOS, I can give more exact comments on visual spacing tweaks. Thanks Fred!
(In reply to Victoria Wang [:victoria] from comment #19)
> - The Console team is also adding the blue border in this bug, so this
> should match it https://bugzilla.mozilla.org/show_bug.cgi?id=1421395
I didn't know about this bug, good catch, thanks!
Honza
Update PR to align items.

The 2nd part is about highlight with the blue border. It's optional for this bug. If bug 1421395 doesn't cover netmonitor part, I'll solve the focus styles in another bug. Add here to better demo the result to Victoria.
Victoria, you could try
https://queue.taskcluster.net/v1/task/LKr_ob83TPGD4jXEDyjz9g/runs/0/artifacts/public/build/target.dmg

to see the latest change
Flags: needinfo?(victoria)
Blocks: 1429296
Comment on attachment 8939721 [details]
Bug 1426041 - rearrange and add 2 columns toolbar layout to support bottom and side dock mode;

https://reviewboard.mozilla.org/r/210028/#review217756

Looks good to me.

R+, assuming try is green

Thanks Fred for working on this!
Honza
Attachment #8939721 - Flags: review?(odvarko) → review+
Btw. the patch needs to be rebased on top of m-c.

Honza
Thanks for the build link Fred! I'm slower to respond on Network tasks these days because things are getting really busy over in DesignTools/Inspector, and will continue to be busy for the next 3 weeks. If you all have any specific questions that are blocking work, let me know. Here are a couple quick comments:

Vertical mode is looking great! When I first bring it up and open the request details part though, that request details panel is really short - maybe we could make it 50% height by default. Also, I think the truncation in the table header names is a bit too aggressive - maybe we could drop some left/right padding so most of those don't have to be truncated.

In horizontal mode, with the new wider-by-default 7-tab sidebar, I'm a bit worried that the new right-aligned filter buttons look too associated with the sidebar. I think this effect will be lessened after we put in the throttle drop down in the right side of the toolbar. (Which I know I haven't had a chance to make a mockup for yet)

Sounds like focus ring issues will be dealt with separately. I'm eager to see the filtering by request type as well :).
Flags: needinfo?(victoria)
A few other tweaks: 
- The separator line between the clear button and filter field seems to be 1px too far to the left (compared to the separator above it)
- I think the breakpoint for switching to 2-row toolbar should happen only after the filter input is narrow enough to almost start cutting off the "Filter URLs" label. Also it seems like the breakpoints for 2-row and for moving the checkbox options are a bit off. (The latter happens a bit before the former when making the window smaller)
- RE: left spacing on Persist logs and Disabled Cache, I would recommend these changes:

.devtools-checkbox-label
    margin-inline-start: 4px;
    margin-inline-end: 7px;

Less space after the first one:
.devtools-checkbox-label:first-child 
    margin-inline-end: 0;

Also a little more spacing around these would be nice:
.requests-list-filter-buttons {
    margin: 0 8px;
Assignee: gasolin → odvarko
Attachment #8939721 - Attachment is obsolete: true
Comment on attachment 8967701 [details]
Bug 1426041 - Fix toolbar layout in Portrait mode;

https://reviewboard.mozilla.org/r/236420/#review242204


Code analysis found 7 defects in this patch:
 - 7 defects 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/netmonitor/src/components/Toolbar.js:276
(Diff revision 1)
> +    let {
> +      toggleRecording,
> +      clearRequests,
> +      requestFilterTypes,
> +      setRequestFilterText,
> +      networkDetailsToggleDisabled,

Error: 'networkdetailstoggledisabled' is assigned a value but never used. [eslint: no-unused-vars]

::: devtools/client/netmonitor/src/components/Toolbar.js:276
(Diff revision 1)
> +    let {
> +      toggleRecording,
> +      clearRequests,
> +      requestFilterTypes,
> +      setRequestFilterText,
> +      networkDetailsToggleDisabled,

Error: 'networkdetailstoggledisabled' is missing in props validation [eslint: react/prop-types]

::: devtools/client/netmonitor/src/components/Toolbar.js:277
(Diff revision 1)
> +      toggleRecording,
> +      clearRequests,
> +      requestFilterTypes,
> +      setRequestFilterText,
> +      networkDetailsToggleDisabled,
> +      networkDetailsOpen,

Error: 'networkdetailsopen' is assigned a value but never used. [eslint: no-unused-vars]

::: devtools/client/netmonitor/src/components/Toolbar.js:277
(Diff revision 1)
> +      toggleRecording,
> +      clearRequests,
> +      requestFilterTypes,
> +      setRequestFilterText,
> +      networkDetailsToggleDisabled,
> +      networkDetailsOpen,

Error: 'networkdetailsopen' is missing in props validation [eslint: react/prop-types]

::: devtools/client/netmonitor/src/components/Toolbar.js:278
(Diff revision 1)
> +      clearRequests,
> +      requestFilterTypes,
> +      setRequestFilterText,
> +      networkDetailsToggleDisabled,
> +      networkDetailsOpen,
> +      toggleNetworkDetails,

Error: 'togglenetworkdetails' is assigned a value but never used. [eslint: no-unused-vars]

::: devtools/client/netmonitor/src/components/Toolbar.js:278
(Diff revision 1)
> +      clearRequests,
> +      requestFilterTypes,
> +      setRequestFilterText,
> +      networkDetailsToggleDisabled,
> +      networkDetailsOpen,
> +      toggleNetworkDetails,

Error: 'togglenetworkdetails' is missing in props validation [eslint: react/prop-types]

::: devtools/client/netmonitor/src/components/Toolbar.js:286
(Diff revision 1)
> +      toggleBrowserCache,
> +      browserCacheDisabled,
> +      recording,
> +      singleRow,
> +    } = this.props;
> +

Error: More than 1 blank line not allowed. [eslint: no-multiple-empty-lines]
Comment on attachment 8967701 [details]
Bug 1426041 - Fix toolbar layout in Portrait mode;

https://reviewboard.mozilla.org/r/236420/#review243064


Code analysis found 7 defects in this patch:
 - 7 defects 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/netmonitor/src/components/Toolbar.js:276
(Diff revision 2)
> +    let {
> +      toggleRecording,
> +      clearRequests,
> +      requestFilterTypes,
> +      setRequestFilterText,
> +      networkDetailsToggleDisabled,

Error: 'networkdetailstoggledisabled' is assigned a value but never used. [eslint: no-unused-vars]

::: devtools/client/netmonitor/src/components/Toolbar.js:276
(Diff revision 2)
> +    let {
> +      toggleRecording,
> +      clearRequests,
> +      requestFilterTypes,
> +      setRequestFilterText,
> +      networkDetailsToggleDisabled,

Error: 'networkdetailstoggledisabled' is missing in props validation [eslint: react/prop-types]

::: devtools/client/netmonitor/src/components/Toolbar.js:277
(Diff revision 2)
> +      toggleRecording,
> +      clearRequests,
> +      requestFilterTypes,
> +      setRequestFilterText,
> +      networkDetailsToggleDisabled,
> +      networkDetailsOpen,

Error: 'networkdetailsopen' is assigned a value but never used. [eslint: no-unused-vars]

::: devtools/client/netmonitor/src/components/Toolbar.js:277
(Diff revision 2)
> +      toggleRecording,
> +      clearRequests,
> +      requestFilterTypes,
> +      setRequestFilterText,
> +      networkDetailsToggleDisabled,
> +      networkDetailsOpen,

Error: 'networkdetailsopen' is missing in props validation [eslint: react/prop-types]

::: devtools/client/netmonitor/src/components/Toolbar.js:278
(Diff revision 2)
> +      clearRequests,
> +      requestFilterTypes,
> +      setRequestFilterText,
> +      networkDetailsToggleDisabled,
> +      networkDetailsOpen,
> +      toggleNetworkDetails,

Error: 'togglenetworkdetails' is assigned a value but never used. [eslint: no-unused-vars]

::: devtools/client/netmonitor/src/components/Toolbar.js:278
(Diff revision 2)
> +      clearRequests,
> +      requestFilterTypes,
> +      setRequestFilterText,
> +      networkDetailsToggleDisabled,
> +      networkDetailsOpen,
> +      toggleNetworkDetails,

Error: 'togglenetworkdetails' is missing in props validation [eslint: react/prop-types]

::: devtools/client/netmonitor/src/components/Toolbar.js:286
(Diff revision 2)
> +      toggleBrowserCache,
> +      browserCacheDisabled,
> +      recording,
> +      singleRow,
> +    } = this.props;
> +

Error: More than 1 blank line not allowed. [eslint: no-multiple-empty-lines]
Attachment #8940670 - Attachment is obsolete: true
Comment on attachment 8967701 [details]
Bug 1426041 - Fix toolbar layout in Portrait mode;

https://reviewboard.mozilla.org/r/236420/#review244472

Looks awesome and feels fast.  R+ with whatever nits cleanup needed.  NICE!
Attachment #8967701 - Flags: review?(dwalsh) → review+
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6c71dc375f44
Fix toolbar layout in Portrait mode; r=davidwalsh
https://hg.mozilla.org/mozilla-central/rev/6c71dc375f44
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
QA Whiteboard: [good first verify]
I have reproduced this bug with Nightly 59.0a1 (2017-12-18) on Windows 10 , 64 Bit ! 

This bug's fix is Verified with latest Beta !

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

[bugday-20180516]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.