Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not set the dockpanel as parent of the tabbar #606

Merged
merged 7 commits into from
Jul 19, 2023

Conversation

brichet
Copy link
Contributor

@brichet brichet commented Jul 5, 2023

This PR fixes #604

It adds an attribute to Widget to disable the tracking from focusTracker, event if its elements can get the focus.

By default all the widgets are trackable, except for the TabBar.

It avoids setting the parent of the tab bar in the dock panel.

@brichet brichet added bug Something isn't working feature Adds functionality labels Jul 5, 2023
@brichet brichet marked this pull request as ready for review July 5, 2023 10:11
@fcollonval
Copy link
Member

Thanks a lot for digging this one up @brichet

I would rather avoid adding a new attribute for this.

Looking at the current behavior, when the dock panel creates a split, it adds a tab bar. When that tab bar is attached to the panel, its parent is set.

tabBar.parent = this.parent;

That triggers the child-added / child-removed message listened by the focus tracker.

let msg = new Widget.ChildMessage('child-added', this);

The addition of the split handle does not triggered such messages as they are directly attached to the HTML element.


This is actually wrong as adding/removing a tab bar should be transparent for dock panels. The message child-added and child-removed should only be triggered for child panels.

The wrong behavior can also be seen in JupyterLab by the wrongly addition of the class jp-Activity to the tab bars (it should only be on the MainAreaWidget) :

tabbar-issue


Therefore I think a better fix would be to not set the tab bar parent, simply attaching it as it is done for the handle.

cc @afshin @krassowski what do you think?

Another approach could be the inhibition of the child-added/child-removed message emission. But I don't see a good way of doing this without overriding the set parent method of the tab bar. But then it may have unwanted effect for downstream code consuming tab bar outside a dock panel (like the weird usage in the JupyterLab sidebars).

@brichet
Copy link
Contributor Author

brichet commented Jul 17, 2023

Therefore I think a better fix would be to not set the tab bar parent, simply attaching it as it is done for the handle.

Thanks for the suggestion @fcollonval, I reverted the previous changes and add a commit to avoid setting the parent of the tab bar.

@brichet brichet changed the title focusTrackable attribute for Widget Do not set the dockpanel as parent of the tabbar Jul 17, 2023
Comment on lines 50 to 51
let w1 = new Widget();
panel.addWidget(w1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, sorry to drop a comment out of the blue, but I stumbled on this PR while researching something else, and I realized that while trying to read the code in this PR that these two lines in the test function are not self-evident to me. Should they be followed with an assertion?

Suggested change
let w1 = new Widget();
panel.addWidget(w1);
let w1 = new Widget();
panel.addWidget(w1);
expect(panel.contains(w1)).to.be.true;

Copy link
Contributor Author

@brichet brichet Jul 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prior to this modification, the TabBar was added as a child widget of the DockPanel, as well as the document panels. The TabBar was only added if at least one widget had been added to the DockPanel.
This test adds a widget only to ensure the TabBar is not added to the child widgets list (which was the case prior to this PR).
In fact I believe it should be simplified, maybe this is what confused you.

Suggested change
let w1 = new Widget();
panel.addWidget(w1);
panel.addWidget(new Widget());

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TabBar was only added if at least one widget had been added to the DockPanel.

Without knowing these implementation details, somebody reading this test has to guess at the intent of the panel.addWidget() call. Could you clarify your intent with a comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in f0a3d33

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @brichet

@fcollonval fcollonval merged commit adb6838 into jupyterlab:main Jul 19, 2023
15 of 16 checks passed
@brichet brichet deleted the focus_trackable branch July 19, 2023 12:09
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working feature Adds functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Focus tracker does not focus on tab item
3 participants