Fix conflicting scroll instructions in Breadcrumbs
The flakey tests on Windows exposed a bug where:
* We render and click the right button to scroll
* The code that ensures the active node is in view runs, and tries to
scroll the active element into view.
These two actions would conflict with each other. On Linux/Mac and our Windows
bots, it seems the right scroll "wins" and the tests pass. However, on Windows
10 the right scroll is overriden by scrolling the active node into view, and
therefore the test fails.
The fix we landed on is to flag if the user has manually scrolled the
crumbs, and gate the scrolling active node into view logic on that. If
the user manually scrolls, we effectively cede control and let them
control the scrolling. When we get an update (e.g. a new active selected
node), we take back control.
Additionally, the resize observer was being far too aggressive in its behaviour
(just trigger a re-render), so that's been updated to check explicitly just if
the scroll buttons need to be hidden/shown after a render. This caused a bug
when running tests in Windows where it would also cause scroll conflicts. It's
not a bug I think users would ever have hit, but one that the tests hit because
they do things so much quicker than a user actually would (e.g. render + click
button in same frame). There are tests added for the resize observer behaviour.
Change-Id: Iedf4cbe5bef652d16fc80a0e7c0b3b45a95ebbfa
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2449984
Reviewed-by: Paul Lewis <[email protected]>
Commit-Queue: Jack Franklin <[email protected]>
diff --git a/front_end/elements/ElementsBreadcrumbs.ts b/front_end/elements/ElementsBreadcrumbs.ts
index 2b93bfb..4d5fd5d 100644
--- a/front_end/elements/ElementsBreadcrumbs.ts
+++ b/front_end/elements/ElementsBreadcrumbs.ts
@@ -17,17 +17,19 @@
}
export class ElementsBreadcrumbs extends HTMLElement {
private readonly shadow = this.attachShadow({mode: 'open'});
- private readonly resizeObserver = new ResizeObserver(() => this.update());
+ private readonly resizeObserver = new ResizeObserver(() => this.checkForOverflowOnResize());
private crumbsData: ReadonlyArray<DOMNode> = [];
private selectedDOMNode: Readonly<DOMNode>|null = null;
private overflowing = false;
private userScrollPosition: UserScrollPosition = 'start';
private isObservingResize = false;
+ private userHasManuallyScrolled = false;
set data(data: ElementsBreadcrumbsData) {
this.selectedDOMNode = data.selectedNode;
this.crumbsData = data.crumbs;
+ this.userHasManuallyScrolled = false;
this.update();
}
@@ -43,6 +45,34 @@
};
}
+ /*
+ * When the window is resized, we need to check if we either:
+ * 1) overflowing, and now the window is big enough that we don't need to
+ * 2) not overflowing, and now the window is small and we do need to
+ *
+ * If either of these are true, we toggle the overflowing state accordingly and trigger a re-render.
+ */
+ private checkForOverflowOnResize() {
+ const wrappingElement = this.shadow.querySelector('.crumbs');
+ const crumbs = this.shadow.querySelector('.crumbs-scroll-container');
+ if (!wrappingElement || !crumbs) {
+ return;
+ }
+
+ const totalContainingWidth = wrappingElement.clientWidth;
+ const totalCrumbsWidth = crumbs.clientWidth;
+
+ if (totalCrumbsWidth >= totalContainingWidth && this.overflowing === false) {
+ this.overflowing = true;
+ this.userScrollPosition = 'start';
+ this.render();
+ } else if (totalCrumbsWidth < totalContainingWidth && this.overflowing === true) {
+ this.overflowing = false;
+ this.userScrollPosition = 'start';
+ this.render();
+ }
+ }
+
private update() {
this.overflowing = false;
this.userScrollPosition = 'start';
@@ -157,6 +187,7 @@
private onOverflowClick(direction: 'left'|'right') {
return () => {
+ this.userHasManuallyScrolled = true;
const scrollWindow = this.shadow.querySelector('.crumbs-window');
if (!scrollWindow) {
@@ -309,14 +340,26 @@
}
private ensureSelectedNodeIsVisible() {
- if (!this.selectedDOMNode || !this.shadow || !this.overflowing) {
+ /*
+ * If the user has manually scrolled the crumbs in either direction, we
+ * effectively hand control over the scrolling down to them. This is to
+ * prevent the user manually scrolling to the end, and then us scrolling
+ * them back to the selected node. The moment they click either scroll
+ * button we set userHasManuallyScrolled, and we reset it when we get new
+ * data in. This means if the user clicks on a different element in the
+ * tree, we will auto-scroll that element into view, because we'll get new
+ * data and hence the flag will be reset.
+ */
+ if (!this.selectedDOMNode || !this.shadow || !this.overflowing || this.userHasManuallyScrolled) {
return;
}
const activeCrumbId = this.selectedDOMNode.id;
const activeCrumb = this.shadow.querySelector(`.crumb[data-node-id="${activeCrumbId}"]`);
if (activeCrumb) {
- activeCrumb.scrollIntoView();
+ activeCrumb.scrollIntoView({
+ behavior: 'smooth',
+ });
}
}
}
