directionalCombo: handle [key]=left|right|both schema (Support sidewalk=right) (#10935)

To summarize:

* `sidewalk=left` should be interpreted as `sidewalk:left=yes` + `sidewalk:right=no` (and v.v. for `sidewalk=right`)
* `sidewalk=both` should be interpreted as `sidewalk:left=yes` + `sidewalk:right=yes`
* `sidewalk=other` should be interpreted as `sidewalk:left=other` + `sidewalk:right=other`
* `sidewalk:both=value` should always be interpreted as `sidewalk:left=value` + `sidewalk:right=value` (also when value=left/right)

This should always be the case, regardless if the preset uses `:both` for the common key or not.

Also needs to ensure correct handling of multiselections: The fallback mechanism between the ":both" and ":<direction>" tags of a multiselection requires to look at the tags of each selected entity individually: otherwise the undefined state of a specific direction of the field might be overwritten by the value of the ":both" tag on another selected entity. For example:

* entity 1 has `sidewalk:both=yes`
* entity 2 has `sidewalk:left=yes`

The multiselection now must represent that for the `right` direction, there are conflicting values: once 'no', once _unset/undefined_.

(see also #9423 / 4c222fd)

---------

Co-authored-by: Martin Raifer <martin@raifer.tech>

Author: tordans

CHANGELOG.md

@@ -42,6 +42,7 @@ _Breaking developer changes, which may affect downstream projects or sites that
 * Don't suggest values from Taginfo for `addr:*` tags ([#11733], thanks [@k-yle])
 * Don't suggest values from Taginfo for tags with less than 100 uses, even if they're documented on the wiki ([#11794], thanks [@Kaushik4141])
 * Don't suggest values from Taginfo for keys that should only be used on changesets, such as `hashtags` ([#11697], thanks [@bhavyaKhatri2703])
+* Handle `<key>=left|right|both` in the `directionalCombo` UI to enable support for `sidewalk=left|right|both` [in the future](#id-tagging-schema/pull/1507) ([#10935], thanks [@tordans], [@k-yle])
 #### :scissors: Operations
 * Display reflection axis on the map while hovering the reflection operations in the edit menu ([#11774], thanks [@Kaushik4141])
 #### :camera: Street-Level
@@ -73,6 +74,7 @@ _Breaking developer changes, which may affect downstream projects or sites that
 
 [#8464]: https://github.com/openstreetmap/iD/issues/8464
 [#9401]: https://github.com/openstreetmap/iD/issues/9401
+[#10935]: https://github.com/openstreetmap/iD/issues/10935
 [#11522]: https://github.com/openstreetmap/iD/issues/11522
 [#11589]: https://github.com/openstreetmap/iD/pull/11589
 [#11636]: https://github.com/openstreetmap/iD/pull/11636
@@ -91,6 +93,7 @@ _Breaking developer changes, which may affect downstream projects or sites that
 [#11862]: https://github.com/openstreetmap/iD/pull/11862
 [#11870]: https://github.com/openstreetmap/iD/issues/11870
 [#11904]: https://github.com/openstreetmap/iD/issues/11904
+[#id-tagging-schema/pull/1507]: https://github.com/openstreetmap/id-tagging-schema/pull/1507
 [@ilias52730]: https://github.com/ilias52730
 [@Razen04]: https://github.com/Razen04
 [@homersimpsons]: https://github.com/homersimpsons

modules/ui/fields/directional_combo.js

@@ -9,9 +9,9 @@ export function uiFieldDirectionalCombo(field, context) {
     var dispatch = d3_dispatch('change');
     var items = d3_select(null);
     var wrap = d3_select(null);
-    var _tags;
 
-    var _combos = {};
+    /** @type {Record<string, ReturnType<typeof uiFieldCombo>>} */
+    const _combos = {};
 
     // fallback for schema-builder v5's cycleway field type: can be removed eventually
     if (field.type === 'cycleway') {
@@ -85,15 +85,30 @@ export function uiFieldDirectionalCombo(field, context) {
 
     function change(key, newValue) {
         const commonKey = field.key;
-        /** if commonKey ends with :both, this is the key without :both. and vice-verca */
-        const otherCommonKey = field.key.endsWith(':both')
-            ? field.key.replace(/:both$/, '')
+        // if commonKey contains ":both", this is the key without :both. and vice-versa
+        const otherCommonKey = field.key.includes(':both')
+            ? field.key.replace(/:both(:|$)/,'$1')
             : `${field.key}:both`;
+        // this is the key that might contain the direction (e.g. left/right) as a tag value,
+        // instead of the direction being part of the tag key
+        const fallbackKey = commonKey.includes(':both') ? otherCommonKey : commonKey;
 
         const otherKey = key === field.keys[0] ? field.keys[1] : field.keys[0];
 
         dispatch.call('change', this, tags => {
-            const otherValue = tags[otherKey] || tags[commonKey] || tags[otherCommonKey];
+            let otherValue = tags[otherKey] || tags[commonKey] || tags[otherCommonKey];
+
+            if (tags[fallbackKey] === 'both' && !tags[otherKey]) {
+                otherValue = 'yes';
+            } else if (tags[fallbackKey] && !tags[otherKey]) {
+                const directionalKeyRegExp = new RegExp(`:${tags[fallbackKey]}(:|$)`);
+                if (directionalKeyRegExp.test(otherKey)) {
+                    otherValue = 'yes';
+                } else if (directionalKeyRegExp.test(key)) {
+                    otherValue = 'no';
+                }
+            }
+
             if (newValue === otherValue) {
                 // both tags match, use the common tag to tag both sides the same way
                 tags[commonKey] = newValue;
@@ -112,16 +127,61 @@ export function uiFieldDirectionalCombo(field, context) {
     }
 
 
-    directionalCombo.tags = function(tags) {
-        _tags = tags;
-
-        const commonKey = field.key.replace(/:both$/, '');
-        for (let key in _combos) {
-            const uniqueValues = [... new Set([]
-                .concat(_tags[commonKey])
-                .concat(_tags[`${commonKey}:both`])
-                .concat(_tags[key])
-                .filter(Boolean))];
+    directionalCombo.tags = function(_ignored, __test_tags /* for unit tests only */) {
+        const commonKey = field.key;
+        // if commonKey contains ":both", this is the key without :both. and vice-versa
+        const otherCommonKey = field.key.includes(':both')
+            ? field.key.replace(/:both(:|$)/,'$1')
+            : `${field.key}:both`;
+        // this is the key that might contain the direction (e.g. left/right) as a tag value,
+        // instead of the direction being part of the tag key
+        const fallbackKey = commonKey.includes(':both') ? otherCommonKey : commonKey;
+        // this is the key that is explicitly for ":both" directions
+        const bothDirectionsKey = fallbackKey !== commonKey ? commonKey : otherCommonKey;
+
+        // harmonize tags of selected entities
+        const keys = Object.keys(_combos);
+        const entityTags = Array.isArray(__test_tags)
+            ? __test_tags // for unit tests only
+            : context.selectedIDs().map(id => context.graph().hasEntity(id).tags);
+        const combinedTags = {};
+        for (const key of keys) combinedTags[key] = new Set();
+        for (const tags of entityTags) {
+            let hadCommonValue = false;
+            if (tags[fallbackKey] === 'both') {
+                // interpret key=both as key:*=yes
+                for (const key of keys) combinedTags[key].add('yes');
+                hadCommonValue = true;
+            } else if (tags[fallbackKey]) {
+                const directionalKeyRegExp = new RegExp(`:${tags[fallbackKey]}(:|$)`);
+                if (keys.some(key => directionalKeyRegExp.test(key))) {
+                    // tag value looks like a direction: interpret as key:<value>=yes, key:<other>=no
+                    for (const key of keys) {
+                        if (directionalKeyRegExp.test(key)) {
+                            combinedTags[key].add('yes');
+                        } else {
+                            combinedTags[key].add('no');
+                        }
+                    }
+                } else {
+                    // tag does not look like a direction: handle like key:both=<value>
+                    for (const key of keys) combinedTags[key].add(tags[fallbackKey]);
+                }
+                hadCommonValue = true;
+            }
+            if (tags[bothDirectionsKey]) {
+                // handle ":both" key: set all tags to key:*=<value>
+                for (const key of keys) combinedTags[key].add(tags[bothDirectionsKey]);
+                hadCommonValue = true;
+            }
+            for (const key of keys) {
+                if (tags[key] || !hadCommonValue) {
+                    combinedTags[key].add(tags[key]);
+                }
+            }
+        }
+        for (const key in _combos) {
+            const uniqueValues = [...combinedTags[key]];
             _combos[key].tags({ [key]: uniqueValues.length > 1 ? uniqueValues : uniqueValues[0] });
         }
     };

test/spec/ui/fields/directional_combo.js

@@ -23,7 +23,7 @@ describe('iD.uiFieldDirectionalCombo', () => {
         it('populates the left/right fields using :left & :right', () => {
             const instance = iD.uiFieldDirectionalCombo(field, context);
             selection.call(instance);
-            instance.tags({ 'cycleway:left': 'lane' });
+            instance.tags(undefined, [{ 'cycleway:left': 'lane' }]);
 
             expect(selection.selectAll('input').nodes()).toHaveLength(2);
             const [left, right] = selection.selectAll('input').nodes();
@@ -34,7 +34,7 @@ describe('iD.uiFieldDirectionalCombo', () => {
         it('populates the left/right fields using :both', () => {
             const instance = iD.uiFieldDirectionalCombo(field, context);
             selection.call(instance);
-            instance.tags({ 'cycleway:both': 'lane' });
+            instance.tags(undefined, [{ 'cycleway:both': 'lane' }]);
 
             expect(selection.selectAll('input').nodes()).toHaveLength(2);
             const [left, right] = selection.selectAll('input').nodes();
@@ -45,7 +45,7 @@ describe('iD.uiFieldDirectionalCombo', () => {
         it('populates the left/right fields using the unprefixed tag', () => {
             const instance = iD.uiFieldDirectionalCombo(field, context);
             selection.call(instance);
-            instance.tags({ cycleway: 'lane' });
+            instance.tags(undefined, [{ cycleway: 'lane' }]);
 
             expect(selection.selectAll('input').nodes()).toHaveLength(2);
             const [left, right] = selection.selectAll('input').nodes();
@@ -57,7 +57,7 @@ describe('iD.uiFieldDirectionalCombo', () => {
             const instance = iD.uiFieldDirectionalCombo(field, context);
             selection.call(instance);
             const tags = { 'cycleway:left': 'lane', 'cycleway:right': 'shoulder' };
-            instance.tags(tags);
+            instance.tags(undefined, [tags]);
 
             const onChange = vi.fn();
             instance.on('change', v => onChange(v(tags)));
@@ -79,7 +79,7 @@ describe('iD.uiFieldDirectionalCombo', () => {
             const instance = iD.uiFieldDirectionalCombo(field, context);
             selection.call(instance);
             let tags = { [otherCommonKey]: 'lane' };
-            instance.tags(tags);
+            instance.tags(undefined, [tags]);
 
             const onChange = vi.fn();
             instance.on('change', v => onChange(tags = v(tags)));
@@ -89,7 +89,6 @@ describe('iD.uiFieldDirectionalCombo', () => {
             expect(left.value).toBe('lane');
             expect(right.value).toBe('lane');
 
-
             left.value = 'shoulder';
             d3.select(left).dispatch('change');
 
@@ -108,4 +107,105 @@ describe('iD.uiFieldDirectionalCombo', () => {
             });
         });
     });
+
+    describe('handle multiselection', function() {
+        const field = iD.presetField('name', {
+            key: 'cycleway:both',
+            keys: ['cycleway:left', 'cycleway:right'],
+        });
+
+        it('populates the left/right fields using :left/:right and :both', () => {
+            const instance = iD.uiFieldDirectionalCombo(field, context);
+            selection.call(instance);
+            instance.tags(undefined, [{ 'cycleway:left': 'lane', 'cycleway:right': 'lane' }, { 'cycleway:both': 'lane' }]);
+
+            expect(selection.selectAll('input').nodes()).toHaveLength(2);
+            const [left, right] = selection.selectAll('input').nodes();
+            expect(left.value).toBe('lane');
+            expect(right.value).toBe('lane');
+        });
+
+        it('missing explicit direction tag should be reported like a conflicting value', () => {
+            const instance = iD.uiFieldDirectionalCombo(field, context);
+            selection.call(instance);
+            instance.tags(undefined, [{ 'cycleway:left': 'lane' }, { 'cycleway:both': 'lane' }]);
+
+            expect(selection.selectAll('input').nodes()).toHaveLength(2);
+            const [left, right] = selection.selectAll('input').nodes();
+            expect(left.value).toBe('lane');
+            expect(right.value).toBe('');
+        });
+    });
+
+    describe('handle `[key]=left|right|both` schema', function() {
+        const field = iD.presetField('name', {
+            key: 'cycleway:both',
+            keys: ['cycleway:left', 'cycleway:right'],
+        });
+
+        it('transforms `both` to yes/yes', () => {
+            const instance = iD.uiFieldDirectionalCombo(field, context);
+            selection.call(instance);
+            instance.tags(undefined, [{ 'cycleway': 'both' }]);
+
+            expect(selection.selectAll('input').nodes()).toHaveLength(2);
+            const [left, right] = selection.selectAll('input').nodes();
+            expect(left.value).toBe('yes');
+            expect(right.value).toBe('yes');
+        });
+
+        it('transforms `left` to yes/no', () => {
+            const instance = iD.uiFieldDirectionalCombo(field, context);
+            selection.call(instance);
+            instance.tags(undefined, [{ 'cycleway': 'left' }]);
+
+            expect(selection.selectAll('input').nodes()).toHaveLength(2);
+            const [left, right] = selection.selectAll('input').nodes();
+            expect(left.value).toBe('yes');
+            expect(right.value).toBe('no');
+        });
+
+        it('preserves other values', () => {
+            const instance = iD.uiFieldDirectionalCombo(field, context);
+            selection.call(instance);
+            instance.tags(undefined, [{ 'cycleway': 'other' }]);
+
+            expect(selection.selectAll('input').nodes()).toHaveLength(2);
+            const [left, right] = selection.selectAll('input').nodes();
+            expect(left.value).toBe('other');
+            expect(right.value).toBe('other');
+        });
+
+        it('can read the value from key=left, but writes to key:left', () => {
+            const instance = iD.uiFieldDirectionalCombo(field, context);
+            selection.call(instance);
+            let tags = { 'cycleway': 'left' };
+            instance.tags(undefined, [tags]);
+
+            const onChange = vi.fn();
+            instance.on('change', v => onChange(tags = v(tags)));
+
+            expect(selection.selectAll('input').nodes()).toHaveLength(2);
+            const [left, right] = selection.selectAll('input').nodes();
+            expect(left.value).toBe('yes');
+            expect(right.value).toBe('no');
+
+            left.value = 'separate';
+            d3.select(left).dispatch('change');
+
+            expect(onChange).toHaveBeenCalledTimes(1);
+            expect(onChange).toHaveBeenNthCalledWith(1, {
+                'cycleway:left': 'separate', // left was updated
+                'cycleway:right': 'no',
+            });
+
+            right.value = 'separate';
+            d3.select(right).dispatch('change');
+
+            expect(onChange).toHaveBeenCalledTimes(2);
+            expect(onChange).toHaveBeenNthCalledWith(2, {
+                'cycleway:both': 'separate', // now left & right have been updated
+            });
+        });
+    });
 });