diff --git a/CHANGELOG.md b/CHANGELOG.md index a6dcf837d..04cb4d714 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,7 @@ _Breaking developer changes, which may affect downstream projects or sites that #### :sparkles: Usability & Accessibility * Render housenumbers (or housenames) of address points or buildings as dedicated labels on the map ([#10970]) +* Simplify raw tag editor and make it easier to use with keyboard-only input ([#10889]) #### :scissors: Operations #### :camera: Street-Level #### :white_check_mark: Validation @@ -49,6 +50,7 @@ _Breaking developer changes, which may affect downstream projects or sites that #### :mortar_board: Walkthrough / Help #### :hammer: Development +[#10889]: https://github.com/openstreetmap/iD/pull/10889 [#10970]: https://github.com/openstreetmap/iD/pull/10970 [#11027]: https://github.com/openstreetmap/iD/pull/11027 diff --git a/css/80_app.css b/css/80_app.css index 2ab6f09b0..8e9351410 100644 --- a/css/80_app.css +++ b/css/80_app.css @@ -359,6 +359,9 @@ button.disabled { color: rgba(0,0,0,.4); cursor: not-allowed; } +button.disabled .icon { + fill: rgba(0,0,0,.4); +} .joined > * { border-radius: 0; @@ -4250,7 +4253,8 @@ li.issue-fix-item button:not(.actionable) .fix-icon { .inspector-hover .form-field-input-radio label, .inspector-hover .form-field-input-radio label span, .inspector-hover .form-field-input-radio label.remove .icon, -.inspector-hover .add-row, +.inspector-hover .tag-row button, +.inspector-hover .tag-row.add-tag, .inspector-hover .section-entity-issues .issue-container .issue-fix-list, .inspector-hover .section-entity-issues .issue-container .issue-info-button { display: none; @@ -4264,9 +4268,8 @@ li.issue-fix-item button:not(.actionable) .fix-icon { .inspector-hover .hide-toggle:before, .inspector-hover .more-fields, .inspector-hover .field-label button, -.inspector-hover .tag-row button, .inspector-hover .footer * { - opacity: 0; + visibility: hidden; } /* Unstyle the active entity issue on hover */ diff --git a/data/core.yaml b/data/core.yaml index 9873cf07d..7b9a55961 100644 --- a/data/core.yaml +++ b/data/core.yaml @@ -713,7 +713,6 @@ en: features: Features title_count: "{title} ({count})" add_to_relation: Add to a relation - add_to_tag: Add a tag new_relation: New relation... choose_relation: Choose a parent relation role: Role diff --git a/modules/ui/sections/raw_tag_editor.js b/modules/ui/sections/raw_tag_editor.js index b54f3e1ea..1d3eda7e5 100644 --- a/modules/ui/sections/raw_tag_editor.js +++ b/modules/ui/sections/raw_tag_editor.js @@ -1,5 +1,6 @@ import { dispatch as d3_dispatch } from 'd3-dispatch'; import { select as d3_select } from 'd3-selection'; +import { isEmpty } from 'lodash-es'; import { services } from '../../services'; import { svgIcon } from '../../svg/icon'; @@ -7,10 +8,9 @@ import { uiCombobox } from '../combobox'; import { uiSection } from '../section'; import { uiTagReference } from '../tag_reference'; import { prefs } from '../../core/preferences'; -import { localizer, t } from '../../core/localizer'; +import { t } from '../../core/localizer'; import { utilArrayDifference, utilArrayIdentical } from '../../util/array'; import { utilGetSetValue, utilNoAuto, utilRebind, utilTagDiff } from '../../util'; -import { uiTooltip } from '..'; import { allowUpperCaseTagValues } from '../../osm/tags'; import { fileFetcher } from '../../core'; @@ -42,7 +42,6 @@ export function uiSectionRawTagEditor(id, context) { var _readOnlyTags = []; // the keys in the order we want them to display var _orderedKeys = []; - var _showBlank = false; var _pendingChange = null; var _state; var _presets; @@ -76,11 +75,8 @@ export function uiSectionRawTagEditor(id, context) { return { index: i, key: key, value: _tags[key] }; }); - // append blank row last, if necessary - if (!rowData.length || _showBlank) { - _showBlank = false; - rowData.push({ index: rowData.length, key: '', value: '' }); - } + // append blank row last + rowData.push({ index: rowData.length, key: '', value: '' }); // View Options @@ -160,32 +156,6 @@ export function uiSectionRawTagEditor(id, context) { .merge(list); - // Container for the Add button - var addRowEnter = wrap.selectAll('.add-row') - .data([0]) - .enter() - .append('div') - .attr('class', 'add-row' + (_tagView !== 'list' ? ' hide' : '')); - - addRowEnter - .append('button') - .attr('class', 'add-tag') - .attr('aria-label', t('inspector.add_to_tag')) - .call(svgIcon('#iD-icon-plus', 'light')) - .call(uiTooltip() - .title(() => t.append('inspector.add_to_tag')) - .placement(localizer.textDirection() === 'ltr' ? 'right' : 'left')) - .on('click', addTag); - - addRowEnter - .append('div') - .attr('class', 'space-value'); // preserve space - - addRowEnter - .append('div') - .attr('class', 'space-buttons'); // preserve space - - // Tag list items var items = list.selectAll('.tag-row') .data(rowData, function(d) { return d.key; }); @@ -224,11 +194,11 @@ export function uiSectionRawTagEditor(id, context) { .call(utilNoAuto) .on('focus', interacted) .on('blur', valueChange) - .on('change', valueChange) - .on('keydown.push-more', pushMore); + .on('change', valueChange); innerWrap .append('button') + .attr('tabindex', -1) .attr('class', 'form-field-button remove') .attr('title', t('icons.remove')) .call(svgIcon('#iD-operation-delete')); @@ -240,6 +210,7 @@ export function uiSectionRawTagEditor(id, context) { .sort(function(a, b) { return a.index - b.index; }); items + .classed('add-tag', d => d.key === '') .each(function(d) { var row = d3_select(this); var key = row.select('input.key'); // propagate bound data @@ -260,7 +231,10 @@ export function uiSectionRawTagEditor(id, context) { } row.select('.inner-wrap') // propagate bound data - .call(reference.button); + .call(reference.button) + .select('.tag-reference-button') + .attr('tabindex', -1) + .classed('disabled', d => d.key === ''); // disabled for blank tag line row.call(reference.body); @@ -269,10 +243,13 @@ export function uiSectionRawTagEditor(id, context) { items.selectAll('input.key') .attr('title', function(d) { return d.key; }) - .call(utilGetSetValue, function(d) { return d.key; }) .attr('readonly', function(d) { return isReadOnly(d) || null; - }); + }) + .call(utilGetSetValue, + d => d.key, + (_, newKey) => _pendingChange === null || isEmpty(_pendingChange) || _pendingChange[newKey] // if there are pending changes: skip untouched keys + ); items.selectAll('input.value') .attr('title', function(d) { @@ -284,14 +261,21 @@ export function uiSectionRawTagEditor(id, context) { .attr('placeholder', function(d) { return typeof d.value === 'string' ? null : t('inspector.multiple_values'); }) - .call(utilGetSetValue, function(d) { - return typeof d.value === 'string' ? d.value : ''; - }) .attr('readonly', function(d) { return isReadOnly(d) || null; - }); + }) + .call(utilGetSetValue, + d => { + if (_pendingChange !== null && !isEmpty(_pendingChange) && !_pendingChange[d.value]) { + // if there are pending changes: skip untouched values + return null; + } + return typeof d.value === 'string' ? d.value : ''; + }, (_, newValue) => newValue !== null + ); items.selectAll('button.remove') + .classed('disabled', d => d.key === '') // disabled for blank tag line .on(('PointerEvent' in window ? 'pointer' : 'mouse') + 'down', // 'click' fires too late - #5878 (d3_event, d) => { if (d3_event.button !== 0) return; @@ -388,7 +372,7 @@ export function uiSectionRawTagEditor(id, context) { } }); - if (Object.keys(_pendingChange).length === 0) { + if (isEmpty(_pendingChange)) { _pendingChange = null; section.reRender(); return; @@ -397,15 +381,6 @@ export function uiSectionRawTagEditor(id, context) { scheduleChange(); } - function pushMore(d3_event) { - // if pressing Tab on the last value field with content, add a blank row - if (d3_event.keyCode === 9 && !d3_event.shiftKey && - section.selection().selectAll('.tag-list li:last-child input.value').node() === this && - utilGetSetValue(d3_select(this))) { - addTag(); - } - } - function bindTypeahead(key, value) { if (isReadOnly(key.datum())) return; @@ -574,41 +549,22 @@ export function uiSectionRawTagEditor(id, context) { function removeTag(d3_event, d) { if (isReadOnly(d)) return; - if (d.key === '') { // removing the blank row - _showBlank = false; - section.reRender(); + // remove the key from the ordered key index + _orderedKeys = _orderedKeys.filter(function(key) { return key !== d.key; }); - } else { - // remove the key from the ordered key index - _orderedKeys = _orderedKeys.filter(function(key) { return key !== d.key; }); - - _pendingChange = _pendingChange || {}; - _pendingChange[d.key] = undefined; - scheduleChange(); - } - } - - function addTag() { - // Delay render in case this click is blurring an edited combo. - // Without the setTimeout, the `content` render would wipe out the pending tag change. - window.setTimeout(function() { - _showBlank = true; - section.reRender(); - section.selection().selectAll('.tag-list li:last-child input.key').node().focus(); - }, 20); + _pendingChange = _pendingChange || {}; + _pendingChange[d.key] = undefined; + scheduleChange(); } function scheduleChange() { - // Cache IDs in case the editor is reloaded before the change event is called. - #6028 - var entityIDs = _entityIDs; + if (!_pendingChange) return; - // Delay change in case this change is blurring an edited combo. - #5878 - window.setTimeout(function() { - if (!_pendingChange) return; - - dispatch.call('change', this, entityIDs, _pendingChange); - _pendingChange = null; - }, 10); + for (const key in _pendingChange) { + _tags[key] = _pendingChange[key]; + } + dispatch.call('change', this, _entityIDs, _pendingChange); + _pendingChange = null; } diff --git a/test/spec/ui/sections/raw_tag_editor.js b/test/spec/ui/sections/raw_tag_editor.js index 18e94f878..0e33e9be3 100644 --- a/test/spec/ui/sections/raw_tag_editor.js +++ b/test/spec/ui/sections/raw_tag_editor.js @@ -1,5 +1,3 @@ -import { setTimeout } from 'node:timers/promises'; - describe('iD.uiSectionRawTagEditor', function() { var taglist, element, entity, context; @@ -37,40 +35,22 @@ describe('iD.uiSectionRawTagEditor', function() { it('creates a pair of empty input elements if the entity has no tags', function () { element.remove(); render({}); + expect(element.selectAll('.tag-list li').nodes().length).to.eql(1); expect(element.select('.tag-list').selectAll('input.value').property('value')).to.be.empty; expect(element.select('.tag-list').selectAll('input.key').property('value')).to.be.empty; }); - it('adds tags when clicking the add button', async () => { - happen.click(element.selectAll('button.add-tag').node()); - await setTimeout(20); - expect(element.select('.tag-list').selectAll('input').nodes()[2].value).to.be.empty; - expect(element.select('.tag-list').selectAll('input').nodes()[3].value).to.be.empty; - }); - - it('removes tags when clicking the remove button', async () => { - iD.utilTriggerEvent(element.selectAll('button.remove'), 'mousedown', { button: 0 }); - const tags = await new Promise(cb => { - taglist.on('change', (_, tags) => cb(tags)); - }); - expect(tags).to.eql({highway: undefined}); - }); - - it('adds tags when pressing the TAB key on last input.value', async () => { - expect(element.selectAll('.tag-list li').nodes().length).to.eql(1); - var input = d3.select('.tag-list li:last-child input.value').nodes()[0]; - happen.keydown(d3.select(input).node(), {keyCode: 9}); - await setTimeout(20); + it('adds pair of empty input elements at end of list', () => { expect(element.selectAll('.tag-list li').nodes().length).to.eql(2); expect(element.select('.tag-list').selectAll('input').nodes()[2].value).to.be.empty; expect(element.select('.tag-list').selectAll('input').nodes()[3].value).to.be.empty; }); - it('does not add a tag when pressing TAB while shift is pressed', async () => { - expect(element.selectAll('.tag-list li').nodes().length).to.eql(1); - var input = d3.select('.tag-list li:last-child input.value').nodes()[0]; - happen.keydown(d3.select(input).node(), {keyCode: 9, shiftKey: true}); - await setTimeout(20); - expect(element.selectAll('.tag-list li').nodes().length).to.eql(1); + it('removes tags when clicking the remove button', async () => { + const tags = new Promise(cb => { + taglist.on('change', (_, tags) => cb(tags)); + }); + iD.utilTriggerEvent(element.selectAll('button.remove'), 'mousedown', { button: 0 }); + expect(await tags).to.eql({highway: undefined}); }); });