Closed Bug 1309183 Opened 8 years ago Closed 7 years ago

Replace XUL Splitter by SplitBox

Categories

(DevTools :: Netmonitor, defect, P1)

defect

Tracking

(firefox52 affected, firefox54 verified)

VERIFIED FIXED
Firefox 54
Iteration:
54.2 - Feb 20
Tracking Status
firefox52 --- affected
firefox54 --- verified

People

(Reporter: rickychien, Assigned: rickychien)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [netmonitor])

Attachments

(1 file, 1 obsolete file)

Make sure we actually need the sidebar (Edit & Resend, WS frames in the future)

It depends on new UX spec, let’s discuss on Bug 1307667.
Whiteboard: [netmonitor]
Flags: qe-verify+
QA Contact: ciprian.georgiu
Priority: -- → P2
We should also make sure the that vertical mode is working as expected. It's currently broken and the Sidebar (details view) is taking the entire area (making the request list hidden).

Honza
Priority: P2 → P1
Depends on: 1317645
browser_net_prefs-reload.js is going to disable temporarily in bug 1317645 because width and height of network-details-panel should be tackled in SplitBox.

Please remember to tackle and re-enable browser_net_prefs-reload.js along with this patch.
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Iteration: --- → 54.2 - Feb 20
Depends on: 1336383
This patch includes:

* Create a MonitorPanel react component as alternative <vbox id="inspector-panel">...</vbox> element. The original name has renamed to inspector-panel, but feel free to provide any better naming suggestions.
* Marked bug 1336381 as duplicate.
* Moved networkDetailsWidth and networkDetailsHeight Prefs initialize/destroy and updating formDataSections from RequestList to MonitorPanel.
* Moved toolbar filters Prefs to Toolbar component.
* Removed XUL splitter workaround which was introduced in network-details-panel.js.
* Removed height: calc(100vh - 24px); which was introduced by custom request panel.
* A few fixes is supposed to be done in https://bugzilla.mozilla.org/show_bug.cgi?id=1336383#c7, but in fact I forgot to include these fixes in bug 1336383 after addressing patch conflict. So I merge them into this patch. You will see request-list-context-menu.js and request-list-tooltip.js moved to netmonitor folder.
Blocks: 1336384
Here are several warning I saw after applied the patch, otherwise it works well

Warning: Failed prop type: Invalid prop `maxSize` of type `string` supplied to `SplitBox`, expected `number`.
    in SplitBox (created by MonitorPanel)
    in MonitorPanel (created by Connect(MonitorPanel))
    in Connect(MonitorPanel)
    in Provider  react-dev.js:22807:9
Warning: Failed prop type: Invalid prop `requestFilterTypes` of type `array` supplied to `Toolbar`, expected `object`.
    in Toolbar (created by Connect(Toolbar))
    in Connect(Toolbar) (created by MonitorPanel)
    in div (created by MonitorPanel)
    in MonitorPanel (created by Connect(MonitorPanel))
    in Connect(MonitorPanel)
    in Provider
Warning: Failed prop type: Invalid prop `children` of type `array` supplied to `Tabbar`, expected `object`.
    in Tabbar (created by TabboxPanel)
    in TabboxPanel (created by Connect(TabboxPanel))
    in Connect(TabboxPanel) (created by NetworkDetailsPanel)
    in div (created by NetworkDetailsPanel)
    in NetworkDetailsPanel (created by Connect(NetworkDetailsPanel))
    in Connect(NetworkDetailsPanel) (created by MonitorPanel)
    in div (created by SplitBox)
    in div (created by SplitBox)
    in SplitBox (created by MonitorPanel)
    in div (created by MonitorPanel)
    in MonitorPanel (created by Connect(MonitorPanel))
    in Connect(MonitorPanel)
    in Provider  react-dev.js:22807:9
"Warning: Each child in an array or iterator should have a unique "key" prop. Check the render method of `TreeView`. See https://fb.me/react-warning-keys for more information.
    in tr (created by TreeView)
    in TreeView (created by PropertiesView)
    in div (created by PropertiesView)
    in div (created by PropertiesView)
    in PropertiesView (created by ResponsePanel)
    in div (created by ResponsePanel)
    in ResponsePanel (created by TabboxPanel)
    in div (created by Panel)
    in Panel (created by TabboxPanel)
    in div (created by Tabs)
    in div (created by Tabs)
    in div (created by Tabs)
    in Tabs (created by Tabbar)
    in div (created by Tabbar)
    in Tabbar (created by TabboxPanel)
    in TabboxPanel (created by Connect(TabboxPanel))
    in Connect(TabboxPanel) (created by NetworkDetailsPanel)
    in div (created by NetworkDetailsPanel)
    in NetworkDetailsPanel (created by Connect(NetworkDetailsPanel))
    in Connect(NetworkDetailsPanel) (created by MonitorPanel)
    in div (created by SplitBox)
    in div (created by SplitBox)
    in SplitBox (created by MonitorPanel)
    in div (created by MonitorPanel)
    in MonitorPanel (created by Connect(MonitorPanel))
    in Connect(MonitorPanel)
    in Provider"
Comment on attachment 8836457 [details]
Bug 1309183 - Replace XUL Splitter by SplitBox

https://reviewboard.mozilla.org/r/111870/#review113534

