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

Saving workflow fails when using decimal comma (,) #16271

Closed
microposmp opened this issue Jun 10, 2024 · 7 comments · Fixed by #16274
Closed

Saving workflow fails when using decimal comma (,) #16271

microposmp opened this issue Jun 10, 2024 · 7 comments · Fixed by #16274
Labels
Milestone

Comments

@microposmp
Copy link
Contributor

Describe the bug

When saving a workflow using settings with decimal comma (,) fails.

Orchard Core version

Current dev branch.

Logs and screenshots

image

@Piedone
Copy link
Member

Piedone commented Jun 10, 2024

Where exactly do you provide this number as the input?

I suppose it's a culture thing, and the culture of the app is one that uses a different decimal separator.

@microposmp
Copy link
Contributor Author

Yes this is because of culture setting. I'm using Swedish regional settings where we have the decimal point as comma (,).

The value comes from the internal representation after adding or moving a task or event. I looked a little bit at the javascript to see if it could be changed to always treat it as int, but didn't find the source. But I think it's correct to use a decimal point as the storage format. The comma is only used for presentation.

@MikeAlhayek MikeAlhayek added this to the 2.0 milestone Jun 10, 2024
Copy link
Contributor

We triaged this issue and set the milestone according to the priority we think is appropriate (see the docs on how we triage and prioritize issues).

This indicates when the core team may start working on it. However, if you'd like to contribute, we'd warmly welcome you to do that anytime. See our guide on contributions here.

@Piedone
Copy link
Member

Piedone commented Jun 10, 2024

Got it, thanks!

@BenedekFarkas
Copy link
Member

BenedekFarkas commented Jun 13, 2024

I encountered the same issue a couple weeks back, but the repro was quite exotic, so I didn't report it. The interesting thing about this feature is that the editor uses a 10 by 10 pixel grid to snap elements into such positions that the coordinates are always divisible by 10, thus they will always be integers.

The only way I could break this and produce the same error is by having the fingerprinting-client-rects-noise flag in Ungoogled Chromium enabled (chrome://flags/#fingerprinting-client-rects-noise), which does the following:

Scale the output values of Range::getClientRects() and Element::getBoundingClientRect() with a randomly selected factor in the range -0.0003% to 0.0003%, which are recomputed on every document initialization. ungoogled-chromium flag, Bromite feature.

That's one part of the error regarding integer conversion, but culture settings also play a role (decimal point vs. comma).

@microposmp are you using this flag?

@microposmp
Copy link
Contributor Author

No, I've not set any flags explicitly. Running the latest version of Edge and can reproduce the issue 100% of the time when moving an element in the workflow editor. I think that the fractional numbers can be caused by my scale settings. Using 250% scale setting in Windows. 100% scale in the browser.

sebastienros pushed a commit that referenced this issue Jun 13, 2024
#16274)

* Saving workflow fails

* Fix solution build file references (#16269)

* Remove Gitter references (#16270)

* Add serialisation compatibility of TimeSpan and DateTime  (#16205)

Co-authored-by: Mike Alhayek <[email protected]>
Co-authored-by: Hisham Bin Ateya <[email protected]>

* Update Azure.Identity 1.11.4 (#16286)

* Add rounding.

---------

Co-authored-by: Zoltán Lehóczky <[email protected]>
Co-authored-by: Tony Han <[email protected]>
Co-authored-by: Mike Alhayek <[email protected]>
Co-authored-by: Hisham Bin Ateya <[email protected]>
@BenedekFarkas
Copy link
Member

I didn't think about Windows scaling, although I would expect it to produce different numbers. The number shown in your screenshot is very similar (in terms of inaccuracy) to those I saw in my testing. Maybe Edge has fingerprinting deception features enabled by default, in which case this is less exotic than I thought. :)

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 a pull request may close this issue.

4 participants