fix(number-field): show decimal on iPad minimized keyboard #4784
Conversation
Lighthouse scores
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 Transfer Size
Request Count
|
Pull Request Test Coverage Report for Build 11269966342Details
💛 - Coveralls |
Tachometer resultsChromenumber-field permalinkbasic-test
slider permalinktest-basic
Firefoxnumber-field permalinkbasic-test
slider permalinktest-basic
|
mizgaionutalexandru
force-pushed
the
imizga/iPadOS-decimal-missing
branch
from
bc1fe4d to
f40e01b
Compare
mizgaionutalexandru
force-pushed
the
imizga/iPadOS-decimal-missing
branch
from
f40e01b to
2906995
Compare
Currently all the tests I know in SWC that are device specific (for example this iPhone decimals on number-field) are skipped, so it wouldn't catch the issue if it comes up again. I don't yet see a reason to create these tests just for the sake of it. |
| @@ -802,30 +803,21 @@ export class NumberField extends TextfieldBase { | |||
| } | |||
|
|
|||
| if (changes.has('min') || changes.has('formatOptions')) { | |||
| let inputMode = 'numeric'; | |||
| const hasNegative = typeof this.min !== 'undefined' && this.min < 0; | |||
There was a problem hiding this comment.
What happens if min is not defined but the current value is negative?, would it make sense to add this to the condition?
There was a problem hiding this comment.
Currently that's not taken into account but I see some value in it. I also see some concerns after a minimal PoC - if we do this at (every) value update it might confuse users to show different keyboards based on the component's value. Therefore I really am not sure if we want something like this.
Do you want to further investigate/ talk more about this in a new issue?
I also opened a new one for a visual bug I found during this investigation.
There was a problem hiding this comment.
I also see some concerns after a minimal PoC - if we do this at (every) value update it might confuse users to show different keyboards based on the component's value
Right, one approach could be to implement this as a flag at the component level, which remains fixed once toggled. I don’t have a strong preference here, but it might be helpful to emphasize in our documentation the importance of setting the minimum value explicitly. Currently, if negative numbers are allowed but no minimum value is set, users on iOS won’t see the minus key on the keyboard.
Do you want to further investigate/ talk more about this in a new issue?
Not sure, maybe we can find a compromise with either one (or both?) of the approaches above.
I also opened a #4788 for a visual bug I found during this investigation.
Oof! Thanks for finding and reporting it!
There was a problem hiding this comment.
Not sure, maybe we can find a compromise with either one (or both?) of the approaches above.
What if we use something like this instead:
| const hasNegative = typeof this.min !== 'undefined' && this.min < 0; | |
| const hasNegative = typeof this.min === 'undefined' || this.min < 0; |
This way I think it aligns more with the current documentation saying:
If a valid range is known ahead of time, it is a good idea to provide it to
<sp-number-field>so it can optimize the experience. For example, when the minimum value is greater than or equal to zero, it is possible to use a numeric keyboard on iOS rather than a full text keyboard (necessary to enter a minus sign).
Now we always assume that the user can type in negative values (which he would've gotten to using the decrement button multiple times), easing the experience and opening the keyboard that allows the most flexibility.
This way we also don't need "hack-ish" flags or workarounds. If a consumer knows that he wants only positive values, he can simply set the min to zero and the keyboard will adapt and not try to force-show a minus sign, but will show the minus sign when no min is provided.
Does this make sense for you, @rubencarvalho ?
There was a problem hiding this comment.
I agree with your approach, and it does align better with the documentation!
But since we are assuming the number field can have negative numbers by default unless the min is greater than 0, what do you think about re-writing the condition to be more explicit:
const hasOnlyPositive = typeof this.min !== 'undefined' && this.min >= 0;(and refactoring the rest of the method accordingly)
There was a problem hiding this comment.
Indeed, I think going for default false values is better for boolean variables and using hasOnlyPositives it would be false by default (with no properties provided by the consumer).
I updated the logic. Thank you for pointing this out! 🙏🏻
Maybe this is outside the scope of this PR, but I'm curious—why are these tests being skipped? Is there something specific preventing us from unskipping them? |
AFAIK so far there were very few cases where setting the user agent was neccessary (but playwright doesn't seem to support it). As I see from the I've encountered this issue in this PR as well. Nevertheless I will create an iPad test similar to these, maybe we will enable iPhone/iPad/Android contexts or something similar in the future. |
Thanks for the context! 👍 I am OK with keeping this outside the scope of this PR for now. I’ve noted it down, and it’s also implicitly part of our ongoing and upcoming efforts to improve mobile support in the future. |
Description
This PR changes the inputMode of the
NumberField'sinputtodecimalon iPad so the minimized keyboard can show the decimal point.Related issue(s)
Motivation and context
Currently an iPad user can't use the minimized keyboard to type in decimal values (i.e. 2.5).
How has this been tested?
Test case 1
Test case 2
Did it pass in Desktop?
Did it pass in Mobile?
Did it pass in iPad?
Screenshots (if appropriate)
Types of changes
Checklist
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.