Skip to content

Commit

Permalink
Fix scrolling behavior in menu while zoomed in (#80965)
Browse files Browse the repository at this point in the history
* Fix scrolling behavior in menu while zoomed. Fixes #80047

* 💄

* move scrollbar click detection into menu

* confine preventScroll change to menu

* fix sticky scrollbar
  • Loading branch information
jeanp413 authored and sbatten committed Oct 17, 2019
1 parent 240662d commit d45eaa2
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 31 deletions.
6 changes: 3 additions & 3 deletions src/vs/base/browser/ui/actionbar/actionbar.ts
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
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 });

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
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;
}

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

0 comments on commit d45eaa2

Please sign in to comment.