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

fix the raw tag editor broken and strange cursor behaviour #9766

Merged
merged 3 commits into from
Jul 13, 2023

Conversation

k-yle
Copy link
Collaborator

@k-yle k-yle commented Jul 13, 2023

Closes #9762, Closes #9764, Closes #9767, Closes #9773, Closes #9775, Closes #9774

to test:

edit: this PR indirectly fixes another bug reported on the OSM-US slack, where the changeset comment dropdown doesn't work

Copy link
Collaborator

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

Good catch, but this introduces another subtle regression that should hopefully be simple to avoid.

@@ -32,11 +32,12 @@ export function utilGetSetValue(selection, value, shouldUpdate) {
// see https://html.spec.whatwg.org/multipage/input.html#concept-input-apply
const supportedTypes = ['text', 'search', 'url', 'tel', 'password'];
return function() {
func.apply(this, arguments);

if (!supportedTypes.includes(this.type)) {
return;
}
const cursor = { start: this.selectionStart, end: this.selectionEnd };
Copy link
Collaborator

Choose a reason for hiding this comment

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

Based on the code that changed, these regressions were probably introduced in 14c7525 (related to #8769 and 2b64d70). I believe the intention was for this line to memoize the cursor position before setting the value in order to restore that position afterwards. It needs to go to the very top of this function before the call to apply().

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks, it seems like this bug was present even before this PR (#9764)

I've reworked the logic now so it also fixes #9764. are there any specific cases that are still broken for you?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I was a bit unclear. 14c7525 attempted to fix #9233 but barely avoided fixing #9764 while creating other regressions. As far as I can tell, these changes do fix both issues.

@k-yle k-yle changed the title fix the raw tag editor broken fix the raw tag editor broken and strange cursor behaviour Jul 13, 2023
@@ -32,11 +32,12 @@ export function utilGetSetValue(selection, value, shouldUpdate) {
// see https://html.spec.whatwg.org/multipage/input.html#concept-input-apply
const supportedTypes = ['text', 'search', 'url', 'tel', 'password'];
return function() {
func.apply(this, arguments);

if (!supportedTypes.includes(this.type)) {
return;
}
const cursor = { start: this.selectionStart, end: this.selectionEnd };
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I was a bit unclear. 14c7525 attempted to fix #9233 but barely avoided fixing #9764 while creating other regressions. As far as I can tell, these changes do fix both issues.

@1ec5 1ec5 merged commit 667a620 into openstreetmap:develop Jul 13, 2023
3 checks passed
@k-yle k-yle deleted the hotfix/raw-tag-editor-broken branch July 13, 2023 09:05
tyrasd added a commit that referenced this pull request Jul 13, 2023
// only certain input element types allow manipulating the cursor
// see https://html.spec.whatwg.org/multipage/input.html#concept-input-apply
const supportedTypes = ['text', 'search', 'url', 'tel', 'password'];
if (!supportedTypes.includes(this.type)) {
Copy link
Member

@tyrasd tyrasd Jul 13, 2023

Choose a reason for hiding this comment

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

This does not quite work as probably intended: this is here the global (window) object, and not the input element we'd want to check (d3's selection.each' binds the this` variable to the DOM element in the callback function).

So, in practice, this condition is always fulfilled, even for input element types which would support cursor manipulation. As it still fixes the textarea/raw tag editor bug, I think we can keep it for the moment (even though #9233 is now an issue again), but I'll see if I can fix this a little bit better. 😊

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