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.

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

Jump to bottom

Remove z-index and stacking contexts from list-items #3775

Merged
merged 6 commits into from
Jun 27, 2023
Merged

Remove z-index and stacking contexts from list-items #3775

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1eaf853
Switch list-item z-index to grid order
bearfriend Jun 22, 2023
ca20581
Remove fullscreenWithin, dropdownOpen, and tooltipShowing
bearfriend Jun 22, 2023
1572345
Fix lint
bearfriend Jun 22, 2023
a03c62c
Remove fullscreen-within listener
bearfriend Jun 22, 2023
97fdf59
Try disabling pointer events
bearfriend Jun 22, 2023
42a188d
Fix lint
bearfriend Jun 22, 2023
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
15 changes: 7 additions & 8 deletions components/list/list-item-generic-layout.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,6 @@ class ListItemGenericLayout extends RtlMixin(LitElement) {
[end actions-end];
grid-template-rows: [main-start] [main-end nested-start] [nested-end];
position: relative;
z-index: 0;
}

::slotted([slot="nested"]) {
Expand All @@ -80,15 +79,15 @@ class ListItemGenericLayout extends RtlMixin(LitElement) {
grid-column: control-start / end;
}
:host(.d2l-dragging-over) ::slotted([slot="nested"]) {
z-index: 7; /* must be greater than item's drop-target to allow dropping onto items within nested list */
order: 7; /* must be greater than item's drop-target to allow dropping onto items within nested list */
}

::slotted([slot="drop-target"]) {
height: 100%;
order: 6;
position: absolute;
top: 0;
width: 100%;
z-index: 6;
}

::slotted([slot="outside-control"]),
Expand All @@ -105,7 +104,7 @@ class ListItemGenericLayout extends RtlMixin(LitElement) {
::slotted([slot="expand-collapse"]) {
cursor: pointer;
grid-column: expand-collapse-start / expand-collapse-end;
z-index: 2;
order: 2;
}

::slotted([slot="control"]) {
Expand All @@ -124,7 +123,7 @@ class ListItemGenericLayout extends RtlMixin(LitElement) {
::slotted([slot="actions"]) {
grid-column: actions-start / actions-end;
justify-self: end;
z-index: 5;
order: 5;
}

::slotted([slot="outside-control-action"]),
Expand All @@ -134,23 +133,23 @@ class ListItemGenericLayout extends RtlMixin(LitElement) {
}
::slotted([slot="outside-control-action"]) {
grid-column: start / end;
z-index: 1;
order: 1;
}
:host([no-primary-action]) ::slotted([slot="outside-control-action"]) {
grid-column: start / outside-control-end;
}
::slotted([slot="control-action"]) {
grid-column: control-start / end;
height: 100%;
order: 3;
width: 100%;
z-index: 3;
}
:host([no-primary-action]) ::slotted([slot="control-action"]) {
grid-column: control-start / control-end;
}
::slotted([slot="content-action"]) {
grid-column: content-start / end;
z-index: 4;
order: 4;
}
:host([no-primary-action]) ::slotted([slot="content-action"]) {
display: none;
Expand Down
32 changes: 2 additions & 30 deletions components/list/list-item-mixin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,15 +84,12 @@ export const ListItemMixin = superclass => class extends composeMixins(
paddingType: { type: String, attribute: 'padding-type' },
_breakpoint: { type: Number },
_displayKeyboardTooltip: { type: Boolean },
_dropdownOpen: { type: Boolean, attribute: '_dropdown-open', reflect: true },
_fullscreenWithin: { type: Boolean, attribute: '_fullscreen-within', reflect: true },
_hovering: { type: Boolean, reflect: true },
_hoveringPrimaryAction: { type: Boolean, attribute: '_hovering-primary-action', reflect: true },
_focusing: { type: Boolean, reflect: true },
_focusingPrimaryAction: { type: Boolean, attribute: '_focusing-primary-action', reflect: true },
_highlight: { type: Boolean, reflect: true },
_highlighting: { type: Boolean, reflect: true },
_tooltipShowing: { type: Boolean, attribute: '_tooltip-showing', reflect: true },
_hasNestedList: { state: true }
};
}
Expand All @@ -108,15 +105,6 @@ export const ListItemMixin = superclass => class extends composeMixins(
display: none;
}

:host([_tooltip-showing]),
:host([_dropdown-open]) {
z-index: 10; /* must be greater than adjacent selected items (if this is increased, d2l-collapsible-panel must be updated too) */
}
:host([_fullscreen-within]) {
position: fixed; /* required for Safari */
z-index: 998; /* must be greater than floating workflow buttons */
}

:host([dragging]) d2l-list-item-generic-layout {
filter: grayscale(75%);
opacity: 0.4;
Expand All @@ -126,9 +114,10 @@ export const ListItemMixin = superclass => class extends composeMixins(
}

[slot="control-container"] {
pointer-events: none;
Copy link
Contributor Author

@bearfriend bearfriend Jun 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

position: relative; (necessary for the absolutely positioned borders below) seems to bump this item out of control of order, so I left it on top but disallowed pointer-events, which seems to work fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

No objection to changing this, just calling this out if we run into any issues with it. I don't think this z-index value was causing us any grief. It was just being used for rendering the separators in its ::before/::after pseudo-elements. The element itself is not actually a container for item content, and so would not be setting a stacking context for anything else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem was that without the parent stacking context, -1 puts it behind the list itself (or whatever it eventually hits) so the borders were no longer visible.

position: relative;
z-index: -1; /* must allow for interactive content to be accessible with mouse */
}

:host(:first-of-type) [slot="control-container"]::before,
[slot="control-container"]::after {
border-top: 1px solid var(--d2l-color-mica);
Expand Down Expand Up @@ -389,8 +378,6 @@ export const ListItemMixin = superclass => class extends composeMixins(
this._breakpoint = 0;
this._contentId = getUniqueId();
this._displayKeyboardTooltip = false;
this._fullscreenWithin = false;
this._fullscreenWithinCount = 0;
this._hasNestedList = false;
}

Expand Down Expand Up @@ -421,14 +408,6 @@ export const ListItemMixin = superclass => class extends composeMixins(
ro.unobserve(this);
}

firstUpdated(changedProperties) {
this.addEventListener('d2l-dropdown-open', () => this._dropdownOpen = true);
this.addEventListener('d2l-dropdown-close', () => this._dropdownOpen = false);
this.addEventListener('d2l-tooltip-show', () => this._tooltipShowing = true);
this.addEventListener('d2l-tooltip-hide', () => this._tooltipShowing = false);
super.firstUpdated(changedProperties);
}

updated(changedProperties) {
super.updated(changedProperties);
if (changedProperties.has('breakpoints')) {
Expand Down Expand Up @@ -579,12 +558,6 @@ export const ListItemMixin = superclass => class extends composeMixins(
this._focusingPrimaryAction = false;
}

_onFullscreenWithin(e) {
if (e.detail.state) this._fullscreenWithinCount += 1;
else this._fullscreenWithinCount -= 1;
this._fullscreenWithin = (this._fullscreenWithinCount > 0);
}

_onMouseEnter() {
this._hovering = true;
}
Expand Down Expand Up @@ -629,7 +602,6 @@ export const ListItemMixin = superclass => class extends composeMixins(
align-nested="${ifDefined(this.draggable && this.selectable ? 'control' : undefined)}"
@focusin="${this._onFocusIn}"
@focusout="${this._onFocusOut}"
@d2l-fullscreen-within="${this._onFullscreenWithin}"
class="${classMap(classes)}"
data-breakpoint="${this._breakpoint}"
data-separators="${ifDefined(this._separators)}"
Expand Down