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
This commit is contained in:
Abhinav Jha 2023-09-13 11:59:41 +05:30 committed by GitHub
parent cc00c77685
commit e62a2d8eb6
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 78 additions and 28 deletions

View File

@ -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) &&

View File

@ -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)}`);

View File

@ -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)}`,

View File

@ -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<WidgetProps>;
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);
}
}
}

View File

@ -47,12 +47,7 @@ class ButtonWidget extends BaseWidget<ButtonWidgetProps, ButtonWidgetState> {
}
static getFeatures() {
return {
dynamicHeight: {
sectionIndex: 0,
active: false,
},
};
return null;
}
static getDefaults() {

View File

@ -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<WidgetProps>,
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