fix(carousel): improved tab page navigation #5035
Conversation
|
Deploy preview created for package Built with commit: 1b9e7790cc5ca3ef9ff16fdc0b6c2057dbf81416 |
|
Deploy preview created for package Built with commit: 1b9e7790cc5ca3ef9ff16fdc0b6c2057dbf81416 |
|
Deploy preview created for package Built with commit: 1b9e7790cc5ca3ef9ff16fdc0b6c2057dbf81416 |
| if (currentContains) { | ||
| // going forwards, change page depending on card index | ||
| if (currentCardIndex >= this.start + this.pageSize) { | ||
| this.start = currentCardIndex - (currentCardIndex % this.pageSize); |
There was a problem hiding this comment.
Considering the following condition:
- We have many cards in carousel
this.startpreviously was2- Screen size as large as putting 4 cards in a page
And user keeps hitting tab key to reach to the code here. What should happen?
There was a problem hiding this comment.
@asudo I see, wasn't aware the this.start could be set by the user. Added an additional calculation where it considers the offset to calculate the right index if this.start isn't a proper multiple of this.pageSize.
Now in the condition you provided this.start will become the index 6 instead of 4 as it was before i.e. the next page will start with the proper card.
IgnacioBecerra
added
the
package: web components
| const nextStart = currentCardIndex - (currentCardIndex % this.pageSize); | ||
| const pageOffset = this.start % this.pageSize; | ||
|
|
||
| this.start = nextStart + pageOffset; |
There was a problem hiding this comment.
Probably you meant this.start = currentCardIndex? (Though there are some edge cases to consider)
There was a problem hiding this comment.
nextStart calculates the correct this.start if the user focuses on a random card (and show its containing page)
pageOffset adds any remainder to the calculation if needed, usually it's just 0 (adds 2 in the previous scenario)
this.start = currentCardIndex only works if we tab forward but doesn't address the "random card focus" case.
| if (currentContains && oldNotContains && currentCardIndex === 0) { | ||
| (this.children[this.start] as HTMLElement).focus(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Do we still need this logic?
There was a problem hiding this comment.
@asudoh Yes, we still need that logic to focus on the current starting card after having tabbed outside the carrousel. Without it, the focus will always go to the first card automatically, out of view. This will focus on the current page's starting card.
There was a problem hiding this comment.
I see, thank you for clarifying - I suggest preserving the "focused card index" in _handleFocus(). It can be done by "old contains but current node contains" detection and preserving event.relatedTarget. In this way we can set this.start according to the index of such "previously focused card".
|
|
||
| // going backwards, change page depending on card index | ||
| } else if (currentCardIndex < this.start) { | ||
| this.start = Math.max(currentCardIndex + 1 - this.pageSize, 0); |
There was a problem hiding this comment.
Do you want to apply your improvement to "page forward" logic to the "page backward" logic here, too?
There was a problem hiding this comment.
Since we'd use the same code going in either direction, the previous logic runs in both directions whenever the focus is out of the current page.
There was a problem hiding this comment.
@IgnacioBecerra This is looking really good. One thing I'm seeing in the center 8 columns story is when advancing, the pagination happens first – another tab is needed to move focus to the next item. Going back in pages works fine.
|
@kennylam Thanks for the comment! After investigating, I found the issue is caused by |
|
@kennylam Good catch! @IgnacioBecerra This is caused by |
There was a problem hiding this comment.
Thank you for the update @IgnacioBecerra and the discussion offline!
| @@ -107,10 +119,57 @@ class DDSCarousel extends LitElement { | |||
| // @ts-ignore | |||
| this._observerResizeContainer = new ResizeObserver(this._observeResizeContainer); | |||
| this._observerResizeContainer.observe(contentsNode); | |||
| containerNode?.addEventListener('scroll', this._handleScrollFocus); | |||
There was a problem hiding this comment.
Could you attach this event listener with lit-html template syntax? In this way we don't need to explicitly remove the event listener.
| if (currentContains && oldNotContains) { | ||
| // focus coming from the top | ||
| if (currentCardIndex === 0) { | ||
| (this.children[this.start] as HTMLElement).focus(); | ||
|
|
||
| // focus coming from bottom | ||
| } else if (currentCardIndex === this._total - 1) { | ||
| (this.children[this.start + this.pageSize - 1] as HTMLElement).focus(); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
Could you elaborate why you think we still need this logic here? I thought the first card automatically gets focus in forward-tab scenario, and last card automatically gets on focus in backward-tab scenario. If it's not the case, could you point out what element gets focus instead in such scenarios?
There was a problem hiding this comment.
Sure, we still need this logic because the page will not automatically focus on the first/last card on the current page. The focus actually either goes to the very first card (index 0), or to the very last one, so even if the current page is the right one, the focus will be entirely out of view, and the user would need to keep tabbing/shift+tabbing until it reaches the current page if we don't have the focus() here.
There was a problem hiding this comment.
Thank you for clarifying. My thought is tabbing on the very first card, instead of the first card on the current page, in forward-tab scenario. Similar for backward-scenario. Could you work with the designer to double-check that? Thanks!
There was a problem hiding this comment.
Thanks @IgnacioBecerra - Please add a comment as such.
IgnacioBecerra
added
the
Ready to merge
### Related Ticket(s) carbon-design-system#4799 ### Description Carousel now moves pages automatically when using tab navigation and focusing on the next card in a different page. Works going forward or backwards. ### Changelog **New** - pages now change dynamically depending on the focused card index <!-- React and Web Component deploy previews are enabled by default. --> <!-- To enable additional available deploy previews, apply the following --> <!-- labels for the corresponding package: --> <!-- *** "package: services": Services --> <!-- *** "package: utilities": Utilities --> <!-- *** "package: styles": Carbon Expressive --> <!-- *** "RTL": React / Web Components (RTL) --> <!-- *** "feature flag": React / Web Components (experimental) -->
Related Ticket(s)
#4799
Description
Carousel now moves pages automatically when using tab navigation and focusing on the next card in a different page. Works going forward or backwards.
Changelog
New