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

Implement tab scrolling on tabbar #297

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

krassowski
Copy link
Member

@krassowski krassowski commented Mar 27, 2022

As a solution to jupyterlab/jupyterlab#10305 I would like to propose adding (optional) tabbar scrolling.

Downstream applications need to define min-width on tabs for scrolling to work reasonably.

Associated JupyterLab PR: jupyterlab/jupyterlab#12278

TODO:

  • scroll activated tab into view when switching tabs or opening a new tag
  • fix tests
  • replace div buttons with proper buttons?
  • hoist scrolling rate and interval to options, or CSS?
  • add tests
  • scrolling to active tab should not happen when closing another tab

User facing changes

Users can scroll:

  • by pressing the arrow buttons at the ends of tabs list (which only show up if the tab bar overflows)
  • by using browser built-in scrolling (e.g. Shift + mouse wheel, or just a wheel)
  • when dragging tabs by hovering over the arrow buttons

scrollbar

Backward compatibility

One wrapper div was added to the DOM hierarchy which may invalidate some some of the downstream CSS styles (if they use strict > operator). Otherwise this PR aims to be backward compatible.

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 for starting this @krassowski

I'm 👍 to use real buttons.

Comment on lines +1015 to +1006
let scrollBeforeButtonClicked =
this.scrollingEnabled &&
this.scrollBeforeButtonNode.contains(event.target as HTMLElement);

let scrollAfterButtonClicked =
this.scrollingEnabled &&
this.scrollAfterButtonNode.contains(event.target as HTMLElement);
Copy link
Member

Choose a reason for hiding this comment

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

Could we add the event listeners directly on the buttons instead of looking afterwards?

Comment on lines +1120 to +1127
let overBeforeScrollButton =
this.scrollingEnabled &&
this.scrollBeforeButtonNode.contains(event.target as HTMLElement);

let overAfterScrollButton =
this.scrollingEnabled &&
this.scrollAfterButtonNode.contains(event.target as HTMLElement);
Copy link
Member

Choose a reason for hiding this comment

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

Same question

@ajbozarth
Copy link
Member

Adding my overall review here (I tested both PRs together):

Testing this locally I've found a few issues/oddities:

  • The scroll on my Mac trackpad is reversed when scrolling (this may be that lab is ignoring a system setting for scroll direction on a trackpad). My scroll is set to behave like a phone but it's behaving like a mouse wheel. Also the scroll is inconsistent in behavior, it sometime starts to scroll then stops (again a trackpad thing, usually happens when I do short quick scrolls, which I do often).
  • The scrolling is "fluid" unlike how Firefox does it. In FF clicking the button hard scrolls the approx. length of a tab, whereas this scrolls like a webpage scrollbar. I believe the "hard pop" from tab to tab like in FF is a better UX.
  • There's now a gap between the tab and the main area that wasn't there before, you can see the difference in screenshots posted by @krassowski and @aiqc in Enable scrolling tabs in tab bar in main area jupyterlab#12278
  • The "fade" around the scroll button is a bit tall for some reason Screen Shot 2022-04-07 at 2 44 29 PM
  • On the note of default tab width I think we could get away with it being the width of the icon set (ie the tab width if you ignore the label). That way the icon, close button, and kernel icon will all be visible
  • On a positive note I like the size of the scroll buttons, they look good

Note that I tested this on Safari 15.3

I'll come back to test this a bit more later, as for the code I saw no large issues with it, but will admit the scroll handling code was a bit dense to understand since I haven't dealt with scroll handling in years

@ajbozarth
Copy link
Member

A code point I forgot: Is this still considered backwards compatible if we change the DOM structure? I'm not sure how detailed jupyterlab defines API breaks, does it include changes to the DOM? If so would this even be valid for lab 3.4?

@krassowski
Copy link
Member Author

There's now a gap between the tab and the main area that wasn't there before, you can see the difference in screenshots posted by krassowski and aiqc in jupyterlab/jupyterlab#12278

@ajbozarth I don't see this. Could you please circle the fragment you are referring or link before-after pictures?

@ajbozarth
Copy link
Member

Before:

Note how the tab and the empty toolbar are all one continuous "block"
compared to

where there is a line/border between the tab and toolbar section. That "line" should be there for tabs in the background, but not the active tab

@krassowski krassowski self-assigned this Aug 3, 2022
- smoother: use `requestAnimationFrame` instead of `setInterval`
- dragging:
  - detach by scrolling (requires scroll delta computation)
  - update drag when scrolling (by dispatching last event again to
    invoke handler as if mouse moved)
- wheel: allow to scroll using mouse wheel
Reducing the complexity of the logic and fixing tab add/close actions
@krassowski
Copy link
Member Author

The scroll on my Mac trackpad is reversed when scrolling (this may be that lab is ignoring a system setting for scroll direction on a trackpad). My scroll is set to behave like a phone but it's behaving like a mouse wheel. Also the scroll is inconsistent in behavior, it sometime starts to scroll then stops (again a trackpad thing, usually happens when I do short quick scrolls, which I do often).

This could be solved by using a dummy element which intercepts scroll events as in #652 - which is the recommended approach (as compared to using mouse wheel event as this PR attempted).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants