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.

Sign up for GitHub

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

Jump to bottom

Fix scrolling behavior in menu while zoomed in #80965

Merged
merged 9 commits into from
Oct 17, 2019
Merged

Fix scrolling behavior in menu while zoomed in #80965

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
25e9c2e
Fix scrolling behavior in menu while zoomed. Fixes #80047
jeanp413 Sep 16, 2019
1a7fe74
Merge branch 'master' into fix-menu-scroll-zoom
jeanp413 Oct 13, 2019
818d9c9
Merge branch 'master' into fix-menu-scroll-zoom
sbatten Oct 17, 2019
1a4ca1a
:lipstick:
jeanp413 Oct 17, 2019
4c434da
move scrollbar click detection into menu
sbatten Oct 17, 2019
82b0b32
confine preventScroll change to menu
sbatten Oct 17, 2019
e8861a9
Merge branch 'master' into fix-menu-scroll-zoom
sbatten Oct 17, 2019
f6d61f0
fix sticky scrollbar
sbatten Oct 17, 2019
d94604f
Merge branch 'master' into fix-menu-scroll-zoom
sbatten Oct 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/vs/base/browser/ui/actionbar/actionbar.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -768,9 +768,9 @@ export class ActionBar extends Disposable implements IActionRunner {
this.updateFocus(true);
}

protected updateFocus(fromRight?: boolean): void {
protected updateFocus(fromRight?: boolean, preventScroll?: boolean): void {
if (typeof this.focusedItem === 'undefined') {
this.actionsList.focus();
this.actionsList.focus({ preventScroll });
}

for (let i = 0; i < this.viewItems.length; i++) {
Expand All @@ -782,7 +782,7 @@ export class ActionBar extends Disposable implements IActionRunner {
if (actionViewItem.isEnabled() && types.isFunction(actionViewItem.focus)) {
actionViewItem.focus(fromRight);
} else {
this.actionsList.focus();
this.actionsList.focus({ preventScroll });
}
}
} else {
Expand Down
51 changes: 23 additions & 28 deletions src/vs/base/browser/ui/menu/menu.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import { DisposableStore } from 'vs/base/common/lifecycle';
import { Color } from 'vs/base/common/color';
import { DomScrollableElement } from 'vs/base/browser/ui/scrollbar/scrollableElement';
import { ScrollbarVisibility, ScrollEvent } from 'vs/base/common/scrollable';
import { Event, Emitter } from 'vs/base/common/event';
import { Event } from 'vs/base/common/event';
import { AnchorAlignment } from 'vs/base/browser/ui/contextview/contextview';
import { isLinux, isMacintosh } from 'vs/base/common/platform';

Expand Down Expand Up @@ -66,9 +66,6 @@ export class Menu extends ActionBar {
private readonly menuDisposables: DisposableStore;
private scrollableElement: DomScrollableElement;
private menuElement: HTMLElement;
private scrollTopHold: number | undefined;

private readonly _onScroll: Emitter<void>;

constructor(container: HTMLElement, actions: ReadonlyArray<IAction>, options: IMenuOptions = {}) {
addClass(container, 'monaco-menu-container');
Expand All @@ -88,8 +85,6 @@ export class Menu extends ActionBar {

this.menuElement = menuElement;

this._onScroll = this._register(new Emitter<void>());

this.actionsList.setAttribute('role', 'menu');

this.actionsList.tabIndex = 0;
Expand Down Expand Up @@ -153,17 +148,11 @@ export class Menu extends ActionBar {
let relatedTarget = e.relatedTarget as HTMLElement;
if (!isAncestor(relatedTarget, this.domNode)) {
this.focusedItem = undefined;
this.scrollTopHold = this.menuElement.scrollTop;
this.updateFocus();
e.stopPropagation();
}
}));

this._register(addDisposableListener(this.domNode, EventType.MOUSE_UP, e => {
// Absorb clicks in menu dead space https://github.com/Microsoft/vscode/issues/63575
EventHelper.stop(e, true);
}));

this._register(addDisposableListener(this.actionsList, EventType.MOUSE_OVER, e => {
let target = e.target as HTMLElement;
if (!target || !isAncestor(target, this.actionsList) || target === this.actionsList) {
Expand All @@ -176,7 +165,6 @@ export class Menu extends ActionBar {

if (hasClass(target, 'action-item')) {
const lastFocusedItem = this.focusedItem;
this.scrollTopHold = this.menuElement.scrollTop;
this.setFocusedItem(target);

if (lastFocusedItem !== this.focusedItem) {
Expand All @@ -191,8 +179,6 @@ export class Menu extends ActionBar {

this.mnemonics = new Map<string, Array<BaseMenuActionViewItem>>();

this.push(actions, { icon: true, label: true, isMenu: true });

// Scroll Logic
this.scrollableElement = this._register(new DomScrollableElement(menuElement, {
alwaysConsumeMouseWheel: true,
Expand All @@ -206,19 +192,15 @@ export class Menu extends ActionBar {
const scrollElement = this.scrollableElement.getDomNode();
scrollElement.style.position = '';

menuElement.style.maxHeight = `${Math.max(10, window.innerHeight - container.getBoundingClientRect().top - 30)}px`;
this._register(addDisposableListener(scrollElement, EventType.MOUSE_UP, e => {
// Absorb clicks in menu dead space https://github.com/Microsoft/vscode/issues/63575
// We do this on the scroll element so the scroll bar doesn't dismiss the menu either
e.preventDefault();
}));

this.menuDisposables.add(this.scrollableElement.onScroll(() => {
this._onScroll.fire();
}, this));
menuElement.style.maxHeight = `${Math.max(10, window.innerHeight - container.getBoundingClientRect().top - 30)}px`;

this._register(addDisposableListener(this.menuElement, EventType.SCROLL, (e: ScrollEvent) => {
if (this.scrollTopHold !== undefined) {
this.menuElement.scrollTop = this.scrollTopHold;
this.scrollTopHold = undefined;
}
this.scrollableElement.scanDomNode();
}));
this.push(actions, { icon: true, label: true, isMenu: true });
Copy link
Contributor Author

@jeanp413 jeanp413 Sep 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This change is necessary. this.scrollableElement must be created before calling this because now it's being used directly in get onScroll()


container.appendChild(this.scrollableElement.getDomNode());
this.scrollableElement.scanDomNode();
Expand Down Expand Up @@ -254,8 +236,8 @@ export class Menu extends ActionBar {
return this.scrollableElement.getDomNode();
}

get onScroll(): Event<void> {
return this._onScroll.event;
get onScroll(): Event<ScrollEvent> {
return this.scrollableElement.onScroll;
}

get scrollOffset(): number {
Expand Down Expand Up @@ -295,6 +277,19 @@ export class Menu extends ActionBar {
}
}

protected updateFocus(fromRight?: boolean): void {
super.updateFocus(fromRight, true);

if (typeof this.focusedItem !== 'undefined') {
// Workaround for #80047 caused by an issue in chromium
// https://bugs.chromium.org/p/chromium/issues/detail?id=414283
// When that's fixed, just call this.scrollableElement.scanDomNode()
this.scrollableElement.setScrollPosition({
scrollTop: Math.round(this.menuElement.scrollTop)
});
}
}

private doGetActionViewItem(action: IAction, options: IMenuOptions, parentData: ISubMenuData): BaseActionViewItem {
if (action instanceof Separator) {
return new MenuSeparatorActionViewItem(options.context, action, { icon: true });
Expand Down
4 changes: 4 additions & 0 deletions src/vs/base/browser/ui/menu/menubar.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,10 @@ export class MenuBar extends Disposable {
}));

this._register(DOM.addDisposableListener(buttonElement, DOM.EventType.MOUSE_UP, (e) => {
if (e.defaultPrevented) {
return;
}
Comment on lines +277 to +279
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sbatten I tested this and it fixes the issue for level 1 menus but it doesn't fix it for submenus with level > 1
I think we need to add the same check here

vscode/src/vs/base/browser/ui/menu/menu.ts

Lines 392 to 395 in 26cb1c8

this._register(addDisposableListener(this.element, EventType.MOUSE_UP, e => {
EventHelper.stop(e, true);
this.onClick(e);
}));

vscode/src/vs/base/browser/ui/menu/menubar.ts

Lines 371 to 379 in 26cb1c8

this._register(DOM.addDisposableListener(buttonElement, DOM.EventType.MOUSE_UP, (e) => {
if (!this.ignoreNextMouseUp) {
if (this.isFocused) {
this.onMenuTriggered(MenuBar.OVERFLOW_INDEX, true);
}
} else {
this.ignoreNextMouseUp = false;
}
}));


if (!this.ignoreNextMouseUp) {
if (this.isFocused) {
this.onMenuTriggered(menuIndex, true);
Expand Down