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

Update /accname/name/comp_tooltip.html #42093

Merged
merged 4 commits into from
Oct 10, 2023
Merged

Update /accname/name/comp_tooltip.html #42093

merged 4 commits into from
Oct 10, 2023

Conversation

howard-e
Copy link
Contributor

@howard-e howard-e commented Sep 20, 2023 •
edited
Loading

@howard-e howard-e marked this pull request as ready for review September 20, 2023 22:00
cookiecrook
cookiecrook reviewed Sep 25, 2023
View reviewed changes
accname/name/comp_tooltip.html Outdated
Comment on lines 19 to 21
<img src="foo.jpg" title="label" data-expectedlabel="label" data-testname="img with tooltip label without alt" class="ex">
<img src="foo.jpg" title="label" data-expectedlabel="broken" alt="broken" data-testname="img with tooltip label with alt" class="ex">
<img src="foo.jpg" data-expectedlabel="broken" alt="broken" data-testname="img with tooltip label without title" class="ex">
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to test explicitly empty alt too (alt="" label="title")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 0f8aef2

accname/name/comp_tooltip.html Outdated
<a href="#" title="label" data-expectedlabel="label" data-testname="link label from tooltip" class="ex"><img src="#" alt=""></a>
<a href="#" title="label" data-expectedlabel="label" data-testname="link with img with tooltip label" class="ex"><img src="#" alt=""></a>
<a href="#" title="label" data-expectedlabel="label" data-testname="link with text with tooltip label" class="ex">content</a>
<div title="label" data-expectedlabel="label" data-testname="div with text with tooltip label" class="ex">content</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

This div test may be ambiguous since its role is undefined and should in theory be "", generic, or none, which don't support name from: author or contents. Adding role="group" should remove the ambiguity IIRC.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please file this test as-is, in a new, separate issue (on this repo or AccName) to make sure there isn't a spec ambiguity.

Copy link
Contributor Author

@howard-e howard-e Sep 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Please file this test as-is, in a new, separate issue (on this repo or AccName) to make sure there isn't a spec ambiguity.

Got it, submitted w3c/accname#202

Copy link
Contributor Author

@howard-e howard-e Sep 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This div test may be ambiguous since its role is undefined and should in theory be "", generic, or none, which don't support name from: author or contents. Adding role="group" should remove the ambiguity IIRC.

Thanks! Done in 73e4577

accname/name/comp_tooltip.html Outdated
Comment on lines 23 to 26
<textarea title="enter text" data-expectedlabel="enter text" placeholder="textarea" data-testname="textarea with tooltip label" class="ex"></textarea>
<input type="text" title="label" data-expectedlabel="label" placeholder="text" data-testname="text input with tooltip label" class="ex">
<input type="email" title="label" data-expectedlabel="label" placeholder="email" data-testname="email input with tooltip label" class="ex">
<input type="password" title="label" data-expectedlabel="label" placeholder="password" data-testname="password input with tooltip label" class="ex">
Copy link
Contributor

Choose a reason for hiding this comment

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

@scottaohara please review. I think placeholder vs title precedence is still undefined in AccName but defined in HTML-AAM, is that correct? If so, this is probably okay as-is.

Copy link
Contributor

Choose a reason for hiding this comment

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

@howard-e these test names should include mention of the placeholder attr, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@howard-e these test names should include mention of the placeholder attr, I think.

Thanks! Done in 5fd64fd

@howard-e howard-e mentioned this pull request Sep 27, 2023
Closed
cookiecrook
cookiecrook approved these changes Sep 27, 2023
View reviewed changes
Copy link
Contributor

@cookiecrook cookiecrook left a comment

Choose a reason for hiding this comment

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

Thanks for the changes... I'm going to go ahead and r+, but give @scottaohara a few days to respond. As long as he doesn't raise an objection on the placeholder tests, you could merge it next week. If there's additional feedback later, we can address it in a new PR. Thanks Howard!

@cookiecrook cookiecrook mentioned this pull request Sep 27, 2023
Open
@howard-e
Copy link
Contributor Author

Thanks for the changes... I'm going to go ahead and r+, but give @scottaohara a few days to respond. As long as he doesn't raise an objection on the placeholder tests, you could merge it next week. If there's additional feedback later, we can address it in a new PR. Thanks Howard!

Okay noted, thanks for the review!

@cookiecrook
Copy link
Contributor

Ready to merge, @howard-e? Thanks!

@howard-e howard-e merged commit fe042a5 into master Oct 10, 2023
@howard-e howard-e deleted the bocoup/accname-comp-tooltip branch October 10, 2023 14:24
@howard-e
Copy link
Contributor Author

@cookiecrook yep, merged. Thanks!

cookiecrook pushed a commit to cookiecrook/wpt that referenced this pull request Oct 11, 2023
@howard-e @cookiecrook
Update /accname/name/comp_tooltip.html (web-platform-tests#42093)
c793a15 
* Update /accname/name/comp_tooltip.html

* Add role=group to div test

* Add test to check <img> with empty alt

* Update testnames of tests with placeholders
@cookiecrook cookiecrook mentioned this pull request Nov 21, 2024
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cookiecrook cookiecrook cookiecrook approved these changes

@scottaohara scottaohara Awaiting requested review from scottaohara

@MelSumner MelSumner Awaiting requested review from MelSumner

Assignees

@cookiecrook cookiecrook

Labels
accname
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AccName: tests/todos in accname/name/comp_tooltip
3 participants
@howard-e @cookiecrook @wpt-pr-bot