Improve resizing behavior of debugger panels #10653

trungleduc · 2021-07-20T13:03:47Z

References

The panels of debugger sidebar are resizable but since the height of panel is not limited, users can completely overlap one over others.

old.mp4

This PR improves the resizing behavior of these panels to prevent this situation and adds the possibility to click on the header to expand or contract corresponding panel.

Code changes

Create new base class of header and panel of debugger to handle resizing event and onclick event.
Add new method to DebuggerSidebar in order to recompute height of all panels, this method is called by children panels.
Some minor refactorings to avoid circular import problem of tests in debugger package.

User-facing changes

Panel will push back if it is overlapped by others
Click on panel header will expand or contract this panel, depend on its current height
A new icon to indicate the open/close status of panel

new.mp4

Backwards-incompatible changes

N/A

welcome · 2021-07-20T13:03:49Z

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.

You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

jupyterlab-dev-mode · 2021-07-20T13:03:54Z

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link:

jtpio · 2021-07-21T10:07:56Z

Thanks for this, looking good!

packages/debugger/src/panels/basedebuggerpanel.ts

krassowski

Great work, thank you. Have you considered using CSS grid instead of doing the size computations manually? See https://github.com/jess-x/jupyterlab/pull/1/files for some inspiration; this is a question and not a request - I am just curious to know if you tried. Also, two minor UX points:

Since the header is now a click target, it is now possible for the annoying text-selection-on-click effect to kick-in. I would consider adding user-select: none; to the header label to prevent that effect:
The height limitation only kicks in after resizing. I wonder if it could work during resizing. I imagine it could be more difficult to implement, so absolutely no need to rework it right now, but it would be good to know if you tried addressing that and if you have any thoughts on what would be needed to solve it.

krassowski · 2021-07-25T23:07:07Z

packages/debugger/src/panels/header.ts

+    this._paddingDiv.node.style.flexGrow = '1';
+    this._paddingDiv.node.style.height = '100%';


Can we move this to a CSS file? I think the convention is to create a new class name matching the name of the class, so jp-PanelHeader and use appropriate selector in CSS styles file.

packages/debugger/src/sidebar.ts

trungleduc · 2021-07-26T07:55:57Z

@krassowski thank you for reviewing my PR.
For the CSS approach, I haven't considered it because this debugger panel used SplitPanel from lumino, and in this widget, the absolute position and height of the child panel are re-computed manually on resize event so i tried to use the same approach.

On the other hand, the PR of @fcollonval on lumino (jupyterlab/lumino#205) uses a CSS approach and I think it's a better way to directly use an upstream widget instead of reinventing it in client code. I can update this PR once the AccordionPanel widget is available.

jtpio · 2021-08-26T08:52:59Z

I can update this PR once the AccordionPanel widget is available.

Looks like jupyterlab/lumino#205 has now been merged and released, so maybe we can update this PR to reuse the upstream widget?

trungleduc · 2021-08-27T07:24:07Z

@jtpio yes i'll do it

jtpio · 2021-09-06T08:07:37Z

@trungleduc mind doing one more rebase to fix the conflicts? Thanks!

trungleduc · 2021-09-06T10:27:43Z

@jtpio I'll update this PR this week to incorporate AccordionPanel from upstream.

jtpio · 2021-09-08T12:45:47Z

packages/debugger/src/panels/breakpoints/index.ts

    this.addClass('jp-DebuggerBreakpoints');
  }

