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

Number Controller: use period instead of comma for decimal point regardless of locale settings #79

Merged
merged 2 commits into from
Jun 11, 2023

Conversation

juliangarnier
Copy link
Contributor

@juliangarnier juliangarnier commented Dec 19, 2022

Following the discussion in #16, Here's my take on how to fix the comma / period problem in Number controllers.

This PR greatly improves the experience for the users of these 74 countries since it's currently not possible for us to simply copy / paste values from a lil-gui number field to our code without having to manually replace the comma with a period.

This fix make sure that:

  • Mobile and tablet users have a fully functional numeric keyboard when interacting with a Number controller.
  • Chrome Desktop users from countries that use comma as decimal mark (even when using a touch screen) always see decimal values separated with a period.

Without the fix on Chrome Desktop:

Screenshot 2022-12-19 at 13 59 51
After the fix:

Screenshot 2022-12-19 at 13 59 57
While keeping the numeric keyboard on mobile:

IMG_BAE4200CD36D-1

Let me know if you have any questions regarding the changes, I tried to be as concise as possible.
Thanks.

… fully functional numeric keyboard for mobile user
@@ -65,9 +65,26 @@ export default class NumberController extends Controller {

_initInput() {

// Detect if the device has touch as his primary pointer input.
// This will targets only phones and tablets.
const hasTouchAsPrimaryPointerInput = window.matchMedia( '(pointer: coarse)' ).matches;
Copy link

Choose a reason for hiding this comment

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

say, do you know if this value changes at runtime? e g if they switch from a finger to a stylus, does it now match pointer: fine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No from what I understand the pointer media query only returns the primary input.

From the specs:

If multiple pointing devices are present, the pointer media feature must reflect the characteristics of the “primary” pointing device

So if you plug a mouse to an iPad for example, pointer: coarse will still returns true since the primary input device is the touchscreen.

any-pointer does what you're describing, but in our case that would introduce false positive.

@georgealways
Copy link
Owner

So if we were to do this, I wish there was a better condition to switch on than a media query. That's subject to a lot of false positives (laptop with touchscreen, iPad with mouse).

Another option would be to pivot on the locale:

( 1.1 ).toLocalString().contains( ',' )

Or (only half kidding) user agent sniffing for iOS. That's the real offender here.

Appreciate your work on this and thanks for bringing the issue to my attention! I'd like to hold off until we have a better sense of how many people this is affecting, and a super solid if-statement for switching input types.

Editing the title here for a bit more googleability.

@georgealways georgealways changed the title Fix wrong decimal separator in Number controller on Chrome desktop Number Controller: use period instead of comma for decimal point regardless of locale settings Dec 26, 2022
@juliangarnier
Copy link
Contributor Author

juliangarnier commented Dec 27, 2022

That's subject to a lot of false positives (laptop with touchscreen, iPad with mouse).

The media query pointer looks for the "primary" pointing device, not any pointing device.
I don't have the right devices to test this, but according to the specs, pointer: coarse should returns false in the case of a laptop with a touchscreen since its primary pointing device is a trackpad, and returns true in the iPad with a mouse case since the primary pointing device is the touchscreen.

Regarding the use of toLocaleString(),

const number = 1.1;
console.log(number.toLocaleString());

Returns '1.1' on my laptop in Chrome, so it's unfortunately not possible to detect the comma like this.

I'd like to hold off until we have a better sense of how many people this is affecting

I understand, maybe we should mention this on the three.js repo to get more feedback?

I'll switch to a different GUI library in my current projects for now since this makes working with decimal values more complicated than it should be, but happy to help if you need any help on this issue.

@georgealways
Copy link
Owner

Been thinking about this a bit more. This issue is the exact sort of idiosyncrasy I was worried we might run into when using the number type.

I'm not actually sure we need a perfect conditional. Even if the media query gets the wrong answer for some reason, the experience wouldn't really suffer.

False positive on desktop would mean commas for certain locales (current experience)
False negative on iOS would mean no negative numbers (not ideal but I don't see this happening)

Hopefully we can find a totally bulletproof test in the future, but this should already improve the experience for a lot of people. And maybe one day iOS will put a damn negative sign on the inputmode=decimal keyboard so we can forget the whole thing ;)

Anyway, I'll aim to have this conditional in the release after this coming one. Appreciate your help!

@georgealways georgealways added this to the 0.19.0 milestone Mar 11, 2023
@georgealways georgealways merged commit 4c0b348 into georgealways:dev Jun 11, 2023
@georgealways
Copy link
Owner

That's finally merged and published. Thanks for your help and your patience!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants