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

Fix scrolling behavior in menu while zoomed in #80965

Merged
merged 9 commits into from Oct 17, 2019

Conversation

@jeanp413
Copy link
Contributor

jeanp413 commented Sep 16, 2019

Fixes #80047

@jeanp413 jeanp413 changed the title Fix scrolling behavior in menu while zoomed Fix scrolling behavior in menu while zoomed in Sep 16, 2019
@sbatten

This comment has been minimized.

Copy link
Member

sbatten commented Sep 27, 2019

@jeanp413 thanks for the PR. and thanks for the pointer about the chrome issue. I notice some of the changes are just rearranging of lines and additionally some changes in the submenuviewitem. Are these changes strictly necessary to address the issue? If not, I would like to keep this PR to the minimum amount of changes.

Additionally, some of the changes are in actionbar, which I am hesitant to take anything that affects more than the menu widget. So, it might be better to pull more of that code up a layer.

@sbatten sbatten added this to the October 2019 milestone Sep 27, 2019
@@ -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 });

This comment has been minimized.

Copy link
@jeanp413

jeanp413 Sep 28, 2019

Author Contributor

The { preventScroll: true } is necessary to avoid scrolling up when focusing on the actions-container because of the padding from monaco-action-bar animated vertical

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

This comment has been minimized.

Copy link
@jeanp413

jeanp413 Sep 28, 2019

Author Contributor

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

jeanp413 and others added 2 commits Oct 13, 2019
@sbatten

This comment has been minimized.

Copy link
Member

sbatten commented Oct 17, 2019

@jeanp413 If it isn't necessary to fix the original issue, let's revert it. Testing it, I seet that it works and once we have it to a minimum we can see if we need to worry about the code change in actionbar.ts

@sbatten sbatten self-requested a review Oct 17, 2019
Copy link
Member

sbatten left a comment

as mentioned, let's focus the changes strictly to what's necessary

@jeanp413

This comment has been minimized.

Copy link
Contributor Author

jeanp413 commented Oct 17, 2019

Reverted some changes. Let me know if that's enough

@sbatten

This comment has been minimized.

Copy link
Member

sbatten commented Oct 17, 2019

@jeanp413 This is looking great, I hope you don't mind but I moved the logic you had in menubar.ts to avoid dismissing the menu on scrollbar clicking into menu.ts, since those clicks should not be propagated.

I also made one additional minor change to ensure the prevent scroll behavior doesn't break other instances of actionbar.

Please take a look and see if you agree with my changes.

@sbatten sbatten self-requested a review Oct 17, 2019
@sbatten sbatten added the menus label Oct 17, 2019
@jeanp413

This comment has been minimized.

Copy link
Contributor Author

jeanp413 commented Oct 17, 2019

@sbatten yeah it's great, but I found a minor issue when you click on the scrollbar it doesn't stop tracking it. Seems like it's necessary to let the event propagate because it's used here

for (const element of windowChain) {
this.hooks.add(dom.addDisposableThrottledListener(element.window.document, 'mousemove',
(data: R) => this.mouseMoveCallback!(data),
(lastEvent: R, currentEvent) => this.mouseMoveEventMerger!(lastEvent, currentEvent as MouseEvent)
));
this.hooks.add(dom.addDisposableListener(element.window.document, 'mouseup', (e: MouseEvent) => this.stopMonitoring(true)));
}

sbatten added 2 commits Oct 17, 2019
@sbatten

This comment has been minimized.

Copy link
Member

sbatten commented Oct 17, 2019

@jeanp413 good catch, updated and merging

@sbatten sbatten merged commit d45eaa2 into microsoft:master Oct 17, 2019
if (e.defaultPrevented) {
return;
}
Comment on lines +277 to +279

This comment has been minimized.

Copy link
@jeanp413

jeanp413 Oct 18, 2019

Author Contributor

@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

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

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

@jeanp413 jeanp413 deleted the jeanp413:fix-menu-scroll-zoom branch Oct 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.