-
Notifications
You must be signed in to change notification settings - Fork 350
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
Multiple Combobox Examples: Adjust input height so text is fully visible in Safari on iOS #2780
Conversation
The ARIA Authoring Practices (APG) Task Force just discussed The full IRC log of that discussion<jugglinmike> Topic: PR 2780: Fix for iOS Safari combobox bug<jugglinmike> github: https://github.com//pull/2780 <jugglinmike> Matt_King: I put in a review checklist, and I'm pretty sure we only need people looking at the visual presentation and the code <jugglinmike> Matt_King: There's not much code, but the code review is important because we want to make sure the approach is appropriate <jugglinmike> Matt_King: The patch uses absolute numbers, and I wonder if that is the best way <jugglinmike> jongund: I can do a code review <jugglinmike> Matt_King: My second question is about the two examples which are NOT changed by this pull request. Could we have someone look and verify that this problem does not exist in iOS and Safari on those two? <jugglinmike> Matt_King: The other two examples are "select only" and "date picker". They do not use the CSS which are modified by this patch (they have their own separate CSS) <jugglinmike> jongund: I can look at those, too <jugglinmike> Matt_King: Thanks! If those also need changes, I'd like those changes to be included in the scope of this patch <jugglinmike> Matt_King: Because I don't actually understand what was wrong (or what "auto" does in that patch), I had trouble choosing a good title for this pull request <jugglinmike> [Jem explains the problem] <jugglinmike> Matt_King: Okay, thank you. I have the answer that I need for the third question. If jongund and jem submit reviews, then Andrea will have what she needs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
I checked on iOS Safari and in OSX with Chrome, Safari and Firefox.
The code changes look fine to me.
Thanks for getting this fixed.
The ARIA Authoring Practices (APG) Task Force just discussed The full IRC log of that discussion<jugglinmike> Topic: PR 2780: Fix for iOS Safari combobox bug<jugglinmike> github: https://github.com//pull/2780 <jugglinmike> Matt_King: We had a visual design review from Jem and a code review from jongund <jugglinmike> Matt_King: If we can get those reviews done this week, I think we can land it in time for the deployment we're planning for next week <jugglinmike> Matt_King: We don't need any other reviewers because this is a CSS fix <jugglinmike> Matt_King: jongund has done his review, so it's up to Jem! <jugglinmike> Matt_King: We're gonna have another great publication next week! <jugglinmike> Zakim, end the meeting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I reviewed this several weeks back but I didnot add my approval.
I checked those again today and all examples have enough space not to hide the part of the text. Thanks for the improvement, @andreancardona
Resolves #2778 by revising CSS for the following 4 combobox examples so that the space for the input is tall enough to fit the text when viewed in Safari on iOS. Previously, the tops of the letters were chopped off.
Worked on this with @ariellalgilmore 🎉
Checkout the combobox examples and make sure they work in Safari iOS - it should look like below.
Preview links
Review checklist
Reviewers: To learn what needs to be covered by each review, Follow the link for the type of review to which you are assigned.
WAI Preview Link (Last built on Mon, 28 Aug 2023 19:19:21 GMT).