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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
Jul 13, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7203f1f
fix the raw tag editor broken
k-yle Jul 13, 2023
ca4e688
new approach
k-yle Jul 13, 2023
a26755b
fix typo to satisfy github checks
k-yle Jul 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ _Breaking developer changes, which may affect downstream projects or sites that
* Upgrade OSM data dependencies: `id-tagging-schema` to v6.3, `osm-community-index` to 5.5.3
* Upgrade icon sets: `fortawesome` to v6.4, `temaki` to v5.4
* Upgrade `osm-auth` to v2.1,
* Upgrade dev dependecies, including the following major version upgrades: `glob` to v10, `marked` to v5, `cldr-core` and `cldr-localenames-full` to v43, `esbuild` to v0.18
* Upgrade dev dependencies, including the following major version upgrades: `glob` to v10, `marked` to v5, `cldr-core` and `cldr-localenames-full` to v43, `esbuild` to v0.18
* Build icons from configured presets source and also process field value `icons` in `npm run build:data`

[#8769]: https://github.com/openstreetmap/iD/pull/8769
Expand Down
13 changes: 7 additions & 6 deletions modules/util/get_set_value.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,7 @@ export function utilGetSetValue(selection, value, shouldUpdate) {
}

function stickyCursor(func) {
// 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'];
return function() {
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.

func.apply(this, arguments);
this.setSelectionRange(cursor.start, cursor.end);
Expand All @@ -49,5 +43,12 @@ export function utilGetSetValue(selection, value, shouldUpdate) {
shouldUpdate = (a, b) => a !== b;
}

// 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 •
edited
Loading

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. 😊

return selection.each(setValue(value, shouldUpdate));
}

return selection.each(stickyCursor(setValue(value, shouldUpdate)));
}