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

Test radio button interaction between groups #27029

Merged
merged 4 commits into from Jan 11, 2021

Conversation

s0lidstate
Copy link
Contributor

@s0lidstate s0lidstate commented Jan 3, 2021

Greetings,

This is my first GitHub pull request ever. I have been looking through the wpt issues labeled with 'good first issue' and I thought I would give issue #1551 a shot. @kaixinjxq already has a PR created in 2018 for this issue, and it looks like good work, but I thought I would try something a little different. I will appreciate any review comments and advice that you can give me. Hoping to make my first contribution!

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good to me. Would you be willing to file bugs against Firefox and Safari for their one failure? (It seems they do not group buttons that are not associated with a form.)

I'll leave this open to see if someone else has concerns.

@s0lidstate
Copy link
Contributor Author

I tried to reproduce the bug whereby Firefox and Safari seem to not correctly group buttons that are not associated with a form. When I created an HTML file on my computer that was similar to the test scenario, Safari automatically added body and html tags and the radio buttons worked per the spec.

I then updated the test scenario to use the createHTMLDocument method and the test passed in both Firefox and Safari. So apparently my original test setup was not realistic. I would certainly be willing to file bugs if necessary but I now believe that things are working correctly!

Thank you for your review and comments @annevk!

@domenic
Copy link
Member

domenic commented Jan 8, 2021

Eek, no, I don't think you want to hide failures by making the tests "more realistic"! The point is to ensure that all browsers behave the same in all cases, not just in "realistic" cases. I think the old test was better!

@s0lidstate
Copy link
Contributor Author

Thank you for the feedback @domenic. Upon further consideration, I appreciate your point that the goal of the web platform tests is to ensure consistent behavior between browsers. Creating realistic test scenarios is not as important as ensuring consistent behavior. I will change the tests back to the prior design.

Having said that, now I am wondering how I would document this bug in Firefox and Safari. I was not able to reproduce the bug earlier which is what led me to changing my test in aaf8531. Should I put a link to the relevant wpt.fyi results and the relevant portion of the spec? Is there a standard procedure for creating bugs resulting from wpt tests?

@annevk
Copy link
Member

annevk commented Jan 8, 2021

You can file bugs at https://bugzilla.mozilla.org/enter_bug.cgi?product=Core for Firefox and https://bugs.webkit.org/enter_bug.cgi?product=WebKit for Safari. Adding a link to this pull request is sufficient.

@s0lidstate
Copy link
Contributor Author

I have created a Firefox bug and a Safari bug for this issue. Would anyone be willing to take a look to make sure that I filled these out properly? Thanks again for your help!

@annevk annevk merged commit 6c2e813 into web-platform-tests:master Jan 11, 2021
@annevk
Copy link
Member

annevk commented Jan 11, 2021

Thanks for doing all that @s0lidstate! Great first PR too!

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.

None yet

5 participants