fix(tabs): scroll exceeding tabs limit #4722

mizgaionutalexandru · 2024-09-04T13:00:17Z

Description

This PR aims to address said problem by limitting the delta a consumer can pass to the scrollTabs public method, used in sp-tabs-overflow.

Related issue(s)

[Bug]: Tabs overflow exceed tabs limit #4700

Motivation and context

Currently on iOS devices the user can still act upon the "left/right" chevron icons to scroll through the tabs even when there are no more tabs to scroll to. Also, an empty space is visible when scrolling to the right/left most tab.

How has this been tested?

Test case 1
1. Go to the tabs overflow storybook using an iOS device
2. Click on the "right" icon to scroll to the right until you reach the last tab item
3. Observe that the scroll stops right at the last item, without any empty spaces and the icon disappears
Test case 2
1. Repeat test case 1
2. Click on the "left" icon to scroll to the left until you reach the first item
3. Observe that the scroll stops right at the first item, without any empty spaces and the icon disappears
Test case 3
1. Go to the tabs overflow storybook using an iOS device
2. Set the direction to 'rtl'
3. Click on the "left" icon to scroll to the left until you reach the last tab item
4. Observe that the scroll stops right at the last item, without any empty spaces and the icon disappears
Test case 4
1. Repeat test case 3
2. Click on the "right" icon to scroll to the right until you reach the first item
3. Observe that the scroll stops right at the first item, without any empty spaces and the icon disappears
Did it pass in Desktop?
Did it pass in Mobile?
Did it pass in iPad?

Screenshots (if appropriate)

Screen.Recording.2024-09-04.at.16.04.58.mp4

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Checklist

I have signed the Adobe Open Source CLA.
My code follows the code style of this project.
If my change required a change to the documentation, I have updated the documentation in this pull request.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.
I have reviewed at the Accessibility Practices for this feature, see: Aria Practices

Best practices

This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against main.

github-actions · 2024-09-04T13:02:37Z

Branch preview

Visual regression test results

When a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:

github-actions · 2024-09-04T13:04:54Z

Lighthouse scores

Category	Latest (report)	Main (report)	Branch (report)
Performance	0.99	0.99	0.99
Accessibility	1	1	1
Best Practices	1	1	1
SEO	1	0.92	0.92
PWA	1	1	1

What is this?

Lighthouse scores comparing the documentation site built from the PR ("Branch") to that of the production documentation site ("Latest") and the build currently on main ("Main"). Higher scores are better, but note that the SEO scores on Netlify URLs are artifically constrained to 0.92.

Transfer Size

Category	Latest	Main	Branch
Total	228.36 kB	217.134 kB 🏆	217.358 kB
Scripts	58.036 kB	52.558 kB 🏆	52.624 kB
Stylesheet	34.315 kB	30.087 kB 🏆	30.254 kB
Document	6.238 kB	5.462 kB 🏆	5.507 kB
Font	126.778 kB	126.615 kB 🏆	126.623 kB

Request Count

Category	Latest	Main	Branch
Total	52	52	52
Scripts	41	41	41
Stylesheet	5	5	5
Document	1	1	1
Font	2	2	2

coveralls · 2024-09-04T13:07:38Z

Pull Request Test Coverage Report for Build 11269714249

Details

26 of 26 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.002%) to 98.235%

Totals
Change from base Build 11259704832:	0.002%
Covered Lines:	32948
Relevant Lines:	33360

💛 - Coveralls

github-actions · 2024-09-04T13:07:57Z

Tachometer results

Chrome

tabs permalink

basic-test

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	440 kB	115.61ms - 118.53ms	-	faster ✔ 0% - 3% 0.11ms - 3.78ms
branch	417 kB	117.91ms - 120.12ms	slower ❌ 0% - 3% 0.11ms - 3.78ms	-

top-nav permalink

basic-test

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	446 kB	39.51ms - 40.36ms	-	faster ✔ 3% - 6% 1.04ms - 2.45ms
branch	423 kB	41.12ms - 42.24ms	slower ❌ 3% - 6% 1.04ms - 2.45ms	-

Firefox

tabs permalink

basic-test

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	440 kB	197.96ms - 205.08ms	-	unsure 🔍 -5% - +1% -9.91ms - +1.23ms
branch	417 kB	201.58ms - 210.14ms	unsure 🔍 -1% - +5% -1.23ms - +9.91ms	-

top-nav permalink

basic-test

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	446 kB	87.56ms - 94.48ms	-	faster ✔ 0% - 10% 0.14ms - 9.46ms
branch	423 kB	92.69ms - 98.95ms	unsure 🔍 -0% - +11% +0.14ms - +9.46ms	-

