Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Revert "Revert "feat: Added focus ring for focus visible (#37700)" (#… #38655

Merged
merged 17 commits into from
Feb 3, 2025
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
0cec0af
Revert "Revert "feat: Added focus ring for focus visible (#37700)" (#…
albinAppsmith Jan 15, 2025
f8bb57f
Merge branch 'release' of github.com:appsmithorg/appsmith into focus-…
albinAppsmith Jan 15, 2025
2ff3d4b
Merge branch 'release' of github.com:appsmithorg/appsmith into focus-…
albinAppsmith Jan 15, 2025
271ff45
Merge branch 'release' of github.com:appsmithorg/appsmith into focus-…
albinAppsmith Jan 23, 2025
bc6028e
Merge branch 'release' of github.com:appsmithorg/appsmith into focus-…
albinAppsmith Jan 23, 2025
a743c64
fix: updated cypress failures for focus ring
albinAppsmith Jan 23, 2025
734fd4a
Merge branch 'release' of github.com:appsmithorg/appsmith into focus-…
albinAppsmith Jan 23, 2025
2f6c1d6
fix: reverted propertypanesearch input changes
albinAppsmith Jan 24, 2025
4d8d3f7
fix: skipped focus tests
albinAppsmith Jan 24, 2025
16642bd
Merge branch 'release' of github.com:appsmithorg/appsmith into focus-…
albinAppsmith Jan 24, 2025
38905b5
fix: propertry pane search test case
albinAppsmith Jan 24, 2025
488f21c
Merge branch 'release' of github.com:appsmithorg/appsmith into focus-…
albinAppsmith Jan 30, 2025
d3cbfce
fix: eslint error
albinAppsmith Jan 30, 2025
7b48b1f
Merge branch 'release' of github.com:appsmithorg/appsmith into focus-…
albinAppsmith Jan 31, 2025
9b5c755
Merge branch 'release' of github.com:appsmithorg/appsmith into focus-…
albinAppsmith Jan 31, 2025
e6a4968
fix :focus ring in EditableEntityName
albinAppsmith Jan 31, 2025
7f7dcd2
Merge branch 'release' of github.com:appsmithorg/appsmith into focus-…
albinAppsmith Feb 3, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -17,26 +17,26 @@ describe("Property Pane Search", { tags: ["@tag.PropertyPane"] }, function () {

// Initially the search input will only be soft focused
// We need to press Enter to properly focus it
agHelper.AssertElementFocus(propPane._propertyPaneSearchInputWrapper);
agHelper.AssertElementFocus(propPane._propertyPaneSearchInput);
agHelper.PressEnter();
agHelper.AssertElementFocus(propPane._propertyPaneSearchInput);

// Pressing Escape should soft focus the search input
agHelper.PressEscape();
agHelper.AssertElementFocus(propPane._propertyPaneSearchInputWrapper);
agHelper.AssertElementFocus(propPane._propertyPaneSearchInput);

// Opening a panel should focus the search input
propPane.OpenTableColumnSettings("name");
agHelper.AssertElementFocus(propPane._propertyPaneSearchInputWrapper);
agHelper.AssertElementFocus(propPane._propertyPaneSearchInput);

// Opening some other widget and then going back to the initial widget should soft focus the search input that is inside a panel
EditorNavigation.SelectEntityByName("Switch1", EntityType.Widget);
EditorNavigation.SelectEntityByName("Table1", EntityType.Widget);
agHelper.AssertElementFocus(propPane._propertyPaneSearchInputWrapper);
agHelper.AssertElementFocus(propPane._propertyPaneSearchInput);

// Going out of the panel should soft focus the search input
propPane.NavigateBackToPropertyPane();
agHelper.AssertElementFocus(propPane._propertyPaneSearchInputWrapper);
agHelper.AssertElementFocus(propPane._propertyPaneSearchInput);
});

it("2. Search for Properties", function () {
Expand Down
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Invalid image source.

Is this expected?

Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,11 @@ const SearchInputWrapper = styled(InputGroup)<{
text-overflow: ellipsis;
width: 100%;
}
input:focus {
border: 1.2px solid var(--ads-old-color-fern-green);
box-sizing: border-box;
width: 100%;
outline: var(--ads-v2-border-width-outline) solid
var(--ads-v2-color-outline);
outline-offset: var(--ads-v2-offset-outline);
}
input:active {
Expand Down
16 changes: 10 additions & 6 deletions app/client/packages/design-system/ads/src/Button/Button.styles.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,7 @@ export const StyledButton = styled.button<{
UNSAFE_width?: string;
disabled?: boolean;
isIconButton?: boolean;
isFocusVisible?: boolean;
}>`
${Variables}
/* Kind style */
Expand Down Expand Up @@ -269,11 +270,14 @@ export const StyledButton = styled.button<{
}
}

/* Focus styles */
&:focus-visible {
outline: var(--ads-v2-border-width-outline) solid
var(--ads-v2-color-outline);
outline-offset: var(--ads-v2-offset-outline);
}
${({ isFocusVisible }) =>
isFocusVisible &&
css`
:focus-visible {
outline: var(--ads-v2-border-width-outline) solid
var(--ads-v2-color-outline) !important;
outline-offset: var(--ads-v2-offset-outline) !important;
}
`}
}
`;
18 changes: 13 additions & 5 deletions app/client/packages/design-system/ads/src/Button/Button.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import React, { forwardRef } from "react";
import clsx from "classnames";
import { useFocusRing } from "@react-aria/focus";

import { StyledButton, ButtonContent } from "./Button.styles";
import type { ButtonProps } from "./Button.types";
Expand All @@ -16,6 +17,11 @@ import {
} from "./Button.constants";
import { Spinner } from "../Spinner";

// Add this before the Button component definition
const SPINNER_ICON_PROPS = {
className: ButtonLoadingIconClassName,
};

/**
* TODO:
* - if both left and right icon is used, disregard left icon and print a warning in the console.
Expand All @@ -37,21 +43,25 @@ const Button = forwardRef<HTMLButtonElement, ButtonProps>(
...rest
} = props;

// disable button when loading
rest.onClick =
// Replace the direct mutation of rest.onClick with this
const handleClick =
props.isLoading || props.isDisabled ? undefined : props.onClick;
const buttonRef = useDOMRef(ref);
const { focusProps, isFocusVisible } = useFocusRing();

return (
<StyledButton
as={renderAs || "button"}
{...rest}
onClick={handleClick}
{...focusProps}
UNSAFE_height={UNSAFE_height}
UNSAFE_width={UNSAFE_width}
className={clsx(ButtonClassName, className)}
data-disabled={props.isDisabled || false}
data-loading={isLoading}
disabled={props.isDisabled}
isFocusVisible={isFocusVisible}
isIconButton={isIconButton}
kind={kind}
ref={buttonRef}
Expand All @@ -61,9 +71,7 @@ const Button = forwardRef<HTMLButtonElement, ButtonProps>(
{isLoading === true && (
<Spinner
className={ButtonLoadingClassName}
iconProps={{
className: ButtonLoadingIconClassName,
}}
iconProps={SPINNER_ICON_PROPS}
size="md"
/>
)}
Expand Down
22 changes: 10 additions & 12 deletions app/client/packages/design-system/ads/src/Input/Input.styles.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ export const StyledInput = styled.input<{
renderer === "input" &&
css`
padding-left: calc((var(--input-padding-x) * 2) + var(--icon-size) - 1px);
`};
`}

/* adjust padding end according to icon present or not */
${({ hasEndIcon, renderer }) =>
Expand All @@ -174,20 +174,13 @@ export const StyledInput = styled.input<{
padding-right: calc(
(var(--input-padding-x) * 2) + var(--icon-size) - 1px
);
`};

&:focus:enabled:not(:read-only) {
outline: var(--ads-v2-border-width-outline) solid
var(--ads-v2-color-outline);
outline-offset: var(--ads-v2-offset-outline);
}
`}

&:hover:enabled:not(:read-only) {
--input-color-border: var(--ads-v2-colors-control-field-hover-border);
}

&:active:enabled:not(:read-only),
&:focus:enabled:not(:read-only) {
&:active:enabled:not(:read-only) {
--input-color-border: var(--ads-v2-colors-control-field-active-border);
}

Expand All @@ -203,11 +196,16 @@ export const StyledInput = styled.input<{
--input-color-border: var(--ads-v2-colors-control-field-error-border);
}

&:active:enabled:not(:read-only),
&:focus:enabled:not(:read-only) {
&:active:enabled:not(:read-only) {
--input-color-border: var(--ads-v2-colors-control-field-error-border);
}
}

&:focus {
outline: var(--ads-v2-border-width-outline) solid
var(--ads-v2-color-outline) !important;
outline-offset: var(--ads-v2-offset-outline) !important;
}
`;

export const Description = styled(Text)`
Expand Down
16 changes: 8 additions & 8 deletions app/client/packages/design-system/ads/src/Input/Input.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import React, { forwardRef } from "react";
import { useFocusRing } from "@react-aria/focus";
import React, { forwardRef, useCallback } from "react";
import { useTextField } from "@react-aria/textfield";
import clsx from "classnames";

Expand Down Expand Up @@ -55,7 +54,7 @@ const Input = forwardRef<HTMLInputElement, InputProps>(
const { descriptionProps, errorMessageProps, inputProps, labelProps } =
// @ts-expect-error fix this the next time the file is edited
useTextField(props, inputRef);
const { focusProps, isFocusVisible } = useFocusRing();

const {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix the type assertion.

The @ts-expect-error comment indicates a known type issue that needs to be addressed.

Consider fixing the type issue by:

  1. Adding proper types for useTextField hook
  2. Updating the component props interface

className: startIconClassName,
onClick: startIconOnClick,
Expand All @@ -67,9 +66,12 @@ const Input = forwardRef<HTMLInputElement, InputProps>(
...restOfEndIconProps
} = endIconProps || {};

const handleOnChange = (event: React.ChangeEvent<HTMLInputElement>) => {
onChange?.(event.target.value);
};
const handleOnChange = useCallback(
(event: React.ChangeEvent<HTMLInputElement>) => {
onChange?.(event.target.value);
},
[onChange],
);

isValid = isValid === undefined ? !errorMessage : isValid;

Expand Down Expand Up @@ -116,7 +118,6 @@ const Input = forwardRef<HTMLInputElement, InputProps>(
<StyledInput
as={renderAs}
type={type}
{...focusProps}
{...inputProps}
UNSAFE_height={UNSAFE_height}
UNSAFE_width={UNSAFE_width}
Expand All @@ -126,7 +127,6 @@ const Input = forwardRef<HTMLInputElement, InputProps>(
hasEndIcon={!!endIcon}
hasStartIcon={!!startIcon}
inputSize={size}
isFocusVisible={isFocusVisible}
onChange={handleOnChange}
readOnly={isReadOnly}
ref={inputRef}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,8 @@ export const Styles = css<{ kind?: LinkKind }>`
:focus-visible {
outline: var(--ads-v2-border-width-outline) solid
var(--ads-v2-color-outline);
outline-offset: var(--ads-v2-offset-outline);
var(--ads-v2-color-outline) !important;
outline-offset: var(--ads-v2-offset-outline) !important;
border-radius: var(--ads-v2-border-radius);
}
`;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,8 @@ export const StyledControlContainer = styled.div`
&:focus-visible {
outline: var(--ads-v2-border-width-outline) solid
var(--ads-v2-color-outline);
outline-offset: var(--ads-v2-offset-outline);
var(--ads-v2-color-outline) !important;
outline-offset: var(--ads-v2-offset-outline) !important;
}
&[data-disabled="true"] {
Expand Down
12 changes: 9 additions & 3 deletions app/client/packages/design-system/ads/src/Select/styles.css
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
.ads-v2-select {
--select-color-border: var(--ads-v2-colors-control-field-default-border);
--select-padding-x: var(--ads-v2-spaces-2); /* padding left and right */
--select-padding-y: var(--ads-v2-spaces-2); /* padding top and bottom */
--select-padding-x: var(--ads-v2-spaces-2);
/* padding left and right */
--select-padding-y: var(--ads-v2-spaces-2);
/* padding top and bottom */
--select-font-size: var(--ads-v2-font-size-2);
--select-height: 24px;
--select-search-input-padding-right: 0;
Expand Down Expand Up @@ -51,7 +53,6 @@
.ads-v2-select.rc-select-focused {
outline: var(--ads-v2-border-width-outline) solid var(--ads-v2-color-outline);
outline-offset: var(--ads-v2-offset-outline);
--select-color-border: var(--ads-v2-colors-control-field-active-border);
}

/* Error */
Expand Down Expand Up @@ -87,6 +88,7 @@
/* padding x + icon size + gap */
--select-search-input-padding-right: calc(var(--select-padding-x) + 16px);
}

.ads-v2-select.rc-select-allow-clear.rc-select-show-arrow.rc-select-selection-search-input,
.ads-v2-select.rc-select-allow-clear.rc-select-show-arrow
.rc-select-selection-overflow {
Expand All @@ -113,6 +115,10 @@
overflow: hidden;
}

.ads-v2-select.rc-select-focused > .rc-select-selector {
border-color: transparent;
}

/* Placeholder */
.ads-v2-select > .rc-select-selector > .rc-select-selection-placeholder,
.ads-v2-select > .rc-select-selector > .rc-select-selection-item {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,9 @@ export const StyledSwitchInput = styled.input<{
${({ isFocusVisible }) =>
isFocusVisible &&
`
outline: var(--ads-v2-border-width-outline) solid var(--ads-v2-color-outline);
outline-offset: var(--ads-v2-offset-outline);
outline: var(--ads-v2-border-width-outline) solid
var(--ads-v2-color-outline) !important;
outline-offset: var(--ads-v2-offset-outline) !important;
`}

&:hover {
Expand Down
4 changes: 2 additions & 2 deletions app/client/packages/design-system/ads/src/Tab/Tab.styles.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ export const StyledTab = styled(RadixTabs.TabsTrigger)`
&:focus-visible {
--tab-color: var(--ads-v2-colors-content-label-default-fg);
outline: var(--ads-v2-border-width-outline) solid
var(--ads-v2-color-outline);
outline-offset: var(--ads-v2-offset-outline);
var(--ads-v2-color-outline) !important;
outline-offset: var(--ads-v2-offset-outline) !important;
}
`;
Original file line number Diff line number Diff line change
Expand Up @@ -201,8 +201,13 @@ export const StyledEditableInput = styled.input`
border-color: var(--ads-v2-colors-control-field-hover-border);
}

&:focus,
&:active {
border-color: var(--ads-v2-colors-control-field-active-border);
}

&:focus-visible {
outline: var(--ads-v2-border-width-outline) solid
var(--ads-v2-color-outline);
outline-offset: var(--ads-v2-offset-outline);
}
`;
Original file line number Diff line number Diff line change
Expand Up @@ -239,9 +239,9 @@
/* --ads-v2-color-outline: var(--ads-v2-color-blue-300);
--ads-v2-border-width-outline: 2px; */
/* TODO: fix focus issue across the platform */
--ads-v2-color-outline: transparent;
--ads-v2-border-width-outline: 0;
--ads-v2-offset-outline: 0;
--ads-v2-color-outline: var(--ads-v2-color-blue-300);
--ads-v2-border-width-outline: 2px;
--ads-v2-offset-outline: -2px;

/**
* ===========================================*
Expand All @@ -251,17 +251,22 @@
--ads-v2-opacity-disabled: 0.6;
}

:focus {
outline: none !important;
}

/* react-aria useFocusRing helper class*/
.ads-v2-focus-ring {
outline: var(--ads-v2-border-width-outline) solid var(--ads-v2-color-outline);
outline-offset: var(--ads-v2-offset-outline);
outline: var(--ads-v2-border-width-outline) solid var(--ads-v2-color-outline) !important;
outline-offset: var(--ads-v2-offset-outline) !important;
}

/* Placeholder color */
::placeholder {
/* This needs to be important to override the placeholder color on main repo */
color: var(--ads-v2-colors-control-placeholder-default-fg) !important;
opacity: 1; /* Firefox */
opacity: 1;
/* Firefox */
}

:-ms-input-placeholder {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1794,6 +1794,7 @@ class CodeEditor extends Component<Props, State> {
onMouseOver={this.handleMouseMove}
ref={this.editorWrapperRef}
removeHoverAndFocusStyle={this.props?.removeHoverAndFocusStyle}
showFocusVisible={!this.props.isJSObject}
size={size}
>
{this.state.peekOverlayProps && (
Expand Down
Loading
Loading