+  get header(): PanelHeader {


Let's add a docstring for consistency?

I'm working on it, this PR needs 1-2 more commits to be ready for review.

jtpio · 2021-09-09T06:47:08Z

Looks like the test failure might be relevant?

fcollonval

Thanks @trungleduc

Some early comment on using the AccordionPanel. Let me know if you want to discuss about that (maybe some interfaces of the AccordionPanel needs to be adapted).

fcollonval · 2021-09-09T08:40:51Z

packages/debugger/src/panels/panelbody.ts

+}
+
+export namespace DebuggerPanelBody {
+  export class Renderer extends SplitPanel.Renderer {


You should inherit from AccordionPanel.Renderer

Resolved is last commits

fcollonval · 2021-09-09T08:43:27Z

packages/debugger/src/panels/panelbody.ts

+     * @returns A element representing the collapse indicator.
+     */
+    createCollapseIcon(data: Title<Widget>): HTMLElement {
+      return document.createElement('span');


You should use an SVG icon here. See for example https://github.com/jupyterlab/jupyterlab/blob/master/packages/ui-components/src/icon/widgets/tabbarsvg.ts#L30 for the close icon on tabs

Resolved is last commits

fcollonval · 2021-09-09T08:51:13Z

packages/debugger/src/panels/panelbody.ts

+   * click. Since the toolbar contains buttons, icons..., we need to check
+   * the type of `env.target` before activating handler.
+   */
+  private _handleClick(event: MouseEvent): void {


I don't get why click event needs to be overridden

fcollonval · 2021-09-09T08:54:14Z

packages/debugger/src/panels/panelbody.ts

+      handle.id = this.createTitleKey(data);
+      handle.className = this.titleClassName;
+      if (header) {
+        console.log('adding', header.node);


Reminder to remove it

Suggested change

console.log('adding', header.node);

Resolved is last commits

fcollonval · 2021-09-09T08:55:32Z

packages/debugger/src/panels/panelbody.ts

+        header.processMessage(Widget.Msg.BeforeAttach);
+        handle.appendChild(header.node);
+        header.processMessage(Widget.Msg.AfterAttach);


Prefer the Widget helper:

Suggested change

header.processMessage(Widget.Msg.BeforeAttach);

handle.appendChild(header.node);

header.processMessage(Widget.Msg.AfterAttach);

Widget.attach(header, handle);

Widget.attach does not work here since handle is not yet attached to DOM

fcollonval · 2021-09-09T08:56:45Z

packages/debugger/src/panels/sources/header.tsx

-    const title = new Widget({ node: document.createElement('h2') });
-    title.node.textContent = trans.__('Source');
+    super(translator);
+    this.titleWidget.node.textContent = this._trans.__('Source');


This should be set on the PanelBody title: this.title.label = ...

Suggested change

this.titleWidget.node.textContent = this._trans.__('Source');

Resolved is last commits

fcollonval · 2021-09-09T08:58:11Z

packages/debugger/src/panels/panelbody.ts

+      const handle = document.createElement('h3');
+      handle.setAttribute('role', 'button');
+      handle.setAttribute('tabindex', '0');
+      handle.id = this.createTitleKey(data);
+      handle.className = this.titleClassName;


It will be better to call the default implementation here first:

Suggested change

const handle = document.createElement('h3');

handle.setAttribute('role', 'button');

handle.setAttribute('tabindex', '0');

handle.id = this.createTitleKey(data);

handle.className = this.titleClassName;

const handle = super.createSectionTitle(data);

Resolved is last commits

fcollonval · 2021-09-09T08:59:59Z

packages/debugger/src/panels/header.ts

+/**
+ * The base header for a debugger panels.
+ */
+export class PanelHeader extends Widget {


You should not use a header widget but add the toolbar to the PanelBody and use its title attribute to set the metadata.

Resolved is last commits

trungleduc · 2021-09-10T06:06:19Z

Hi folks, It looks like benchmark tests and UI tests are failing for others PRs too, so this PR is ready for review again.
Thanks!

jtpio · 2021-09-10T12:30:35Z

It looks like benchmark tests and UI tests are failing for others PRs too, so this PR is ready for review again.

One more rebase should hopefully pick the latest fixes and make these checks green.

blink1073 · 2021-09-10T12:33:33Z

Shouldn't need a rebase, just a new commit or close/open

github-actions · 2021-09-10T12:47:57Z

Benchmark report

The execution time (in milliseconds) are grouped by test file, test type and browser.
For each case, the following values are computed: min <- [1st quartile - median - 3rd quartile] -> max.

Results table

Test file	large_code_notebook	large_md_notebook
open
`chromium`
actual	4828 <- [6130 - 6817 - 6924] -> 7399	3071 <- [3096 - 3138 - 3216] -> 3344
expected	6938 <- [7584 - 7618 - 7760] -> 8204	3461 <- [3524 - 3534 - 3590] -> 4136
switch-from
`chromium`
actual	387 <- [411 - 421 - 436] -> 475	277 <- [291 - 298 - 311] -> 424
expected	464 <- [480 - 489 - 499] -> 789	310 <- [338 - 344 - 352] -> 370
switch-to
`chromium`
actual	1564 <- [1624 - 1654 - 1664] -> 1721	1320 <- [1371 - 1388 - 1416] -> 1484
expected	1706 <- [1730 - 1764 - 1814] -> 2015	1541 <- [1549 - 1562 - 1574] -> 1608
close
`chromium`
actual	4896 <- [10768 - 13033 - 13282] -> 17488	3188 <- [3357 - 3471 - 3537] -> 3715
expected	5176 <- [5770 - 10533 - 14493] -> 18824	3325 <- [3691 - 3890 - 3939] -> 4073

❗ Test metadata have changed

--- /dev/fd/63	2021-09-15 10:27:23.746670755 +0000
+++ /dev/fd/62	2021-09-15 10:27:23.746670755 +0000
@@ -34,7 +34,7 @@
       "voltage": ""
     },
     "mem": {
-      "total": 7291699200
+      "total": 7291695104
     },
     "osInfo": {
       "arch": "x64",
@@ -42,11 +42,11 @@
       "codename": "Focal Fossa",
       "codepage": "UTF-8",
       "distro": "Ubuntu",
-      "kernel": "5.8.0-1041-azure",
+      "kernel": "5.8.0-1040-azure",
       "logofile": "ubuntu",
       "platform": "linux",
       "release": "20.04.3 LTS",
-      "serial": "faac5b9e00cf4ae3b2d2c013e98d933a",
+      "serial": "0f5fd491ff264b5f9d56e03599b4fae0",
       "servicepack": "",
       "uefi": false
     }

fcollonval

Thanks @trungleduc I tested and noticed a couple of styling issues:

The collapsed title is smaller than the uncollapsed one so the buttons are not contained in the collapsed stated
Clicking on the icon does not toggle the collapse state
- We should ask at the dev meeting why the CommandToolbarButton is not stopping the propagation of the mouse event. It will simplify the code and avoid further problem.
The icon is not changing with the collapsed state

Test done on FireFox 88

trungleduc · 2021-09-10T21:00:50Z

Thanks @fcollonval for testing it.
Most of CSS bugs are caused by removing the intermediate header class in recent commits, I fixed it in last commit, it now works in both Firefox and Chrome.

debugger.mp4

Clicking on the icon does not toggle the collapse state
We should ask at the dev meeting why the CommandToolbarButton is not stopping the propagation of the mouse event. It will simplify the code and avoid further problem.

I tried to call stopPropagation in onClick and onMouseDown of toolbar button (ToolbarButtonComponent) but it does not help.

packages/debugger/src/panels/sources/index.tsx

…leduc/jupyterlab into ft/improve-debugger-panels

fcollonval · 2021-09-15T09:02:08Z

I push two commits. The first one simplify the code:

Stop propagating the click event bubbling from the toolbar
Remove no more needed event filtering steps
Rename header to toolbar to be coherent
Handle the toolbar where it is defined (not adding it in all panels)

The second switch to use caretDownIcon for consistency with what is used for cells collapser and extension panel:

We need to wait for a new release of @lumino/widgets to remove code dealing with the lm-mod-expanded class not set on the correct element (see upstream PR).

trungleduc · 2021-09-15T09:13:12Z

Thanks @fcollonval.

welcome · 2021-09-15T12:27:51Z

Congrats on your first merged pull request in this project! 🎉

Thank you for contributing, we are very proud of you! ❤️

github-actions bot assigned trungleduc Jul 20, 2021

github-actions bot added Design System CSS pkg:debugger tag:CSS For general CSS related issues and pecadilloes labels Jul 20, 2021

trungleduc changed the title ~~Ft/improve debugger panels~~ Improve debugger panels resizing behavior Jul 20, 2021

trungleduc changed the title ~~Improve debugger panels resizing behavior~~ Improve resizing behavior of debugger panels Jul 20, 2021

fcollonval added the bug label Jul 20, 2021

fcollonval added this to the 4.0 milestone Jul 20, 2021

trungleduc marked this pull request as draft July 20, 2021 13:18

trungleduc marked this pull request as ready for review July 20, 2021 13:43

jtpio reviewed Jul 21, 2021

View reviewed changes

packages/debugger/src/panels/basedebuggerpanel.ts Outdated Show resolved Hide resolved

krassowski reviewed Jul 25, 2021

View reviewed changes

packages/debugger/src/sidebar.ts Outdated Show resolved Hide resolved

trungleduc force-pushed the ft/improve-debugger-panels branch from 9533b46 to b363ca0 Compare September 8, 2021 11:53

jtpio reviewed Sep 8, 2021

View reviewed changes

fcollonval reviewed Sep 9, 2021

View reviewed changes

trungleduc added 5 commits September 9, 2021 23:22

Refactor debugger panel to expose header widget

502888c

Use Accordion widget from lumino

1d42ae5

Rotate title icon on click

babd5cf

Fix CSS problem

abfebc3

Lint code

add5551

trungleduc force-pushed the ft/improve-debugger-panels branch from 37c27df to 88ad9a9 Compare September 9, 2021 21:25

User Toolbar directly instead of header widget

b8453b0

fcollonval requested changes Sep 10, 2021

View reviewed changes

trungleduc added 2 commits September 10, 2021 22:07

Fix CSS bug

1848d48

Toggle panel on icon click

bd54fe4

Toggle panel on icon click

c1541cc

fcollonval mentioned this pull request Sep 14, 2021

Class and aria attribute must be changed on title jupyterlab/lumino#232

Merged

krassowski reviewed Sep 14, 2021

View reviewed changes

packages/debugger/src/panels/sources/index.tsx Outdated Show resolved Hide resolved

trungleduc and others added 3 commits September 14, 2021 22:13

Merge branch 'ft/improve-debugger-panels' of https://github.com/trung…

eb8bb91

…leduc/jupyterlab into ft/improve-debugger-panels

Fix typo

246ca1f

Simplify the code

bb2c9bc

github-actions bot added the pkg:ui-components label Sep 15, 2021

Use caret icon coherent with JLab

0cc8950

Fix unit tests

91a0b5e

fcollonval self-requested a review September 15, 2021 09:23

fcollonval approved these changes Sep 15, 2021

View reviewed changes

Set title label line-height to 22px

7290955

blink1073 merged commit bcc1dc1 into jupyterlab:master Sep 15, 2021

trungleduc deleted the ft/improve-debugger-panels branch September 15, 2021 12:48

jtpio mentioned this pull request Nov 30, 2021

Collapse the sections in the Debugger sidebar #9108

Closed

github-actions bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Mar 15, 2022

github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2022

		this._paddingDiv.node.style.flexGrow = '1';
		this._paddingDiv.node.style.height = '100%';

Improve resizing behavior of debugger panels #10653

Improve resizing behavior of debugger panels #10653

Conversation

trungleduc commented Jul 20, 2021 • edited Loading

References

Code changes

User-facing changes

Backwards-incompatible changes

welcome bot commented Jul 20, 2021

jupyterlab-dev-mode bot commented Jul 20, 2021

jtpio commented Jul 21, 2021

krassowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trungleduc commented Jul 26, 2021 • edited Loading

jtpio commented Aug 26, 2021

trungleduc commented Aug 27, 2021

jtpio commented Sep 6, 2021

trungleduc commented Sep 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtpio commented Sep 9, 2021

fcollonval left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trungleduc commented Sep 10, 2021

jtpio commented Sep 10, 2021

blink1073 commented Sep 10, 2021

github-actions bot commented Sep 10, 2021 • edited Loading

Benchmark report

fcollonval left a comment • edited Loading

Choose a reason for hiding this comment

trungleduc commented Sep 10, 2021

fcollonval commented Sep 15, 2021

trungleduc commented Sep 15, 2021

welcome bot commented Sep 15, 2021

trungleduc commented Jul 20, 2021 •

edited

Loading

trungleduc commented Jul 26, 2021 •

edited

Loading

github-actions bot commented Sep 10, 2021 •

edited

Loading

fcollonval left a comment •

edited

Loading