Rajdeepc · 2024-09-05T07:44:09Z

Great Start! Would you also be interested in creating a test case for this so that we can surface this up and make sure that we get non-breaking changes.

mizgaionutalexandru · 2024-09-05T08:18:10Z

Great Start! Would you also be interested in creating a test case for this so that we can surface this up and make sure that we get non-breaking changes.

I've been trying to get that working but wasn't able to create a test for it. I tried to focus the nextButton and use sendKeys to simulate multiple clicks and then checking the scroll position of the #list with scrollLeft but I got some random numbers that weren't reliable. Do you have any idea on how can i approach this?

Also, I'm not aware how I can make a test specific for iOS, where the problem is. Are there any guidelines in SWC for this type of situations?

Rajdeepc · 2024-09-05T08:59:58Z

Great Start! Would you also be interested in creating a test case for this so that we can surface this up and make sure that we get non-breaking changes.

I've been trying to get that working but wasn't able to create a test for it. I tried to focus the nextButton and use sendKeys to simulate multiple clicks and then checking the scroll position of the #list with scrollLeft but I got some random numbers that weren't reliable. Do you have any idea on how can i approach this?

Also, I'm not aware how I can make a test specific for iOS, where the problem is. Are there any guidelines in SWC for this type of situations?

I am working with the playwright team to work on a solution to enable touch specific renderers which will help us test mobile specific touch events.
Currently it is still not supported but you can create mobile viewport emulation as described here

packages/tabs/test/tabs-overflow.test.ts

Rajdeepc · 2024-09-11T09:51:00Z

packages/tabs/src/Tabs.ts

@@ -235,12 +235,26 @@ export class Tabs extends SizedMixin(Focusable, { noDefaultSize: true }) {
        return this.rovingTabindexController.focusInElement || this;
    }

