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: Safari list item slot order #3808

Merged
merged 6 commits into from
Jul 7, 2023
Merged

Conversation

bearfriend
Copy link
Contributor

DE54029: Safari list-item slot order

After #3775, list items were broken in Safari because it renders the slots out of order in a very strange way. After giving each slot an explicit order CSS property or rearranging the DOM, Safari renders the slots in the correct order visually, but attempting to interact with the slots that were on top both in the DOM and visually still did not work, with pointer events being sent to the slots behind for no apparent reason.

To fix this, I needed to both re-order the slots in the DOM and strategically disable pointer events or change the columns on slots that should not be interactive in each scenario. This allows the order to be purely DOM-based, without order or z-index.

I also did some pretty minor cleanup of things that seemed unnecessary or poorly named.

@bearfriend bearfriend requested a review from a team as a code owner July 6, 2023 13:37
@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2023

Thanks for the PR! 🎉

We've deployed an automatic preview for this PR - you can see your changes here:

URL https://pr-3808-brightspace-ui-core.d2l.dev/

Note
The build needs to finish before your changes are deployed.
Changes to the PR will automatically update the instance.

}
}

_focusLastItem() {
Copy link
Contributor

@dbatiste dbatiste Jul 6, 2023

Choose a reason for hiding this comment

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

I don't feel strongly about it, but I think the "Item" term was being used because it is focusing the last item in the cell. But it would appear if that's the case, it was done inconsistently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense. Yeah it was certainly inconsistent. I can go back through and try to add that context everywhere if it makes sense.

<slot name="actions" class="d2l-cell" data-cell-num="7"></slot>
<slot name="content" class="d2l-cell" data-cell-num="8" @focus="${!this.noPrimaryAction ? this._preventFocus : null}"></slot>

<slot name="content-action" class="d2l-cell" data-cell-num="0"></slot>
Copy link
Contributor

Choose a reason for hiding this comment

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

Original order...

1 ::slotted([slot="outside-control-action"]) {
2 ::slotted([slot="expand-collapse"]) {
3 ::slotted([slot="control-action"]) {
4 ::slotted([slot="content-action"]) {
5 ::slotted([slot="actions"]) {
6 ::slotted([slot="drop-target"]) ➡️  now using `z-index: 1`
7 :host(.d2l-dragging-over) ::slotted([slot="nested"]) ➡️ now using `z-index: 2`

With these changes, it looks like content-action is being moved to be underneath everything, where is was previously on top of control-action. Does that cause issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's why I'm managing the outside-control-action and control-action column-end based on the whether there is a primary action or not. expand-collapse does not itself ever overlap (it uses a separate handler in control-action) so that one is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also the z-index for drag-n-drop probably aren't necessary anymore either.

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome. Thanks for the explanation. 😄

Copy link
Contributor

@dbatiste dbatiste left a comment

Choose a reason for hiding this comment

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

I did a little bit of manual testing with it and all seems to be "ok".

Just the follow-up about the start/end grid lines, but nbd.

Looks good.

@bearfriend bearfriend merged commit 00bcb9e into main Jul 7, 2023
@bearfriend bearfriend deleted the dgleckler/safari-list-item-order branch July 7, 2023 14:24
@ghost
Copy link

ghost commented Jul 7, 2023

🎉 This PR is included in version 2.132.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants