fix(progress-bar): removed duplicate label

nikkimk commented 1 month ago

Description

Removes duplicate label from progress bar.

Related issue(s)

fixes #4225

How has this been tested?

[ ] Test case 1
1. Go here
2. Right click the first progress bar example and choose Inspect Element
3. From the Element Inspector click on the progress bar HTML and select Edit HTML
4. Replace the existing progress bar HTML the following HTML: <sp-progress-bar label="" size="m" indeterminate ></sp-progress-bar>.
5. Click Enter.
6. Verify that the new progress bar displayed has no label.
7. Right click the new progress bar's HTML and choose Add Attribute.
8. Type the attribute label="label1".
9. Click Enter.
10. Verify that the progress bar label displays as label1 not label1label1.

Types of changes

[x] 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

[x] I have signed the Adobe Open Source CLA.
[x] 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.
[x] I have read the CONTRIBUTING document.
[x] I have added tests to cover my changes. (Note: Testing won't allow an empty an empty label, so I can't test for this.)
[x] All new and existing tests passed.
[x] I have reviewed at the Accessibility Practices for this feature, see: Aria Practices

github-actions[bot] commented 1 month ago

Branch preview

Visual regression test results

github-actions[bot] commented 1 month ago

Lighthouse scores

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

What is this?

[Lighthouse](https://github.com/GoogleChrome/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	221.238 kB	210.968 kB	210.773 kB 🏆
Scripts	53.413 kB	48.609 kB	48.458 kB 🏆
Stylesheet	34.827 kB	30.416 kB	30.381 kB 🏆
Document	6.027 kB	5.321 kB	5.32 kB 🏆
Font	126.971 kB	126.622 kB	126.614 kB 🏆

Request Count

Category	Latest	Main	Branch
Total	45	45	45
Scripts	37	37	37
Stylesheet	5	5	5
Document	1	1	1
Font	2	2	2

github-actions[bot] commented 1 month ago

Tachometer results

Chrome

## progress-bar [_permalink_](#user-content-progress-bar) ### basic-test | Version | Bytes | Avg Time | vs remote | vs branch | |---|---|---|---|---| | npm latest | 407 kB | 29.26ms - 30.54ms | - | slower ❌
1% - 7%
0.40ms - 2.00ms | | branch | 395 kB | 28.21ms - 29.19ms | faster ✔
1% - 7%
0.40ms - 2.00ms | - |

Firefox

## progress-bar [_permalink_](#user-content-progress-bar) ### basic-test | Version | Bytes | Avg Time | vs remote | vs branch | |---|---|---|---|---| | npm latest | 407 kB | 61.74ms - 65.34ms | - | slower ❌
0% - 9%
0.26ms - 5.18ms | | branch | 395 kB | 59.14ms - 62.50ms | faster ✔
0% - 8%
0.26ms - 5.18ms | - |

Rajdeepc commented 1 month ago

@nikkimk Can you catch up to main and add some documentation on this?

blunteshwar commented 1 month ago

Can we add a relevant test for this

blunteshwar commented 1 month ago

I have added a couple of tests and updated the logic as well

nikkimk commented 1 month ago

Think about how this will render <sp-progress-bar label="Loading" indeterminate>abcdefgh</sp-progress-bar>

@Rajdeepc What is the desired behavior for this use case?

blunteshwar commented 1 month ago

Think about how this will render <sp-progress-bar label="Loading" indeterminate>content</sp-progress-bar>

@Rajdeepc What is the desired behavior for this use case?

In this case <sp-progress-bar label="Loading" indeterminate>abcdefgh</sp-progress-bar> when both label is present and content are present then, both of them will exist inside slot sp-field-label which is logically not so correct. Either of them should be present in sp-filed-label. There shouldn't be a scenario when both label and slot content are present inside slot

nikkimk commented 1 month ago

Think about how this will render <sp-progress-bar label="Loading" indeterminate>content</sp-progress-bar>

@Rajdeepc What is the desired behavior for this use case?

In this case <sp-progress-bar label="Loading" indeterminate>abcdefgh</sp-progress-bar> when both label is present and content are present then, both of them will exist inside slot sp-field-label which is logically not so correct. Either of them should be present in sp-filed-label. There shouldn't be a scenario when both label and slot content are present inside slot

So when both are present, is the attribute the only one that should exist?

blunteshwar commented 1 month ago

Think about how this will render <sp-progress-bar label="Loading" indeterminate>content</sp-progress-bar>

@Rajdeepc What is the desired behavior for this use case?

In this case <sp-progress-bar label="Loading" indeterminate>abcdefgh</sp-progress-bar> when both label is present and content are present then, both of them will exist inside slot sp-field-label which is logically not so correct. Either of them should be present in sp-filed-label. There shouldn't be a scenario when both label and slot content are present inside slot

So when both are present, is the attribute the only one that should exist?

When both are present only the content should exist which in the above example is 'abcdefgh'.

nikkimk commented 1 month ago

When both are present only the content should exist which in the above example is 'abcdefgh'.

@Rajdeepc To make sure I'm understanding, is the following correct?

If `label` attribute is	and default `slot` is	then what appears visually should be	and the `aria-label` should be
empty	`content`	`content`	`content`
`abcdefgh`	empty	`abcdefgh`	`abcdefgh`
`abcdefgh`	`content`	`content`	`abcdefgh`
empty	empty	empty	empty

blunteshwar commented 1 month ago

abcdefgh

In the third row when both label="abcdefgh" and content are present, then what appears visually should becontent

nikkimk commented 4 weeks ago

@blunteshwar

abcdefgh

In the third row when both label="abcdefgh" and content are present, then what appears visually should becontent

What about the aria-label? Ideally sighted screenreader users should hear and see the same content?

blunteshwar commented 3 weeks ago

@blunteshwar

abcdefgh

In the third row when both label="abcdefgh" and content are present, then what appears visually should becontent

What about the aria-label? Ideally sighted screenreader users should hear and see the same content?

Indeed I think we have to make sure that when content is present then, aria-label =content.

nikkimk commented 1 week ago

Making sure I capture that part of our discussion revolved around this previous issue: https://github.com/adobe/spectrum-web-components/issues/3146 and whether or not this is how progress bar should work.

Rajdeepc commented 1 week ago

Making sure I capture that part of our discussion revolved around this previous issue: #3146 and whether or not this is how progress bar should work.

I think since this is a part of a longer discussion chain, how do we feel we fix the related bug of duplicate label and take this in a follow up work?

adobe / spectrum-web-components