From 25e9c2ee4e9608e0b378fb9ba86bf4e5229f0fb1 Mon Sep 17 00:00:00 2001 From: jeanp413 Date: Mon, 16 Sep 2019 03:08:26 -0500 Subject: [PATCH 1/5] Fix scrolling behavior in menu while zoomed. Fixes #80047 --- src/vs/base/browser/ui/actionbar/actionbar.ts | 4 +- src/vs/base/browser/ui/menu/menu.ts | 67 +++++++------------ src/vs/base/browser/ui/menu/menubar.ts | 7 ++ 3 files changed, 33 insertions(+), 45 deletions(-) diff --git a/src/vs/base/browser/ui/actionbar/actionbar.ts b/src/vs/base/browser/ui/actionbar/actionbar.ts index 538f7fa0dbe34..a45158123b004 100644 --- a/src/vs/base/browser/ui/actionbar/actionbar.ts +++ b/src/vs/base/browser/ui/actionbar/actionbar.ts @@ -747,7 +747,7 @@ export class ActionBar extends Disposable implements IActionRunner { protected updateFocus(fromRight?: boolean): void { if (typeof this.focusedItem === 'undefined') { - this.actionsList.focus(); + this.actionsList.focus({ preventScroll: true }); } for (let i = 0; i < this.viewItems.length; i++) { @@ -759,7 +759,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: true }); } } } else { diff --git a/src/vs/base/browser/ui/menu/menu.ts b/src/vs/base/browser/ui/menu/menu.ts index 49d35fd57a804..1f67b0b51c285 100644 --- a/src/vs/base/browser/ui/menu/menu.ts +++ b/src/vs/base/browser/ui/menu/menu.ts @@ -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'; @@ -60,9 +60,6 @@ export class Menu extends ActionBar { private readonly menuDisposables: DisposableStore; private scrollableElement: DomScrollableElement; private menuElement: HTMLElement; - private scrollTopHold: number | undefined; - - private readonly _onScroll: Emitter; constructor(container: HTMLElement, actions: ReadonlyArray, options: IMenuOptions = {}) { addClass(container, 'monaco-menu-container'); @@ -82,8 +79,6 @@ export class Menu extends ActionBar { this.menuElement = menuElement; - this._onScroll = this._register(new Emitter()); - this.actionsList.setAttribute('role', 'menu'); this.actionsList.tabIndex = 0; @@ -147,7 +142,6 @@ 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(); } @@ -170,7 +164,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) { @@ -185,7 +178,7 @@ export class Menu extends ActionBar { this.mnemonics = new Map>(); - this.push(actions, { icon: true, label: true, isMenu: true }); + menuElement.style.maxHeight = `${Math.max(10, window.innerHeight - container.getBoundingClientRect().top - 30)}px`; // Scroll Logic this.scrollableElement = this._register(new DomScrollableElement(menuElement, { @@ -200,26 +193,14 @@ export class Menu extends ActionBar { const scrollElement = this.scrollableElement.getDomNode(); scrollElement.style.position = null; - menuElement.style.maxHeight = `${Math.max(10, window.innerHeight - container.getBoundingClientRect().top - 30)}px`; - - this.menuDisposables.add(this.scrollableElement.onScroll(() => { - this._onScroll.fire(); - }, this)); + this.viewItems.filter(item => !(item instanceof MenuSeparatorActionViewItem)).forEach((item: BaseMenuActionViewItem, index: number, array: any[]) => { + item.updatePositionInSet(index + 1, array.length); + }); - 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(); - - this.viewItems.filter(item => !(item instanceof MenuSeparatorActionViewItem)).forEach((item: BaseMenuActionViewItem, index: number, array: any[]) => { - item.updatePositionInSet(index + 1, array.length); - }); } style(style: IMenuStyles): void { @@ -248,8 +229,8 @@ export class Menu extends ActionBar { return this.scrollableElement.getDomNode(); } - get onScroll(): Event { - return this._onScroll.event; + get onScroll(): Event { + return this.scrollableElement.onScroll; } get scrollOffset(): number { @@ -289,6 +270,19 @@ export class Menu extends ActionBar { } } + protected updateFocus(fromRight?: boolean): void { + super.updateFocus(fromRight); + + 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 }); @@ -666,8 +660,7 @@ class SubmenuMenuActionViewItem extends BaseMenuActionViewItem { })); this._register(this.parentData.parent.onScroll(() => { - this.parentData.parent.focus(false); - this.cleanupExistingSubmenu(false); + this.cleanupExistingSubmenu(true); })); } @@ -730,13 +723,7 @@ class SubmenuMenuActionViewItem extends BaseMenuActionViewItem { this.parentData.parent.focus(); - if (this.parentData.submenu) { - this.parentData.submenu.dispose(); - this.parentData.submenu = undefined; - } - - this.submenuDisposables.clear(); - this.submenuContainer = undefined; + this.cleanupExistingSubmenu(true); } })); @@ -751,13 +738,7 @@ class SubmenuMenuActionViewItem extends BaseMenuActionViewItem { this.submenuDisposables.add(this.parentData.submenu.onDidCancel(() => { this.parentData.parent.focus(); - if (this.parentData.submenu) { - this.parentData.submenu.dispose(); - this.parentData.submenu = undefined; - } - - this.submenuDisposables.clear(); - this.submenuContainer = undefined; + this.cleanupExistingSubmenu(true); })); this.parentData.submenu.focus(selectFirstItem); diff --git a/src/vs/base/browser/ui/menu/menubar.ts b/src/vs/base/browser/ui/menu/menubar.ts index 960dc57e267e9..17faf6a821131 100644 --- a/src/vs/base/browser/ui/menu/menubar.ts +++ b/src/vs/base/browser/ui/menu/menubar.ts @@ -262,6 +262,13 @@ export class MenuBar extends Disposable { })); this._register(DOM.addDisposableListener(buttonElement, DOM.EventType.MOUSE_UP, (e) => { + const target = e.target as HTMLElement; + + // If target is scrollbar, do nothing + if (target.classList.contains('slider')) { + return; + } + if (!this.ignoreNextMouseUp) { if (this.isFocused) { this.onMenuTriggered(menuIndex, true); From 1a4ca1a5c6394da129f4faf7ab5b528cc878a5d1 Mon Sep 17 00:00:00 2001 From: jeanp413 Date: Thu, 17 Oct 2019 03:51:28 -0500 Subject: [PATCH 2/5] :lipstick: --- src/vs/base/browser/ui/menu/menu.ts | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/src/vs/base/browser/ui/menu/menu.ts b/src/vs/base/browser/ui/menu/menu.ts index 74af484b56d5a..a78a3c4cad9a2 100644 --- a/src/vs/base/browser/ui/menu/menu.ts +++ b/src/vs/base/browser/ui/menu/menu.ts @@ -184,8 +184,6 @@ export class Menu extends ActionBar { this.mnemonics = new Map>(); - menuElement.style.maxHeight = `${Math.max(10, window.innerHeight - container.getBoundingClientRect().top - 30)}px`; - // Scroll Logic this.scrollableElement = this._register(new DomScrollableElement(menuElement, { alwaysConsumeMouseWheel: true, @@ -199,14 +197,16 @@ export class Menu extends ActionBar { const scrollElement = this.scrollableElement.getDomNode(); scrollElement.style.position = ''; - this.viewItems.filter(item => !(item instanceof MenuSeparatorActionViewItem)).forEach((item: BaseMenuActionViewItem, index: number, array: any[]) => { - item.updatePositionInSet(index + 1, array.length); - }); + menuElement.style.maxHeight = `${Math.max(10, window.innerHeight - container.getBoundingClientRect().top - 30)}px`; this.push(actions, { icon: true, label: true, isMenu: true }); container.appendChild(this.scrollableElement.getDomNode()); this.scrollableElement.scanDomNode(); + + this.viewItems.filter(item => !(item instanceof MenuSeparatorActionViewItem)).forEach((item: BaseMenuActionViewItem, index: number, array: any[]) => { + item.updatePositionInSet(index + 1, array.length); + }); } style(style: IMenuStyles): void { @@ -699,7 +699,8 @@ class SubmenuMenuActionViewItem extends BaseMenuActionViewItem { })); this._register(this.parentData.parent.onScroll(() => { - this.cleanupExistingSubmenu(true); + this.parentData.parent.focus(false); + this.cleanupExistingSubmenu(false); })); } @@ -772,7 +773,13 @@ class SubmenuMenuActionViewItem extends BaseMenuActionViewItem { this.parentData.parent.focus(); - this.cleanupExistingSubmenu(true); + if (this.parentData.submenu) { + this.parentData.submenu.dispose(); + this.parentData.submenu = undefined; + } + + this.submenuDisposables.clear(); + this.submenuContainer = undefined; } })); @@ -787,7 +794,13 @@ class SubmenuMenuActionViewItem extends BaseMenuActionViewItem { this.submenuDisposables.add(this.parentData.submenu.onDidCancel(() => { this.parentData.parent.focus(); - this.cleanupExistingSubmenu(true); + if (this.parentData.submenu) { + this.parentData.submenu.dispose(); + this.parentData.submenu = undefined; + } + + this.submenuDisposables.clear(); + this.submenuContainer = undefined; })); this.parentData.submenu.focus(selectFirstItem); From 4c434daa39ddf56601b7b0b18ecf21f4dc35a3cc Mon Sep 17 00:00:00 2001 From: SteVen Batten Date: Thu, 17 Oct 2019 10:59:01 -0700 Subject: [PATCH 3/5] move scrollbar click detection into menu --- src/vs/base/browser/ui/menu/menu.ts | 11 ++++++----- src/vs/base/browser/ui/menu/menubar.ts | 7 ------- 2 files changed, 6 insertions(+), 12 deletions(-) diff --git a/src/vs/base/browser/ui/menu/menu.ts b/src/vs/base/browser/ui/menu/menu.ts index a78a3c4cad9a2..2d63eda018983 100644 --- a/src/vs/base/browser/ui/menu/menu.ts +++ b/src/vs/base/browser/ui/menu/menu.ts @@ -153,11 +153,6 @@ export class Menu extends ActionBar { } })); - 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) { @@ -197,6 +192,12 @@ export class Menu extends ActionBar { const scrollElement = this.scrollableElement.getDomNode(); scrollElement.style.position = ''; + 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 + EventHelper.stop(e, true); + })); + menuElement.style.maxHeight = `${Math.max(10, window.innerHeight - container.getBoundingClientRect().top - 30)}px`; this.push(actions, { icon: true, label: true, isMenu: true }); diff --git a/src/vs/base/browser/ui/menu/menubar.ts b/src/vs/base/browser/ui/menu/menubar.ts index 5167fdb75dd02..4ba4fff01bb1e 100644 --- a/src/vs/base/browser/ui/menu/menubar.ts +++ b/src/vs/base/browser/ui/menu/menubar.ts @@ -274,13 +274,6 @@ export class MenuBar extends Disposable { })); this._register(DOM.addDisposableListener(buttonElement, DOM.EventType.MOUSE_UP, (e) => { - const target = e.target as HTMLElement; - - // If target is scrollbar, do nothing - if (target.classList.contains('slider')) { - return; - } - if (!this.ignoreNextMouseUp) { if (this.isFocused) { this.onMenuTriggered(menuIndex, true); From 82b0b321c5ff7498789055b7cdde21750baac0d7 Mon Sep 17 00:00:00 2001 From: SteVen Batten Date: Thu, 17 Oct 2019 11:16:37 -0700 Subject: [PATCH 4/5] confine preventScroll change to menu --- src/vs/base/browser/ui/actionbar/actionbar.ts | 6 +++--- src/vs/base/browser/ui/menu/menu.ts | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/vs/base/browser/ui/actionbar/actionbar.ts b/src/vs/base/browser/ui/actionbar/actionbar.ts index 4496379fde7ed..eea4040338184 100644 --- a/src/vs/base/browser/ui/actionbar/actionbar.ts +++ b/src/vs/base/browser/ui/actionbar/actionbar.ts @@ -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({ preventScroll: true }); + this.actionsList.focus({ preventScroll }); } for (let i = 0; i < this.viewItems.length; i++) { @@ -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({ preventScroll: true }); + this.actionsList.focus({ preventScroll }); } } } else { diff --git a/src/vs/base/browser/ui/menu/menu.ts b/src/vs/base/browser/ui/menu/menu.ts index 2d63eda018983..482396ce72d93 100644 --- a/src/vs/base/browser/ui/menu/menu.ts +++ b/src/vs/base/browser/ui/menu/menu.ts @@ -278,7 +278,7 @@ export class Menu extends ActionBar { } protected updateFocus(fromRight?: boolean): void { - super.updateFocus(fromRight); + super.updateFocus(fromRight, true); if (typeof this.focusedItem !== 'undefined') { // Workaround for #80047 caused by an issue in chromium From f6d61f08712511522b8800ab5616ccebed307855 Mon Sep 17 00:00:00 2001 From: SteVen Batten Date: Thu, 17 Oct 2019 14:47:53 -0700 Subject: [PATCH 5/5] fix sticky scrollbar --- src/vs/base/browser/ui/menu/menu.ts | 2 +- src/vs/base/browser/ui/menu/menubar.ts | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/vs/base/browser/ui/menu/menu.ts b/src/vs/base/browser/ui/menu/menu.ts index 482396ce72d93..cda0d9db4068d 100644 --- a/src/vs/base/browser/ui/menu/menu.ts +++ b/src/vs/base/browser/ui/menu/menu.ts @@ -195,7 +195,7 @@ export class Menu extends ActionBar { 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 - EventHelper.stop(e, true); + e.preventDefault(); })); menuElement.style.maxHeight = `${Math.max(10, window.innerHeight - container.getBoundingClientRect().top - 30)}px`; diff --git a/src/vs/base/browser/ui/menu/menubar.ts b/src/vs/base/browser/ui/menu/menubar.ts index 4ba4fff01bb1e..8018637cde0ff 100644 --- a/src/vs/base/browser/ui/menu/menubar.ts +++ b/src/vs/base/browser/ui/menu/menubar.ts @@ -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);