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

Failure when server returns "Value has more than 255 unicode characters" #6817

Closed
tbertels opened this issue Aug 29, 2019 · 10 comments
Closed
Assignees
Labels
bug A bug - let's fix this!
Milestone

Comments

@tbertels
Copy link

To reproduce:

  • Edit a element property (in this case source) and add more than 255 characters (typically multiple addresses).
  • Try to save

Result:
iD shows an error dialog: "Errors occurred while trying to save"
image
Then after clicking OK and trying to save again, we get an OAuth dialog:

image
Granting access leads to a File not found server error:

image

This last error may be due to osm itself, but why do we get the OAuth dialog in the first place?

And isn't it possible to show a more helpful message to the user? Including the name of the element if not null and the name of the property would help. This may need some work on the osm API though.

@mmd-osm
Copy link
Contributor

mmd-osm commented Aug 31, 2019

Some of the input fields already have a max length of 255 characters, in case of "Source" (maybe others?), it seems to be missing, so that a user can enter much longer strings.

And isn't it possible to show a more helpful message to the user? Including the name of the element if not null and the name of the property would help. This may need some work on the osm API though.

This can be much better handled by iD by no letting you enter longer strings in the first place. Nothing to do on the OSM API side here.

@quincylvania quincylvania added the bug A bug - let's fix this! label Sep 3, 2019
@quincylvania
Copy link
Collaborator

This can be much better handled by iD by no letting you enter longer strings in the first place.

Indeed. We should audit all tag inputs and operations to ensure that they adhere to the character limit.

@quincylvania
Copy link
Collaborator

Related: #4061

@quincylvania quincylvania self-assigned this Jan 27, 2020
@quincylvania quincylvania added this to the Next Release milestone Jan 27, 2020
quincylvania added a commit that referenced this issue Jan 27, 2020
…characters remaining to add another value (re: #6817)
@mmd-osm
Copy link
Contributor

mmd-osm commented Jan 27, 2020

I left a comment on 17ae12b#commitcomment-36988467, hope it doesn't get lost somehow.

Unless there's some other substr implementation in place, the default one wouldn't handle unicode chars as expected. In the worst case this could lead to some very long description being cut off too early.

@bhousel
Copy link
Member

bhousel commented Jan 29, 2020

Unless there's some other substr implementation in place, the default one wouldn't handle unicode chars as expected. In the worst case this could lead to some very long description being cut off too early.

Thanks @mmd-osm , following our conversation on Slack I'm reopening this so that we can reimplement string counting and truncation functions that match what the API code does.

Our notes:

https://stackoverflow.com/questions/54369513/how-to-count-the-correct-length-of-a-string-with-emojis-in-javascript

led to
https://github.com/orling/grapheme-splitter#examples

But while that contains some great tests, we don't really want to split on graphemes, and using the spread operator to split on codepoints is probably what we really want.:

Screenshot 2020-01-28 13 10 54

But Bublé's support for transpiling the spread operator is buggy:
bublejs/buble#215

So I'll just take the code from that issue (which mimics what Babel does) and wrap it up into a function.

@bhousel bhousel reopened this Jan 29, 2020
@quincylvania quincylvania modified the milestones: 3.0.0, 2.18.0 May 18, 2020
quincylvania added a commit that referenced this issue Jun 9, 2020
@quincylvania
Copy link
Collaborator

Okay, I think I finally have this working correctly.

iD now counts tag and role lengths in unicode characters instead of native JavaScript UTF-16 code units. I had to implement the checks in code rather than relying on the maxlength attribute we used previously since browsers count maxlength inconsistently. Doing it in code is safer in any case.

I also added unicode string normalization as a bonus since we're now cleaning all string input. This can reduce the total length of a string without changing the encoded content.

@jleedev
Copy link
Contributor

jleedev commented Jun 11, 2020

I'm unable to type spaces when writing a changeset description, and this change looks related.

@quincylvania
Copy link
Collaborator

I'm unable to type spaces when writing a changeset description, and this change looks related.

@jleedev Whoops, this change is definitely related! iD now trims whitespace from the ends of textarea fields. Generally this is done only after the field loses focus, but the changeset comment is special.

@tbertels
Copy link
Author

I did notice that the last space gets stripped when the changeset description loses focus, but I'm still able to type spaces. Tested with Firefox and Chrome.

@quincylvania
Copy link
Collaborator

@tbertels This issue relates to the development preview. I was able to reproduce it in Firefox on macOS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug - let's fix this!
Projects
None yet
Development

No branches or pull requests

5 participants