::: devtools/client/netmonitor/components/monitor-panel.js:40
(Diff revision 8)
> + */
> +const MonitorPanel = createClass({
> +  displayName: "MonitorPanel",
> +
> +  propTypes: {
> +    isEmpty: PropTypes.bool,

missing .isRequired

::: devtools/client/netmonitor/components/monitor-panel.js:109
(Diff revision 8)
> +    }
> +  },
> +
> +  onLayoutChange() {
> +    this.setState({
> +      vertical: MediaQueryList.matches,

`isVertical` is more clear when we check `this.state.isVertical`to save width or height Prefs

::: devtools/client/netmonitor/components/monitor-panel.js:119
(Diff revision 8)
> +    // Allow requests to settle down first.
> +    setNamedTimeout("resize-events", 50, () => {
> +      const waterfallHeaderEl =
> +        document.querySelector("#requests-menu-waterfall-header-box");
> +      if (waterfallHeaderEl) {
> +        const { width } = waterfallHeaderEl.getBoundingClientRect();

this part looks like XUL, could we put them into helper or somewhere, therefore we can handle XUL without touch this component again?

::: devtools/client/netmonitor/components/request-list.js:23
(Diff revision 8)
>  const { div } = DOM;
>  
>  /**
>   * Request panel component
>   */
> -const RequestList = createClass({
> +function RequestList({ isEmpty }) {

nit: { isEmpty }

::: devtools/client/netmonitor/netmonitor-view.js:60
(Diff revision 8)
>      this.unsubscribeStore();
>    },
>  
>    toggleFrontendMode: function () {
>      if (gStore.getState().ui.statisticsOpen) {
>        this._body.selectedPanel = document.querySelector("#react-statistics-panel-hook");

this._body.selectedPanel = this.statisticsPanel;

::: devtools/client/netmonitor/netmonitor-view.js:63
(Diff revision 8)
>    toggleFrontendMode: function () {
>      if (gStore.getState().ui.statisticsOpen) {
>        this._body.selectedPanel = document.querySelector("#react-statistics-panel-hook");
>        NetMonitorController.triggerActivity(ACTIVITY_TYPE.RELOAD.WITH_CACHE_ENABLED);
>      } else {
> -      this._body.selectedPanel = document.querySelector("#inspector-panel");
> +      this._body.selectedPanel = document.querySelector("#react-monitor-panel-hook");

this._body.selectedPanel = this.monitorPanel;

::: devtools/client/themes/netmonitor.css
(Diff revision 8)
>  /* Custom request view */
>  
>  .custom-request-panel {
>    overflow: auto;
> -  /* Full view hight - toolbar height */
> +  padding: 0 4px;
> -  height: calc(100vh - 24px);

The padding is changed, and scroll bar is missing without that setting
Fred, thanks for reporting warnings and warnings are addressed.

Some of these warnings didn't introduced by this patch, but you found it while reaching / triggering these UI components. It's hard to find out all warnings since it needs to navigate all UIs in runtime.

However, it would be easier for us to clean it up in one sweep (open a new issue on Github) after Netmonitor moved to GitHub and enable Flow [1] to do static analysis.

[1] https://flowtype.org/docs/react.html
Comment on attachment 8836457 [details]
Bug 1309183 - Replace XUL Splitter by SplitBox

https://reviewboard.mozilla.org/r/111870/#review113534

> this part looks like XUL, could we put them into helper or somewhere, therefore we can handle XUL without touch this component again?

All of them are standard WebAPIs, it doesn't look like XUL at all. But I noticed this part doesn't belong here, I'll move them to right place. Thanks!

> nit: { isEmpty }

I'm using { isEmpty } here, what do you mean?
Comment on attachment 8836457 [details]
Bug 1309183 - Replace XUL Splitter by SplitBox

https://reviewboard.mozilla.org/r/111870/#review113802

Excellent work here!

Honza

::: devtools/client/netmonitor/components/monitor-panel.js:30
(Diff revision 8)
> +const NetworkDetailsPanel = createFactory(require("../shared/components/network-details-panel"));
> +const RequestList = createFactory(require("./request-list"));
> +const Toolbar = createFactory(require("./toolbar"));
> +
> +const { div } = DOM;
> +const MediaQueryList = window.matchMedia("(min-width: 700px)");

Nice utilization of the matchMedia() API!

::: devtools/client/netmonitor/reducers/ui.js:11
(Diff revision 8)
>  
>  const I = require("devtools/client/shared/vendor/immutable");
>  const {
>    OPEN_NETWORK_DETAILS,
>    OPEN_STATISTICS,
> +  REMOVE_SELECTED_CUSTOM_REQUEST,

How come this wasn't there before?
Attachment #8836457 - Flags: review?(odvarko) → review+
Blocks: 1316291
There is a conflict in autoland, so I'll trigger this land after m-c updated.
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0df3f1f92052
Replace XUL Splitter by SplitBox r=Honza
https://hg.mozilla.org/mozilla-central/rev/0df3f1f92052
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
This issue is verified fixed on latest Nightly 54.0a1 (2017-03-05) under the following OSes:
- Windows 10 x64
- Mac OS X 10.11.6
- Ubuntu 16.04 x64 LTS

The SplitBox is working as expected and, also the vertical mode problem mentioned in comment 1 is fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: