diff --git a/root/static/scripts/common/components/Autocomplete2.js b/root/static/scripts/common/components/Autocomplete2.js index 78497e77e4f..7b873a7935b 100644 --- a/root/static/scripts/common/components/Autocomplete2.js +++ b/root/static/scripts/common/components/Autocomplete2.js @@ -337,7 +337,7 @@ const Autocomplete2 = (React.memo(<+T: EntityItemT>( const inputTimeout = React.useRef(null); const containerRef = React.useRef(null); const prevIsOpen = React.useRef(false); - const prevHighlightedIndex = React.useRef(-1); + const shouldUpdateScrollPositionRef = React.useRef(false); const highlightedItem = highlightedIndex >= 0 ? (items[highlightedIndex] ?? null) @@ -528,6 +528,7 @@ const Autocomplete2 = (React.memo(<+T: EntityItemT>( event.preventDefault(); if (isOpen) { + shouldUpdateScrollPositionRef.current = true; dispatch(HIGHLIGHT_NEXT_ITEM); } else { showAvailableItemsOrBeginLookupOrSearch(); @@ -537,6 +538,7 @@ const Autocomplete2 = (React.memo(<+T: EntityItemT>( case 'ArrowUp': if (isOpen) { event.preventDefault(); + shouldUpdateScrollPositionRef.current = true; dispatch(HIGHLIGHT_PREVIOUS_ITEM); } break; @@ -676,22 +678,15 @@ const Autocomplete2 = (React.memo(<+T: EntityItemT>( React.useLayoutEffect(() => { const shouldUpdateScrollPosition = ( - isOpen && - ( - !prevIsOpen.current || - highlightedIndex !== prevHighlightedIndex.current - ) + (isOpen && !prevIsOpen.current) || + shouldUpdateScrollPositionRef.current ); prevIsOpen.current = isOpen; - prevHighlightedIndex.current = highlightedIndex; if (shouldUpdateScrollPosition) { setScrollPosition(menuId); + shouldUpdateScrollPositionRef.current = false; } - }, [ - isOpen, - highlightedIndex, - menuId, - ]); + }); type AutocompleteItemComponent = React$AbstractComponent, void>; diff --git a/root/static/scripts/relationship-editor/components/RelationshipEditor.js b/root/static/scripts/relationship-editor/components/RelationshipEditor.js index 1079cff14cb..fdb5aec7b07 100644 --- a/root/static/scripts/relationship-editor/components/RelationshipEditor.js +++ b/root/static/scripts/relationship-editor/components/RelationshipEditor.js @@ -435,9 +435,19 @@ export function runReducer( sourceEntity, } = action; + const relationshipStateChanged = ( + oldRelationshipState != null && + !relationshipsAreIdentical( + oldRelationshipState, + newRelationshipState, + ) + ); + if ( oldRelationshipState == null || - !relationshipsAreIdentical(oldRelationshipState, newRelationshipState) + relationshipStateChanged || + creditsToChangeForSource || + creditsToChangeForTarget ) { const targetEntity = getRelationshipTarget( newRelationshipState, @@ -445,7 +455,10 @@ export function runReducer( ); const updates = []; - if (oldRelationshipState != null) { + if ( + oldRelationshipState != null && + relationshipStateChanged + ) { /* * The old relationship state must be removed first in a separate * `updateRelationships` call, because its presence affects other @@ -464,11 +477,16 @@ export function runReducer( ); } - updates.push(...getUpdatesForAcceptedRelationship( - writableState, - newRelationshipState, - sourceEntity, - )); + if ( + oldRelationshipState == null || + relationshipStateChanged + ) { + updates.push(...getUpdatesForAcceptedRelationship( + writableState, + newRelationshipState, + sourceEntity, + )); + } /* * `updateEntityCredits` only uses `newRelationshipState` to obtain diff --git a/root/static/scripts/tests/relationship-editor.js b/root/static/scripts/tests/relationship-editor.js index f7dea5080d1..7bbf0360ec0 100644 --- a/root/static/scripts/tests/relationship-editor.js +++ b/root/static/scripts/tests/relationship-editor.js @@ -671,6 +671,71 @@ test('splitRelationshipByAttributes', function (t) { ); }); +test('MBS-12937: Changing credits for other relationships without modifying the source relationship', function (t) { + t.plan(1); + + const relationship1 = { + _lineage: [], + _original: null, + _status: REL_STATUS_ADD, + attributes: null, + begin_date: null, + editsPending: false, + end_date: null, + ended: false, + entity0: artist, + entity0_credit: 'SOMECREDIT', + entity1: recording, + entity1_credit: '', + id: -1, + linkOrder: 0, + linkTypeID: 148, + }; + + const relationship2 = { + ...relationship1, + entity0_credit: '', + id: -2, + linkTypeID: 297, + }; + + Object.freeze(relationship1); + Object.freeze(relationship2); + + let state = addRelationships( + initialState, + recording, + [relationship1, relationship2], + ); + + state = reducer( + state, + { + batchSelectionCount: 0, + creditsToChangeForSource: '', + creditsToChangeForTarget: 'all', + newRelationshipState: relationship1, + oldRelationshipState: relationship1, + sourceEntity: recording, + type: 'update-relationship-state', + }, + ); + + currentRelationshipsEqual( + t, + state, + [ + relationship1, + { + ...relationship2, + entity0_credit: 'SOMECREDIT', + }, + ], + 'all entity credits are updated despite not modifying the source ' + + 'relationship', + ); +}); + function addRelationships( rootState: RelationshipEditorStateT, source: CoreEntityT, diff --git a/t/selenium.mjs b/t/selenium.mjs index 56ce9fa0811..3f44f4f61e5 100755 --- a/t/selenium.mjs +++ b/t/selenium.mjs @@ -314,6 +314,7 @@ const KEY_CODES = { '${KEY_HOME}': Key.HOME, '${KEY_SHIFT}': Key.SHIFT, '${KEY_TAB}': Key.TAB, + '${KEY_UP}': Key.ARROW_UP, '${MBS_ROOT}': DBDefs.MB_SERVER_ROOT.replace(/\/$/, ''), }; diff --git a/t/selenium/Autocomplete2.json5 b/t/selenium/Autocomplete2.json5 index 0154afc7c33..14bf2511c74 100644 --- a/t/selenium/Autocomplete2.json5 +++ b/t/selenium/Autocomplete2.json5 @@ -18,5 +18,108 @@ value: '0', }, // End of test for MBS-12631. + // MBS-12936: The menu scroll position should only be updated when + // navigating items using the arrow keys, or when initially opening the + // menu (to reset it to the top). Hovering over items with the mouse + // should explicitly not affect the scroll position. + // The menu should now be open; first check that the scroll position is + // initially zero: + { + command: 'assertEval', + target: 'document.querySelector("#attribute-type-test-menu").scrollTop', + value: '0', + }, + // Simulate a mouseover event on the last item. + { + command: 'runScript', + target: '\ + document.querySelector("#attribute-type-test-menu li:last-child")\ + .dispatchEvent(new Event("mouseover", {bubbles: true}))\ + ', + value: '', + }, + // Make sure the last item is highlighted. + { + command: 'assertEval', + target: 'document.querySelector("#attribute-type-test-menu li:last-child[aria-selected=true]") != null', + value: 'true', + }, + // Check that the scroll position did not change. + { + command: 'assertEval', + target: 'document.querySelector("#attribute-type-test-menu").scrollTop', + value: '0', + }, + { + command: 'type', + target: 'id=attribute-type-test-input', + value: '', + }, + // Close the menu, open it with the down arrow key, and then move to the + // last item with the up arrow key. + { + command: 'pause', + target: '100', + value: '', + }, + { + command: 'sendKeys', + target: 'id=attribute-type-test-input', + value: '${KEY_ESC}', + }, + { + command: 'pause', + target: '100', + value: '', + }, + { + command: 'sendKeys', + target: 'id=attribute-type-test-input', + value: '${KEY_DOWN}', + }, + { + command: 'pause', + target: '100', + value: '', + }, + { + command: 'sendKeys', + target: 'id=attribute-type-test-input', + value: '${KEY_UP}', + }, + // Check that the scroll position changed. + { + command: 'assertEval', + target: 'document.querySelector("#attribute-type-test-menu").scrollTop > 0', + value: 'true', + }, + // Close the menu, open it with the down arrow key, and check that the + // scroll position is back at the top. + { + command: 'pause', + target: '100', + value: '', + }, + { + command: 'sendKeys', + target: 'id=attribute-type-test-input', + value: '${KEY_ESC}', + }, + { + command: 'pause', + target: '100', + value: '', + }, + { + command: 'sendKeys', + target: 'id=attribute-type-test-input', + value: '${KEY_DOWN}', + }, + { + command: 'assertEval', + target: 'document.querySelector("#attribute-type-test-menu").scrollTop', + value: '0', + }, + // End of test for MBS-12936. ], }