92 submission form changing funding to from na breaks submission form in some cases on firefox#95
Open
amadulhaxxani wants to merge 15 commits intoclarin-v7from
Conversation
Section-level save is no longer triggered on metadata change to prevent unnecessary change detection and scroll jumps, especially in Firefox. Data integrity is maintained by existing save mechanisms on section blur and form deactivation.
Eliminated the expectation for dispatchSaveSection call in the onChange test, focusing the test on form reinitialization behavior.
There was a problem hiding this comment.
Pull request overview
This pull request removes redundant section-level saves triggered on every metadata field change in the submission form, addressing a Firefox-specific scroll jump issue while maintaining data integrity through existing save mechanisms.
Changes:
- Removed
dispatchSaveSectioncall fromdispatchFormSaveAndReinitializemethod to prevent unnecessary store updates - Added comprehensive comments explaining why section-level saves on onChange are redundant and how data persistence is guaranteed through alternative flows
- Updated test expectations to reflect the new behavior
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/app/submission/sections/form/section-form.component.ts | Removed redundant dispatchSaveSection call and added detailed explanatory comments about save mechanisms |
| src/app/submission/sections/form/section-form.component.spec.ts | Updated test to remove expectation that dispatchSaveSection is called during onChange |
kosarko
requested changes
Jan 30, 2026
Member
kosarko
left a comment
kosarko
left a comment
There was a problem hiding this comment.
works for @amadulhaxxani locally, but not for me (check with docker pull ufal/dspace-angular:67367fc3c161d8d57754958fc01b63a906f836b0).
- was not always saving
fixing the scroll
Add visual-empty support for single-item array groups so the first (structural) group can be hidden and revealed without adding/removing items. Key changes: - DynamicRowArrayModel: add serializable properties hideGroupsWhenEmpty and allowDeleteOnSingleItem and read them from config. - Templates & styles: add ds-form-array-group-hidden CSS class and template logic to hide the first group and show an empty-state "Add" button. Add i18n key `form.add-single`. - DsDynamicFormArrayComponent: add shouldHideGroup and isGroupEmpty helpers to determine visual-empty state. - FormComponent: add revealFirstGroup, clearItemValues, handleItemDelete, shouldShowDeleteButton, shouldShowEmptyStateAddButton and isFirstGroupEmpty to handle reveal/clear flows and button visibility; wire delete button to route to those behaviors. - SectionFormOperationsService: update remove dispatch to handle clear-last-item vs real remove so JSON Patch ops are correct when clearing the only group and when stored values exist. - FieldParser: set hideGroupsWhenEmpty/allowDeleteOnSingleItem for sponsor complex inputs and fix getInitArrayIndex logic to ensure at least one initial group. - Sponsor dropdown: remove a now-unnecessary case branch. - SubmissionSectionFormComponent: minor type/ts-ignore fixes and updated test expectations to reflect the new sponsor reinitialize/save behavior. These updates provide a symmetric UX where the first array group can be visually hidden when empty, revealed without creating a new item, and cleared (not removed) to return to the visual-empty state while preserving correct patch operations.
Remove unused imports and apply small formatting fixes across form-related files to address lint warnings and improve consistency. Changes: - Remove unused DYNAMIC_FORM_CONTROL_TYPE_SCROLLABLE_DROPDOWN import from dynamic-sponsor-scrollable-dropdown.component.ts. - Remove unused ConfigObject import from section-form.component.ts. - Add missing semicolon in section-form-operations.service.ts. - Reformat conditional blocks in field-parser.ts (brace/whitespace cleanup) with no logic changes. No functional behavior was changed.
.../shared/form/builder/ds-dynamic-form-ui/models/array-group/dynamic-form-array.component.html
Outdated
Show resolved
Hide resolved
src/app/submission/sections/form/section-form-operations.service.ts
Outdated
Show resolved
Hide resolved
src/app/submission/sections/form/section-form-operations.service.ts
Outdated
Show resolved
Hide resolved
copilot suggestion for accessibility
Add isFormGroupEmpty utility and update FormComponent to use it. This centralizes the logic for determining whether a FormGroup is visually empty (handling strings, arrays, nested objects, numbers/booleans), reduces duplication, and simplifies the component. Files changed: added src/app/shared/form/utils/form-group-empty.util.ts and modified src/app/shared/form/form.component.ts to import and delegate to the new utility.
Replace direct window access in SubmissionSectionFormComponent with NativeWindowService and isPlatformBrowser checks to avoid SSR errors. Inject PLATFORM_ID and NativeWindowRef, only preserve/restore scroll position when running in the browser, and simplify some type casts in the combineLatest usage. Update the unit test to provide NativeWindowService and PLATFORM_ID. Also update array-group template to use shouldHideGroup(...) instead of isGroupEmpty(...).
Cleanup: remove unused variables (metadataKey and isSponsor) from FormComponent.clearItemValues and SubmissionSectionFormComponent.onRemove. These were dead locals that didn't affect behavior; removal reduces clutter and potential linter warnings.
Adds unit tests for FormComponent array group behaviors: revealFirstGroup, clearItemValues, and handleItemDelete (covering single/multi-item and hideGroupsWhenEmpty cases). Imports UntypedFormArray in the spec. Also simplifies error logging in SubmissionSectionFormComponent by removing a redundant cast/ts-ignore and logging e?.stack || e directly.
Optimize FormComponent by adding an emptyStateCache (Map) to memoize isFirstGroupEmpty results and avoid repeated expensive isFormGroupEmpty calls during change detection. The cache is cleared on formGroup.valueChanges via a new subscription and early false values are stored for invalid states. Also replace hardcoded 'local.sponsor' checks in FieldParser with the SPONSOR_METADATA_NAME constant import for better maintainability.
Replace the suppressed TypeScript check and typed map parameter with an untyped parameter and an inline cast: map((configData) => configData.payload as SubmissionFormsModel). This removes the // @ts-ignore workaround for the ConfigObject vs SubmissionFormsModel mismatch and clarifies the payload type without changing runtime behavior.
…rm-changing-funding-to-from-na-breaks-submission-form-in-some-cases-on-firefox
Cleanup: remove unused imports to silence TS/linter warnings. Deleted an unused `UntypedFormArray` import from src/app/shared/form/form.component.spec.ts and an unused `RemoteData` import from src/app/submission/sections/form/section-form.component.ts. No functional changes intended.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request adjusts the form save behavior in
SubmissionSectionFormComponentto avoid unnecessary section-level saves triggered on every metadata change.During investigation of a scroll-jump issue (most noticeable in Firefox), it became clear that calling
dispatchSaveSectionon each change caused repeated store updates and change detection across all sections. While the data was saved correctly, this behavior led to avoidable UI side effects and performance overhead.The section-level save in this case is redundant, as submission data is already reliably persisted through existing save flows (full-form save on blur, autosave where enabled, and backend responses updating all sections).
What changed
dispatchFormSaveAndReinitialize.Tests
section-form.component.spec.tsto reflect the new behavior by removing the expectation thatdispatchSaveSectionis called.This keeps the save logic simpler, avoids unnecessary store churn, and prevents UI issues such as unexpected scrolling, while preserving existing submission reliability.