+    private limitDelta(min: number, max: number) {


Please add a documentation block on these methods!

Thank you for pointing that out. Indeed this method especially seemed somewhat ambiguous. I changed it's implementation so that it does the same thing but in a cleaner manner. What are your thoughts on this?

packages/tabs/src/Tabs.ts

packages/tabs/src/TabsOverflow.ts

Rajdeepc · 2024-10-07T06:08:18Z

Please pull this up to main so that the VRTs are happy!

packages/tabs/src/Tabs.ts

rubencarvalho · 2024-10-07T10:06:55Z

packages/tabs/test/tabs-overflow.test.ts

+        expect(tabsOverflow['overflowState'].canScrollRight).to.be.true;
+    });
+
+    xit('should scroll up to the last item and back in RTL', async () => {


Would it work and make sense to let these tests run, even without setting the user agent?

We can run the tests if we remove the setUserAgent part, this is how I created them in the first place. I think it would make sense to run these for desktop as well, to cover the scenario.

The thing with this approach is that they wouldn't fail even if this bug is reproducible on iOS and I think is good to signal in some way that these tests must be enabled once we have a way to run them on iPad/iOS/Android. I'm thinking to enable them in the current tests contexts and commenting the setUserAgent part, along with a comment like this:

// TODO: run on iPhone as per https://github.com/adobe/spectrum-web-components/pull/4722

What do you think, do you have any suggestions on this?

Yes, I think that approach is good. While this bug is iOS-only, this test is running in a mobile viewport which still helps with our testing coverage 👍

packages/tabs/test/tabs-overflow.test.ts

packages/tabs/src/Tabs.ts

mizgaionutalexandru force-pushed the imizga/ios-tab-scroll branch from 3913223 to d3b3230 Compare September 4, 2024 13:00

fix(tabs): scroll exceeding tabs limit

5182d9e

mizgaionutalexandru force-pushed the imizga/ios-tab-scroll branch from d3b3230 to 5182d9e Compare September 4, 2024 13:02

mizgaionutalexandru marked this pull request as ready for review September 4, 2024 13:10

mizgaionutalexandru requested a review from a team as a code owner September 4, 2024 13:10

Rajdeepc added bug Something isn't working Component: Tabs iOS bug reported in iOS devices labels Sep 5, 2024

test: added unit tests

36b1e19

mizgaionutalexandru commented Sep 9, 2024

View reviewed changes

packages/tabs/test/tabs-overflow.test.ts Show resolved Hide resolved

Rajdeepc requested changes Sep 11, 2024

View reviewed changes

docs: added docs to scrollTabs

80d1a67

mizgaionutalexandru requested review from Rajdeepc and a team September 24, 2024 07:39

mizgaionutalexandru mentioned this pull request Sep 30, 2024

fix(number-field): show decimal on iPad minimized keyboard #4784

Merged

16 tasks

rubencarvalho reviewed Oct 7, 2024

View reviewed changes

mizgaionutalexandru added 2 commits October 7, 2024 15:33

Merge branch 'main' into imizga/ios-tab-scroll

7ec37a9

test: unskip scroll tests

67cf3ef

mizgaionutalexandru force-pushed the imizga/ios-tab-scroll branch 4 times, most recently from b348ab6 to cb31e92 Compare October 8, 2024 11:41

mizgaionutalexandru force-pushed the imizga/ios-tab-scroll branch 3 times, most recently from 6e22ef8 to 67cf3ef Compare October 8, 2024 13:58

najikahalsema linked an issue Oct 8, 2024 that may be closed by this pull request

[Bug]: Tabs overflow exceed tabs limit #4700

Closed

1 task

mizgaionutalexandru force-pushed the imizga/ios-tab-scroll branch from 2158736 to fabe43c Compare October 9, 2024 08:00

test: skip tests in Firefox

4cc9f6d

mizgaionutalexandru force-pushed the imizga/ios-tab-scroll branch from fabe43c to 4cc9f6d Compare October 9, 2024 08:16

mizgaionutalexandru and others added 5 commits October 9, 2024 11:25

test: fix firefox check and comm

9ee6ce3

Merge branch 'main' into imizga/ios-tab-scroll

434e63f

test: coverage fix

ad939e5

Merge branch 'main' into imizga/ios-tab-scroll

258a11f

chore: re-added guard clause

b24869c

rubencarvalho approved these changes Oct 10, 2024

View reviewed changes

rubencarvalho merged commit fc9a448 into main Oct 10, 2024
57 of 61 checks passed

rubencarvalho deleted the imizga/ios-tab-scroll branch October 10, 2024 08:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(tabs): scroll exceeding tabs limit #4722

fix(tabs): scroll exceeding tabs limit #4722

mizgaionutalexandru commented Sep 4, 2024 •

edited

Loading

github-actions bot commented Sep 4, 2024

github-actions bot commented Sep 4, 2024 •

edited

Loading

coveralls commented Sep 4, 2024 •

edited

Loading

github-actions bot commented Sep 4, 2024 •

edited

Loading

tabs permalink

basic-test

top-nav permalink

basic-test

tabs permalink

basic-test

top-nav permalink

basic-test

Rajdeepc commented Sep 5, 2024

mizgaionutalexandru commented Sep 5, 2024 •

edited

Loading

Rajdeepc commented Sep 5, 2024 •

edited

Loading

Rajdeepc Sep 11, 2024

mizgaionutalexandru Sep 12, 2024

Rajdeepc commented Oct 7, 2024

rubencarvalho Oct 7, 2024

mizgaionutalexandru Oct 7, 2024

rubencarvalho Oct 7, 2024

fix(tabs): scroll exceeding tabs limit #4722

fix(tabs): scroll exceeding tabs limit #4722

Conversation

mizgaionutalexandru commented Sep 4, 2024 • edited Loading

Description

Related issue(s)

Motivation and context

How has this been tested?

Screenshots (if appropriate)

Types of changes

Checklist

Best practices

github-actions bot commented Sep 4, 2024

Branch preview

github-actions bot commented Sep 4, 2024 • edited Loading

Lighthouse scores

Transfer Size

Request Count

coveralls commented Sep 4, 2024 • edited Loading

Pull Request Test Coverage Report for Build 11269714249

Details

💛 - Coveralls

github-actions bot commented Sep 4, 2024 • edited Loading

Tachometer results

tabs permalink

basic-test

top-nav permalink

basic-test

tabs permalink

basic-test

top-nav permalink

basic-test

Rajdeepc commented Sep 5, 2024

mizgaionutalexandru commented Sep 5, 2024 • edited Loading

Rajdeepc commented Sep 5, 2024 • edited Loading

Rajdeepc Sep 11, 2024

Choose a reason for hiding this comment

mizgaionutalexandru Sep 12, 2024

Choose a reason for hiding this comment

Rajdeepc commented Oct 7, 2024

rubencarvalho Oct 7, 2024

Choose a reason for hiding this comment

mizgaionutalexandru Oct 7, 2024

Choose a reason for hiding this comment

rubencarvalho Oct 7, 2024

Choose a reason for hiding this comment

mizgaionutalexandru commented Sep 4, 2024 •

edited

Loading

github-actions bot commented Sep 4, 2024 •

edited

Loading

coveralls commented Sep 4, 2024 •

edited

Loading

github-actions bot commented Sep 4, 2024 •

edited

Loading

mizgaionutalexandru commented Sep 5, 2024 •

edited

Loading

Rajdeepc commented Sep 5, 2024 •

edited

Loading