Skip to content
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
7edf43f
fix: popover positioning
snowystinger Sep 4, 2025
feb6eca
fix the position
snowystinger Sep 4, 2025
79efc7b
add pending state to action buttons
snowystinger Sep 4, 2025
1fbe635
support a default label slot in s2 picker
snowystinger Sep 4, 2025
356292a
make it so cell focus ring is always on top of the content
snowystinger Sep 4, 2025
fb185e5
actually disable interactions on buttons in pending state, including …
snowystinger Sep 4, 2025
7304bf8
fix lint
snowystinger Sep 4, 2025
7e1a5eb
fix more lint
snowystinger Sep 4, 2025
7a76308
fix ActionButton avatar visibility during pending
snowystinger Sep 4, 2025
b2270bd
fix lint for real
snowystinger Sep 4, 2025
d3f1749
fix lint for real__FINAL
snowystinger Sep 4, 2025
336bfc8
SelectBoxGroup shouldn't lose focus ring for mouse like checkbox/radi…
snowystinger Sep 4, 2025
cdbe1da
fix lint...
snowystinger Sep 4, 2025
959e4c5
fix lint and remove extra console log
snowystinger Sep 4, 2025
9f9eba9
fix tests by making them more realistic
snowystinger Sep 4, 2025
6170b84
leave focus events on pending button alone
snowystinger Sep 4, 2025
0df0fbc
Add delays
snowystinger Sep 5, 2025
3d005dc
longer delay? and click body first?
snowystinger Sep 5, 2025
404caee
click body before on mobile as well
snowystinger Sep 5, 2025
c7b5d94
cleanup extraneous timeouts
snowystinger Sep 5, 2025
fc7e685
Merge branch 'main' into fix-various-bugs
snowystinger Sep 8, 2025
0e92920
scuffed fix, figure out if we should be flipping other instances of c…
LFDanLu Sep 9, 2025
2ac39a0
make story more useful
snowystinger Sep 9, 2025
808b199
more half fixes for positioning
snowystinger Sep 9, 2025
ee68148
fix import
snowystinger Sep 9, 2025
d83cf83
fix overlay position!!!!!
snowystinger Sep 9, 2025
229343d
fix html as the container
snowystinger Sep 10, 2025
4097f3c
Merge branch 'main' into fix-various-bugs
snowystinger Sep 10, 2025
35bc27b
fix scrolled case
snowystinger Sep 10, 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
4 changes: 2 additions & 2 deletions packages/@react-aria/overlays/src/calculatePosition.ts
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ function getDelta(
// Note that these values are with respect to the visual viewport (aka 0,0 is the top left of the viewport)
let boundaryStartEdge = boundaryDimensions.scroll[AXIS[axis]] + padding;
let boundaryEndEdge = boundarySize + boundaryDimensions.scroll[AXIS[axis]] - padding;
let startEdgeOffset = offset - containerScroll + containerOffsetWithBoundary[axis] - boundaryDimensions[AXIS[axis]];
let startEdgeOffset = offset - containerScroll + containerOffsetWithBoundary[axis];
Copy link
Member

Choose a reason for hiding this comment

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

I double checked this and found that it was tied to pinch zoom positioning: 3722d30. If you comment out what the startEdge/endEdge values here are, try opening the RAC Popover story outside the iframe, and open the popover after pinch zooming on Safari in such a way that the trigger is close to the edge of the left of the window you'll see that the startEdgeOffset calculation is incorrect (the boundary will return 12, but the start edge will return a value that is much larger). Each of the values here should return coordinate values transformed in such a way that a zoomed in window should have its origin at the top left.

As a result, opening a dropdown when the trigger is partially off screen won't do the following (aka preserve the visibility of the dropdown by keeping it in the bounds of the screen)
image

instead this happens, where the dropdown falls outside of its defined boundary
image

I see the original issue you alluded to in your description, but I think the calculated boundary edges are also incorrect in that story (they currently return 0 and 300 respectively but should be the left and right edge of the box with respect to the top left corner of the story).

In addition, the startEdgeOffset should just be equal to the offset in that case since that value is already in the proper coordinate system, but at the moment it is receiving a containerOffsetWithBoundary that has negative values that should instead be offsetting the value of boundaryDimensions (aka containerOffsetWithBoundary is representing how far offset the boundary element is with respect to the containing element aka the page). I think we'll need to

  1. fix the boundaryStartEdge and boundaryEndEdge calcs here
  2. Change it to be - containerOffsetWithBoundary[axis] - boundaryDimensions[AXIS[axis]]; or update how containerOffsetWithBoundary is being calculated to return positive values (this one is probably more correct if we consider the top left corner to be the 0,0 origin and there for the offset should be positive instead of negative if the container is centered in the page)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahhh, thank you, I'll have a look on Monday

Copy link
Member

Choose a reason for hiding this comment

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

no worries, happy to sit with you and debug if you'd like as well, this stuff is tricky :/

let endEdgeOffset = offset - containerScroll + size + containerOffsetWithBoundary[axis] - boundaryDimensions[AXIS[axis]];

// If any of the overlay edges falls outside of the boundary, shift the overlay the required amount to align one of the overlay's
Expand Down Expand Up @@ -536,7 +536,7 @@ export function calculatePosition(opts: PositionOpts): PositionResult {
export function getRect(node: Element, ignoreScale: boolean) {
let {top, left, width, height} = node.getBoundingClientRect();

// Use offsetWidth and offsetHeight if this is an HTML element, so that
// Use offsetWidth and offsetHeight if this is an HTML element, so that
// the size is not affected by scale transforms.
if (ignoreScale && node instanceof node.ownerDocument.defaultView!.HTMLElement) {
width = node.offsetWidth;
Expand Down
131 changes: 99 additions & 32 deletions packages/@react-spectrum/s2/src/ActionButton.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,18 @@ import {control, getAllowedOverrides, staticColor, StyleProps} from './style-uti
import {createContext, forwardRef, ReactNode, useContext} from 'react';
import {FocusableRef, FocusableRefValue, GlobalDOMAttributes} from '@react-types/shared';
import {IconContext} from './Icon';
import {ImageContext} from './Image';
// @ts-ignore
import intlMessages from '../intl/*.json';
import {NotificationBadgeContext} from './NotificationBadge';
import {pressScale} from './pressScale';
import {ProgressCircle} from './ProgressCircle';
import {SkeletonContext} from './Skeleton';
import {Text, TextContext} from './Content';
import {useFocusableRef} from '@react-spectrum/utils';
import {useFormProps} from './Form';
import {useLocalizedStringFormatter} from '@react-aria/i18n';
import {usePendingState} from './Button';
import {useSpectrumContextProps} from './useSpectrumContextProps';

export interface ActionButtonStyleProps {
Expand Down Expand Up @@ -53,7 +59,7 @@ interface ActionGroupItemStyleProps {
isJustified?: boolean
}

export interface ActionButtonProps extends Omit<ButtonProps, 'className' | 'style' | 'children' | 'onHover' | 'onHoverStart' | 'onHoverEnd' | 'onHoverChange' | 'isPending' | 'onClick' | keyof GlobalDOMAttributes>, StyleProps, ActionButtonStyleProps {
export interface ActionButtonProps extends Omit<ButtonProps, 'className' | 'style' | 'children' | 'onHover' | 'onHoverStart' | 'onHoverEnd' | 'onHoverChange' | 'onClick' | keyof GlobalDOMAttributes>, StyleProps, ActionButtonStyleProps {
/** The content to display in the ActionButton. */
children: ReactNode
}
Expand All @@ -67,6 +73,7 @@ export const btnStyles = style<ButtonRenderProps & ActionButtonStyleProps & Togg
...focusRing(),
...staticColor(),
...controlStyle,
position: 'relative',
justifyContent: 'center',
flexShrink: {
default: 1,
Expand Down Expand Up @@ -252,6 +259,8 @@ export const ActionButtonContext = createContext<ContextValue<Partial<ActionButt
export const ActionButton = forwardRef(function ActionButton(props: ActionButtonProps, ref: FocusableRef<HTMLButtonElement>) {
[props, ref] = useSpectrumContextProps(props, ref, ActionButtonContext);
props = useFormProps(props as any);
let stringFormatter = useLocalizedStringFormatter(intlMessages, '@react-spectrum/s2');
let {isPending = false} = props;
let domRef = useFocusableRef(ref);
let overlayTriggerState = useContext(OverlayTriggerStateContext);
let ctx = useSlottedContext(ActionButtonGroupContext);
Expand All @@ -262,21 +271,21 @@ export const ActionButton = forwardRef(function ActionButton(props: ActionButton
orientation = 'horizontal',
staticColor = props.staticColor,
isQuiet = props.isQuiet,
size = props.size || 'M',
isDisabled = props.isDisabled
size = props.size || 'M'
} = ctx || {};

let {isProgressVisible} = usePendingState(isPending);

return (
<RACButton
{...props}
isDisabled={isDisabled}
ref={domRef}
style={pressScale(domRef, props.UNSAFE_style)}
className={renderProps => (props.UNSAFE_className || '') + btnStyles({
...renderProps,
// Retain hover styles when an overlay is open.
isHovered: renderProps.isHovered || overlayTriggerState?.isOpen || false,
isDisabled: renderProps.isDisabled || isProgressVisible,
staticColor,
isStaticColor: !!staticColor,
size,
Expand All @@ -286,34 +295,92 @@ export const ActionButton = forwardRef(function ActionButton(props: ActionButton
orientation,
isInGroup
}, props.styles)}>
<Provider
values={[
[SkeletonContext, null],
[TextContext, {styles: style({order: 1, truncate: true})}],
[IconContext, {
render: centerBaseline({slot: 'icon', styles: style({order: 0})}),
styles: style({size: fontRelative(20), marginStart: '--iconMargin', flexShrink: 0})
}],
[AvatarContext, {
size: avatarSize[size],
styles: style({
marginStart: {
default: '--iconMargin',
':last-child': 0
},
flexShrink: 0,
order: 0
})
}],
[NotificationBadgeContext, {
staticColor: staticColor,
size: props.size === 'XS' ? undefined : props.size,
isDisabled: props.isDisabled,
styles: style({position: 'absolute', top: '--badgeTop', insetStart: '--badgePosition', marginTop: 'calc((self(height) * -1)/2)', marginStart: 'calc((self(height) * -1)/2)'})
}]
]}>
{typeof props.children === 'string' ? <Text>{props.children}</Text> : props.children}
</Provider>
{({isDisabled}) => (
<>
<Provider
values={[
[SkeletonContext, null],
[TextContext, {styles:
style({
order: 1,
truncate: true,
opacity: {
default: 1,
isProgressVisible: 0
}
})({isProgressVisible})
}],
[IconContext, {
render: centerBaseline({slot: 'icon', styles: style({order: 0})}),
styles: style({
size: fontRelative(20),
marginStart: '--iconMargin',
flexShrink: 0,
opacity: {
default: 1,
isProgressVisible: 0
}
})({isProgressVisible})
}],
[AvatarContext, {
size: avatarSize[size],
styles: style({
marginStart: {
default: '--iconMargin',
':last-child': 0
},
flexShrink: 0,
order: 0
})
}],
[ImageContext, {
Copy link
Member Author

Choose a reason for hiding this comment

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

should we just add opacity to be overridable to every component? visibility?

styles: style({
opacity: {
default: 1,
isProgressVisible: 0
}
})({isProgressVisible})
}],
[NotificationBadgeContext, {
staticColor: staticColor,
size: props.size === 'XS' ? undefined : props.size,
isDisabled: isDisabled,
styles: style({position: 'absolute', top: '--badgeTop', insetStart: '--badgePosition', marginTop: 'calc((self(height) * -1)/2)', marginStart: 'calc((self(height) * -1)/2)'})
Copy link
Member Author

Choose a reason for hiding this comment

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

probably shouldn't use notifications and pending together? very much an edge case

Copy link
Member

Choose a reason for hiding this comment

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

i assume it would disappear or something but i doubt design as thought of this case

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, i'm leaving this alone for now

}]
]}>
{typeof props.children === 'string' ? <Text>{props.children}</Text> : props.children}
{isPending &&
<div
className={style({
position: 'absolute',
top: '50%',
left: '50%',
transform: 'translate(-50%, -50%)',
opacity: {
default: 0,
isProgressVisible: 1
}
})({isProgressVisible, isPending})}>
<ProgressCircle
isIndeterminate
aria-label={stringFormatter.format('button.pending')}
size="S"
staticColor={staticColor}
styles={style({
size: {
size: {
S: 14,
M: 18,
L: 20,
XL: 24
}
}
})({size})} />
</div>
}
</Provider>
</>
)}
</RACButton>
);
});
39 changes: 20 additions & 19 deletions packages/@react-spectrum/s2/src/Button.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,24 @@ const gradient = style({
}
});

export function usePendingState(isPending: boolean) {
let [isProgressVisible, setIsProgressVisible] = useState(false);
useEffect(() => {
let timeout: ReturnType<typeof setTimeout>;
if (isPending) {
timeout = setTimeout(() => {
setIsProgressVisible(true);
}, 1000);
} else {
setIsProgressVisible(false);
}
return () => {
clearTimeout(timeout);
};
}, [isPending]);
return {isProgressVisible};
}

/**
* Buttons allow users to perform an action.
* They have multiple styles for various needs, and are ideal for calling attention to
Expand All @@ -302,7 +320,7 @@ export const Button = forwardRef(function Button(props: ButtonProps, ref: Focusa
props = useFormProps(props);
let stringFormatter = useLocalizedStringFormatter(intlMessages, '@react-spectrum/s2');
let {
isPending,
isPending = false,
variant = 'primary',
fillStyle = 'fill',
size = 'M',
Expand All @@ -311,24 +329,7 @@ export const Button = forwardRef(function Button(props: ButtonProps, ref: Focusa
let domRef = useFocusableRef(ref);
let overlayTriggerState = useContext(OverlayTriggerStateContext);

let [isProgressVisible, setIsProgressVisible] = useState(false);
useEffect(() => {
let timeout: ReturnType<typeof setTimeout>;

if (isPending) {
// Start timer when isPending is set to true.
timeout = setTimeout(() => {
setIsProgressVisible(true);
}, 1000);
} else {
// Exit loading state when isPending is set to false. */
setIsProgressVisible(false);
}
return () => {
// Clean up on unmount or when user removes isPending prop before entering loading state.
clearTimeout(timeout);
};
}, [isPending]);
let {isProgressVisible} = usePendingState(isPending);

return (
<RACButton
Expand Down
7 changes: 7 additions & 0 deletions packages/@react-spectrum/s2/src/Picker.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
ButtonRenderProps,
Collection,
ContextValue,
DEFAULT_SLOT,
ListBox,
ListBoxItem,
ListBoxItemProps,
Expand Down Expand Up @@ -526,6 +527,11 @@ const PickerButton = createHideableComponent(function PickerButton<T extends obj
[TextContext, {
slots: {
description: {},
[DEFAULT_SLOT]: {styles: style({
Copy link
Member Author

Choose a reason for hiding this comment

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

this is already how we do it in Menu and ComboBox

display: 'block',
flexGrow: 1,
truncate: true
})},
label: {styles: style({
display: 'block',
flexGrow: 1,
Expand Down Expand Up @@ -592,6 +598,7 @@ export function PickerItem(props: PickerItemProps): ReactNode {
context={TextContext}
value={{
slots: {
[DEFAULT_SLOT]: {styles: label({size})},
label: {styles: label({size})},
description: {styles: description({...renderProps, size})}
}
Expand Down
26 changes: 12 additions & 14 deletions packages/@react-spectrum/s2/src/SelectBoxGroup.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {IllustrationContext} from '../src/Icon';
import {pressScale} from './pressScale';
import React, {createContext, forwardRef, ReactNode, useContext, useMemo, useRef} from 'react';
import {TextContext} from './Content';
import {useFocusVisible} from 'react-aria';
import {useSpectrumContextProps} from './useSpectrumContextProps';

export interface SelectBoxGroupProps<T> extends StyleProps, Omit<ListBoxProps<T>, keyof GlobalDOMAttributes | 'layout' | 'dragAndDropHooks' | 'dependencies' | 'renderEmptyState' | 'children' | 'onAction' | 'shouldFocusOnHover' | 'selectionBehavior' | 'shouldSelectOnPressUp' | 'shouldFocusWrap' | 'style' | 'className'> {
Expand Down Expand Up @@ -293,29 +294,31 @@ const gridStyles = style<{orientation?: Orientation}>({
*/
export function SelectBox(props: SelectBoxProps): ReactNode {
let {children, isDisabled: individualDisabled = false, UNSAFE_style, UNSAFE_className, styles, ...otherProps} = props;

let {
orientation = 'vertical',
isDisabled: groupDisabled = false
} = useContext(SelectBoxContext);

const isDisabled = individualDisabled || groupDisabled;
const ref = useRef<HTMLDivElement>(null);
let {isFocusVisible} = useFocusVisible();

return (
<ListBoxItem
isDisabled={isDisabled}
ref={ref}
className={renderProps => (UNSAFE_className || '') + selectBoxStyles({
...renderProps,
isFocusVisible: isFocusVisible && renderProps.isFocused,
Copy link
Member Author

Choose a reason for hiding this comment

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

This is because Menu/Picker would remove the focus ring if the mouse moves over it, but a SelectBox should behave like a Radio or Checkbox, and should not have this behaviour

orientation
}, styles)}
style={pressScale(ref, UNSAFE_style)}
{...otherProps}>
{({isSelected, isDisabled, isHovered}) => {
return (
<>
<div
<div
className={style({
position: 'absolute',
top: 8,
Expand All @@ -330,8 +333,8 @@ export function SelectBox(props: SelectBoxProps): ReactNode {
isDisabled,
size: 'M'
} as any)}>
<Checkmark
size="S"
<Checkmark
size="S"
className={iconStyles} />
</div>
)}
Expand Down Expand Up @@ -382,21 +385,16 @@ export const SelectBoxGroup = /*#__PURE__*/ forwardRef(function SelectBoxGroup<T
...otherProps
} = props;

const selectBoxContextValue = useMemo(
() => {
const contextValue = {
orientation,
isDisabled
};
return contextValue;
},
[orientation, isDisabled]
);
const selectBoxContextValue = useMemo(() => ({
orientation,
isDisabled
}), [orientation, isDisabled]);

return (
<ListBox
selectionMode={selectionMode}
layout="grid"
orientation={orientation}
className={(UNSAFE_className || '') + gridStyles({orientation}, styles)}
style={UNSAFE_style}
{...otherProps}>
Expand Down
5 changes: 3 additions & 2 deletions packages/@react-spectrum/s2/src/TableView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -457,7 +457,8 @@ const cellFocus = {
outlineOffset: -2,
outlineWidth: 2,
outlineColor: 'focus-ring',
borderRadius: '[6px]'
borderRadius: '[6px]',
pointerEvents: 'none'
} as const;

function CellFocusRing() {
Expand Down Expand Up @@ -1035,8 +1036,8 @@ export const Cell = forwardRef(function Cell(props: CellProps, ref: DOMRef<HTMLD
{...otherProps}>
{({isFocusVisible}) => (
<>
{isFocusVisible && <CellFocusRing />}
<span className={cellContent({...tableVisualOptions, isSticky, align: align || 'start'})}>{children}</span>
{isFocusVisible && <CellFocusRing />}
</>
)}
</RACCell>
Expand Down
Loading