From e62a2d8eb6dbcf9d8af633497701faa9f27bfc99 Mon Sep 17 00:00:00 2001 From: Abhinav Jha Date: Wed, 13 Sep 2023 11:59:41 +0530 Subject: [PATCH] fix: error in scroll on load feature (#27214) ## Description - Fixes #27064 - issue where the scroll on load feature threw an error when failing to find the container to scroll into Fixed by organising the code such that `undefined` values don't have their properties accessed. In an edge case, the widget's parent container like widget was not correctly identified, as a result, accessing the `dynamicHeight` property of the parent container like widget threw an error. - Fixes #27209 - issue where the ButtonWidgetV2's dynamic height feature was incorrectly configured Fixed by removing the dynamic height feature from ButtonWidgetV2. #### Type of change - Bug fix (non-breaking change which fixes an issue) ## Testing - As the issue is an edge case scenario, no reliable mechanism to replicate this has been identified to automate. @Sripriya93 @kamakshibhat-appsmith #### How Has This Been Tested? - [x] Manual - [ ] JUnit - [ ] Jest - [ ] Cypress #### Test Plan #### Issues raised during DP testing ## Checklist: #### Dev activity - [ ] My code follows the style guidelines of this project - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] New and existing unit tests pass locally with my changes - [ ] PR is being merged under a feature flag #### QA activity: - [ ] [Speedbreak features](https://github.com/appsmithorg/TestSmith/wiki/Guidelines-for-test-plans#speedbreakers-) have been covered - [ ] Test plan covers all impacted features and [areas of interest](https://github.com/appsmithorg/TestSmith/wiki/Guidelines-for-test-plans#areas-of-interest-) - [ ] Test plan has been peer reviewed by project stakeholders and other QA members - [ ] Manually tested functionality on DP - [ ] We had an implementation alignment call with stakeholders post QA Round 2 - [ ] Cypress test cases have been added and approved by SDET/manual QA - [ ] Added `Test Plan Approved` label after Cypress tests were reviewed - [ ] Added `Test Plan Approved` label after JUnit tests were reviewed --- .../src/WidgetProvider/factory/helpers.ts | 10 ++- app/client/src/sagas/WidgetOperationSagas.tsx | 10 ++- app/client/src/sagas/WidgetOperationUtils.ts | 6 +- app/client/src/utils/helpers.tsx | 63 ++++++++++++++----- .../widgets/ButtonWidgetV2/widget/index.tsx | 7 +-- app/client/src/widgets/WidgetUtils.ts | 10 ++- 6 files changed, 78 insertions(+), 28 deletions(-) diff --git a/app/client/src/WidgetProvider/factory/helpers.ts b/app/client/src/WidgetProvider/factory/helpers.ts index 0041202812..4fb602c3a8 100644 --- a/app/client/src/WidgetProvider/factory/helpers.ts +++ b/app/client/src/WidgetProvider/factory/helpers.ts @@ -179,8 +179,16 @@ export function enhancePropertyPaneConfig( features[registeredFeature as RegisteredWidgetFeatures]; const sectionName = (config[sectionIndex] as PropertyPaneSectionConfig) ?.sectionName; + // This has been designed to check if the sectionIndex provided in the + // features configuration of the widget to point to the section named "General" + // If not, it logs an error + // This is a sanity check, and doesn't effect the functionality of the feature + // For consistency, we expect that all "Auto Height" property pane controls + // be present in the "General" section of the property pane if (!sectionName || sectionName !== "General") { - log.error(`Invalid section index for feature: ${registeredFeature}`); + log.error( + `Invalid section index for feature: ${registeredFeature} in widget: ${widgetType}`, + ); } if ( Array.isArray(config[sectionIndex].children) && diff --git a/app/client/src/sagas/WidgetOperationSagas.tsx b/app/client/src/sagas/WidgetOperationSagas.tsx index 5b01988a66..6d71e285aa 100644 --- a/app/client/src/sagas/WidgetOperationSagas.tsx +++ b/app/client/src/sagas/WidgetOperationSagas.tsx @@ -1212,11 +1212,17 @@ function* getNewPositionsBasedOnSelectedWidgets( copiedLeftMostColumn: number, ) { //get Parent canvasId - const parentId = selectedWidgets[0].parentId || ""; + const parentId: string | undefined = selectedWidgets[0].parentId; - // get the Id of the container widget based on the canvasId + // If we failed to get the parent canvas widget Id then return empty object + if (parentId === undefined) return {}; + + // get the Id of the container like widget based on the canvasId const containerId = getContainerIdForCanvas(parentId); + // If we failed to get the containing container like widget Id then return empty object + if (containerId === undefined) return {}; + const containerWidget = canvasWidgets[containerId]; const canvasDOM = document.querySelector(`#${getSlidingArenaName(parentId)}`); diff --git a/app/client/src/sagas/WidgetOperationUtils.ts b/app/client/src/sagas/WidgetOperationUtils.ts index da65940564..0db15c96a8 100644 --- a/app/client/src/sagas/WidgetOperationUtils.ts +++ b/app/client/src/sagas/WidgetOperationUtils.ts @@ -815,12 +815,12 @@ export function getDefaultCanvas(canvasWidgets: CanvasWidgetsReduxState) { * @param canvasId * @returns */ -export function getContainerIdForCanvas(canvasId: string) { +export function getContainerIdForCanvas(canvasId: string): string | undefined { if (canvasId === MAIN_CONTAINER_WIDGET_ID) return canvasId; const selector = `#${getStickyCanvasName(canvasId)}`; const canvasDOM = document.querySelector(selector); - if (!canvasDOM) return ""; + if (!canvasDOM) return undefined; //check for positionedWidget parent let containerDOM = canvasDOM.closest(`.${POSITIONED_WIDGET}`); //if positioned widget parent is not found, most likely is a modal widget @@ -1645,6 +1645,8 @@ export function getParentColumnSpace( ) { const containerId = getContainerIdForCanvas(pastingIntoWidgetId); + if (containerId === undefined) return; + const containerWidget = canvasWidgets[containerId]; const canvasDOM = document.querySelector( `#${getStickyCanvasName(pastingIntoWidgetId)}`, diff --git a/app/client/src/utils/helpers.tsx b/app/client/src/utils/helpers.tsx index d27569bd39..5a223eacf1 100644 --- a/app/client/src/utils/helpers.tsx +++ b/app/client/src/utils/helpers.tsx @@ -29,10 +29,11 @@ import { } from "constants/routes"; import history from "./history"; import { APPSMITH_GLOBAL_FUNCTIONS } from "components/editorComponents/ActionCreator/constants"; -import type { CanvasWidgetsReduxState } from "reducers/entityReducers/canvasWidgetsReducer"; +import type { + CanvasWidgetsReduxState, + FlattenedWidgetProps, +} from "reducers/entityReducers/canvasWidgetsReducer"; import { checkContainerScrollable } from "widgets/WidgetUtils"; -import type { ContainerWidgetProps } from "widgets/ContainerWidget/widget"; -import type { WidgetProps } from "widgets/BaseWidget"; import { getContainerIdForCanvas } from "sagas/WidgetOperationUtils"; import scrollIntoView from "scroll-into-view-if-needed"; import validateColor from "validate-color"; @@ -290,25 +291,57 @@ function isElementVisibleInContainer( return visiblePercentage >= percentage; } +/** + * This function provides the correct DOM element to scroll to + * such that the widget (argument) is visible in the viewport. + * This function has been implemented to run when the viewer or editor + * is loaded with a widget ID in the URL. + * This is a part of the Context preserving logic + * + * @param widgetId : Widget ID to scroll to + * @param canvasWidgets : Canvas widgets redux state + * @returns HTMLElement to scroll to or null + */ function getWidgetElementToScroll( widgetId: string, canvasWidgets: CanvasWidgetsReduxState, -) { +): HTMLElement | null { const widget = canvasWidgets[widgetId]; - const parentId = widget.parentId || ""; + const parentId = widget.parentId; + // If the widget doesn't have a parent, scroll to the widget itself + // This is the case for the main container widget, however, + // this scenario is not likely to occur in a normal use case. + if (parentId == undefined) return document.getElementById(widgetId); + + // Get the containing container like widget for the widget + // Note: The parentId is usually pointing to a CANVAS_WIDGET + // However, we can only scroll a container like widget which is the parent + // of the CANVAS_WIDGET. Hence, we need to get the container like widget's Id. const containerId = getContainerIdForCanvas(parentId); - if (containerId === MAIN_CONTAINER_WIDGET_ID) { - if (widget.type !== "MODAL_WIDGET") { - return document.getElementById(widgetId); - } - } - const containerWidget = canvasWidgets[ - containerId - ] as ContainerWidgetProps; - if (checkContainerScrollable(containerWidget)) { + + // If we failed to get the container, try to scroll to the widget itself + if (containerId === undefined) { return document.getElementById(widgetId); } else { - return document.getElementById(containerId); + // If the widget is not within a modal widget, + // but is the child of the main container widget, + // scroll to the widget itself + if (containerId === MAIN_CONTAINER_WIDGET_ID) { + if (widget.type !== "MODAL_WIDGET") { + return document.getElementById(widgetId); + } + } + + // Get the container widget props from the redux state + const containerWidget: FlattenedWidgetProps = canvasWidgets[containerId]; + + // If the widget is within a container, check if the container is scrollable + if (checkContainerScrollable(containerWidget)) { + return document.getElementById(widgetId); + } else { + // If the container is not scrollable, scroll to the container itself + return document.getElementById(containerId); + } } } diff --git a/app/client/src/widgets/ButtonWidgetV2/widget/index.tsx b/app/client/src/widgets/ButtonWidgetV2/widget/index.tsx index b4af257ee2..0d81424e81 100644 --- a/app/client/src/widgets/ButtonWidgetV2/widget/index.tsx +++ b/app/client/src/widgets/ButtonWidgetV2/widget/index.tsx @@ -47,12 +47,7 @@ class ButtonWidget extends BaseWidget { } static getFeatures() { - return { - dynamicHeight: { - sectionIndex: 0, - active: false, - }, - }; + return null; } static getDefaults() { diff --git a/app/client/src/widgets/WidgetUtils.ts b/app/client/src/widgets/WidgetUtils.ts index e6f9139044..1ccac15da1 100644 --- a/app/client/src/widgets/WidgetUtils.ts +++ b/app/client/src/widgets/WidgetUtils.ts @@ -31,10 +31,10 @@ import { getLocale } from "utils/helpers"; import { DynamicHeight } from "utils/WidgetFeatures"; import type { WidgetPositionProps, WidgetProps } from "./BaseWidget"; import { rgbaMigrationConstantV56 } from "../WidgetProvider/constants"; -import type { ContainerWidgetProps } from "./ContainerWidget/widget"; import type { SchemaItem } from "./JSONFormWidget/constants"; import { WIDGET_COMPONENT_BOUNDARY_CLASS } from "constants/componentClassNameConstants"; import punycode from "punycode"; +import type { FlattenedWidgetProps } from "reducers/entityReducers/canvasWidgetsReducer"; type SanitizeOptions = { existingKeys?: string[]; @@ -772,11 +772,17 @@ export const isAutoHeightEnabledForWidget = (props: WidgetProps) => { /** * Check if a container is scrollable or has scrollbars + * "Container" here is any container like widget (Eg: Container, Tabs, etc) + * @param widget: FlattenedWidgetProps + * returns boolean */ export function checkContainerScrollable( - widget: ContainerWidgetProps, + widget: FlattenedWidgetProps, ): boolean { // if both scrolling and auto height is disabled, container is not scrollable + // If auto height is enabled, the container is expected to be scrollable, + // or the widget should already be in view. + // If auto height is disabled, the container is scrollable only if scrolling is enabled return !( !isAutoHeightEnabledForWidget(widget) && widget.shouldScrollContents === false