Skip to content

GAUD-10312 - No interaction with arrows when divider collapsed #7334

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

Closed
svanherk wants to merge 4 commits into from
Closed

GAUD-10312 - No interaction with arrows when divider collapsed #7334

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f7706ac
GAUD-10312 - Add vdiffs showcasing the divider arrows
svanherk Aug 7, 2026
575f9cb
GAUD-10312 - Arrows become inactive when divider is collapsed
svanherk Aug 7, 2026
7a2fe98
Merge branch 'main' into GAUD-10312-No-interaction-with-arrows-when-d…
svanherk Aug 7, 2026
7a5cc69
Updating vdiff goldens (#7335)
github-actions[bot] Aug 7, 2026
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
44 changes: 27 additions & 17 deletions components/page/page-divider-internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import '../colors/colors.js';
import '../icons/icon-custom.js';
import { css, html, LitElement, nothing } from 'lit';
import { classMap } from 'lit/directives/class-map.js';
import { FocusMixin } from '../../mixins/focus/focus-mixin.js';
import { formatPercent } from '@brightspace-ui/intl';
import { ifDefined } from 'lit/directives/if-defined.js';
Expand Down Expand Up @@ -141,7 +142,7 @@ class PageDivider extends FocusMixin(PropertyRequiredMixin(LitElement)) {
.divider-arrow {
align-items: center;
background-color: rgba(255, 255, 255, 0.5);
cursor: pointer;
cursor: default;
display: none;
height: 24px;
inset-block-start: max(50%, 97px); /* Do not hide behind slider on short screens */
Expand All @@ -150,16 +151,19 @@ class PageDivider extends FocusMixin(PropertyRequiredMixin(LitElement)) {
transform: translateY(-50%);
width: 24px;
}
.divider-arrow.active {
cursor: pointer;
}
.divider-arrow d2l-icon-custom {
color: var(--d2l-color-celestine);
}
.divider:focus-within .divider-arrow:not([hidden]) {
display: flex;
}
.divider-arrow:hover {
.divider-arrow.active:hover {
background-color: var(--d2l-color-gypsum);
}
.divider-arrow:hover d2l-icon-custom {
.divider-arrow.active:hover d2l-icon-custom {
color: var(--d2l-color-celestine-minus-1);
}
.divider-arrow.start {
Expand Down Expand Up @@ -205,6 +209,8 @@ class PageDivider extends FocusMixin(PropertyRequiredMixin(LitElement)) {

render() {
const { showStartArrow, showEndArrow } = this.#getArrowVisibility();
const startArrowClasses = { 'divider-arrow': true, 'start': true, 'active': !this.collapsed };
const endArrowClasses = { 'divider-arrow': true, 'end': true, 'active': !this.collapsed };
let ariaValues = {};
if (this.maxSize > 0) {
ariaValues = { max: this.maxSize, min: 0, now: this.currentSize, text: formatPercent(this.currentSize / this.maxSize, { maximumFractionDigits: 0 }) };
Expand All @@ -213,10 +219,10 @@ class PageDivider extends FocusMixin(PropertyRequiredMixin(LitElement)) {
return html`
<div class="divider" @click="${this.#handleClick}" @pointerdown="${this.#handlePointerDown}">
${this.panelType === 'panel' ? html`
<div class="divider-arrow start" data-position="start" ?hidden="${!showStartArrow}">
<div class="${classMap(startArrowClasses)}" data-position="start" ?hidden="${!showStartArrow}">
<d2l-icon-custom size="tier1">${ICON_ARROW_COLLAPSE_LEFT}</d2l-icon-custom>
</div>
<div class="divider-arrow end" data-position="end" ?hidden="${!showEndArrow}">
<div class="${classMap(endArrowClasses)}" data-position="end" ?hidden="${!showEndArrow}">
<d2l-icon-custom size="tier1">${ICON_ARROW_COLLAPSE_RIGHT}</d2l-icon-custom>
</div>
` : nothing}
Expand All @@ -241,6 +247,7 @@ class PageDivider extends FocusMixin(PropertyRequiredMixin(LitElement)) {
`;
}

#clickedArrow;
#clickedHandle = false;

#getArrowVisibility() {
Expand All @@ -265,10 +272,20 @@ class PageDivider extends FocusMixin(PropertyRequiredMixin(LitElement)) {
}

#handleClick(e) {
// Do not toggle until click event is received, to avoid clicking on elements under the handle in drawer mode
// Do not toggle/resize until click event is received,
// to avoid clicking on elements under the arrows in overlay mode or under the handle in drawer mode
e.stopPropagation();
if (this.collapsed || this.#clickedHandle) {
if (this.collapsed && this.#clickedArrow) {
return;
} else if (this.collapsed || this.#clickedHandle) {
this.#sendToggleEvent();
} else if (this.#clickedArrow) {
const endArrowClicked = this.#clickedArrow.dataset.position === 'end';
const shouldGrow = endArrowClicked === (this.panelPosition === 'start');

const step = (shouldGrow ? 1 : -1) * KEYBOARD_STEP;
const requestedSize = this.currentSize + step;
this.#sendResizeEvent(requestedSize);
}
}

Expand Down Expand Up @@ -313,17 +330,10 @@ class PageDivider extends FocusMixin(PropertyRequiredMixin(LitElement)) {

const path = e.composedPath();
this.#clickedHandle = path.some(el => el.classList?.contains('divider-handle'));
this.#clickedArrow = path.find(el => el.classList?.contains('divider-arrow'));
Comment on lines 332 to +333

@svanherk svanherk Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

One thing I wanted to call out here - this is a maybe a tad confusing, because one stores a boolean and one stores the actual arrow (since I need it later). I could store the handle to be consistent, but I don't need it. I could use two variables (one to store that the arrow was clicked, and one to store the arrow) but that felt like overkill. Or I could update the naming, but I liked how this flowed 🤷‍♀️

if (this.#clickedArrow) return; // Arrows don't support dragging

const clickedArrow = path.find(el => el.classList?.contains('divider-arrow'));
if (clickedArrow) {

@svanherk svanherk Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This code moving into the click handler above fixes an eventual bug that would have come up with overlay. It's the same issue the drawer would have had - if we move the divider before the click happens, it'll click what's behind it. I thought this was only an issue with the drawer handle, but it also happens to the arrows in overlay mode, where clicking the overlay closes the drawer.

Even if we don't want that to happen (I need to ask design), it's just better and will likely save us from some other bug in the future.

const endArrowClicked = clickedArrow.dataset.position === 'end';
const shouldGrow = endArrowClicked === (this.panelPosition === 'start');

const step = (shouldGrow ? 1 : -1) * KEYBOARD_STEP;
const requestedSize = this.currentSize + step;
this.#sendResizeEvent(requestedSize);
return;
}
// TO DO: Dragging
}

#sendResizeEvent(requestedSize) {
Expand Down
Binary file modified ...e/test/golden/page-divider-internal/chromium/hover-collapsed-arrow-side-nav.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified ...n/page-divider-internal/chromium/hover-collapsed-arrow-supporting-immersive.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
26 changes: 21 additions & 5 deletions components/page/test/page-divider-internal.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,18 @@ describe('d2l-page-divider-internal', () => {
clickElem(elem);
await oneEvent(elem, 'd2l-page-divider-toggle');
});

it('does not dispatch event when divider arrow is clicked if collapsed', async() => {
const elem = await fixture(createDivider({ collapsed: true }));
const arrow = getDividerArrow(elem, 'end');
expect(arrow.hidden).to.be.false;

await focusElem(elem);
let dispatched = false;
elem.addEventListener('d2l-page-divider-toggle', () => dispatched = true);
await clickElem(arrow);
expect(dispatched).to.be.false;
});
});

});
Expand Down Expand Up @@ -211,15 +223,19 @@ describe('d2l-page-divider-internal', () => {
expect(arrowElem.hidden).to.be.true;
});

it('grow arrow appears and requests a resize to min size when collapsed', async() => {
it('grow arrow appears when collapsed but does not dispatch event on click', async() => {
const elem = await fixture(
createDivider({ collapsed: true, currentSize: 0, panelPosition: test.panelPosition }),
createDivider({ collapsed: true, currentSize, panelPosition: test.panelPosition }),
{ rtl: test.rtl }
);
const arrowElem = getDividerArrow(elem, test.growArrow);
expect(arrowElem.hidden).to.be.false;

await focusElem(elem);
clickElem(getDividerArrow(elem, test.growArrow));
const e = await oneEvent(elem, 'd2l-page-divider-resize');
expect(e.detail.requestedSize).to.equal(minSize);
let dispatched = false;
elem.addEventListener('d2l-page-divider-resize', () => dispatched = true);
await clickElem(elem);
expect(dispatched).to.be.false;
});
});
});
Expand Down