fix(#4049): Express theme should render a border on Popovers and Dialogs with forced-colors: active #4050

majornista · 2023-02-13T19:08:05Z

Closes #4049

✅ Pull Request Checklist:

Included link to corresponding React Spectrum GitHub Issue #4049.
Added/updated unit tests and storybook for this change (for new code or code which already has tests).
Filled out test instructions.
Updated documentation (if it already exists for this component).
Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

In Chrome, open ActionMenu story using Express theme
In Chrome DevTools, under Rendering > Emulate CSS media feature forced-colors, select forced-colors: active.
Confirm that when expanded, the ActionMenu popover has a 1px border using the foreground color for forced-colors: active.
Repeat for other components that open a dropdown menu or listbox, such as ComboBox, DatePicker, DateRangePicker, MenuTrigger, Picker, and SearchAutocomplete.
Open and test each of the Dialog variants, starting with Dialog -- Default, to verify that Dialogs have a border and border-radius.
Full Screen Takeover stories, Dialog - fullscreenTakeover form, DialogContainer - type: fullscreenTakeover, or DialogTrigger - type: fullscreenTakeover, do not have a border or border-radius on the fullscreenTakeover dialog.

🧢 Your Project:

Adobe/Accessibility

rspbot · 2023-02-13T19:15:27Z

Build successful! 🎉

rspbot · 2023-02-14T14:45:09Z

Build successful! 🎉

rspbot · 2023-02-14T21:44:23Z

Build successful! 🎉

rspbot · 2023-02-15T01:02:42Z

Build successful! 🎉

LFDanLu

LGTM for the most part, the only thing I noticed is the following visual defects in the corner of the ComboBox/SearchAutocomplete/other dropdown menus

More noticeable in the async/long list stories. Not entirely sure what is up, perhaps something to do with the scrollbar + rectangular items

majornista · 2023-02-15T16:10:09Z

LGTM for the most part, the only thing I noticed is the following visual defects in the corner of the ComboBox/SearchAutocomplete/other dropdown menus

More noticeable in the async/long list stories. Not entirely sure what is up, perhaps something to do with the scrollbar + rectangular items

.spectrum-Popover has a border-radius, but does not have overflow: hidden. The .spectrum-Menu contained within the .spectrum-Popover does not have a border-radius, and with forced-colors: active the this overflow overlap at the corners becomes noticable.

We may be able to fix this by adding the following to packages/@adobe/spectrum-css-temp/components/popover/index.css:

@media (forced-colors: active) {
  .spectrum-Popover {
    overflow: hidden;
  }
}

Also, the mixed up colors for the ComboBox "Show suggestions" button in forced-colors mode will need to be resolved.

…active Per #4050 (review)

rspbot · 2023-02-15T16:19:54Z

Build successful! 🎉

rspbot · 2023-02-17T22:26:50Z

Build successful! 🎉

rspbot · 2023-02-22T23:35:25Z

Build successful! 🎉

rspbot · 2023-02-25T00:12:03Z

Build successful! 🎉

rspbot · 2023-08-01T16:55:58Z

Build successful! 🎉

devongovett · 2023-08-01T18:11:12Z

packages/@adobe/spectrum-css-temp/components/popover/index.css

@@ -144,3 +144,10 @@ governing permissions and limitations under the License.
    margin-left: calc(var(--spectrum-global-dimension-size-150) * -1);
  }
 }
+
+@media screen and (max-height: 400px) {


where did this value come from?

Perhaps from here:

react-spectrum/packages/@adobe/spectrum-css-temp/components/dialog/index.css

Line 438 in 9a9327f

@media screen and (max-height: 400px) {

LFDanLu

Verified the border/border radius on a variety of Popovers/Dialogs/Modals/Trays in WHCM. A bit concerned that there are several non forced-colors: active scoped css changes but Chromatic seems to run clean and doesn't look like there are any noticeable differences when I checked manually. Just one thing I noticed with the combobox popover, lemme know if that issue wasn't fixable with the same overflow: hidden

LFDanLu · 2023-08-02T22:32:11Z

packages/@adobe/spectrum-css-temp/components/dropdown/index.css

+.spectrum-Dropdown-popover {
+  overflow: hidden;
+}


Is the ComboBox/SearchAutocomplete popover supposed to have this same overflow: hidden as well? Still seeing the following corner defects:

It should probably have the same overflow: hidden to fix the corner defects.

See e035bc5

I think we can avoid the need for this. In 57bc561 I made it so popovers are overflow: hidden by default, except if they have an arrow. In that case, the dialog inside is overflow: hidden as well. That also fixes the issue with overflow that you had added the extra padding for at 400px breakpoint. The dialog itself should never scroll, because either the content or the grid inside does.

rspbot · 2023-08-03T16:46:59Z

Build successful! 🎉

rspbot · 2023-08-07T18:30:14Z

Build successful! 🎉

LFDanLu · 2023-08-07T20:01:18Z

chromatic from e670de7: https://www.chromatic.com/build?appId=5f0dd5ad2b5fc10022a2e320&number=603

rspbot · 2023-08-07T22:58:58Z

Build successful! 🎉

rspbot · 2023-08-08T18:57:30Z

Build successful! 🎉

rspbot · 2023-08-08T22:22:34Z

Build successful! 🎉

rspbot · 2023-08-08T22:22:36Z

## API Changes

unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }

majornista requested review from devongovett, jnurthen and snowystinger February 13, 2023 19:08

majornista added bug Something isn't working rsp:ComboBox rsp:MenuTrigger rsp:Picker rsp:DialogTrigger accessibility rsp:DatePicker rsp:ActionMenu rsp:SearchAutocomplete labels Feb 13, 2023

majornista added the small review Easy to review PR label Feb 14, 2023

majornista requested review from ktabors, LFDanLu, dannify and reidbarber February 14, 2023 21:35

LFDanLu reviewed Feb 15, 2023

View reviewed changes

majornista added a commit that referenced this pull request Feb 15, 2023

fix(#4049): popover should have overflow: hidden with forced-colors: …

3dbf350

…active Per #4050 (review)

majornista requested a review from LFDanLu February 15, 2023 17:32

majornista added a commit that referenced this pull request Feb 25, 2023

fix(#4050): correct border on correct element with correct border-radius

291e166

majornista force-pushed the Issue-4049 branch from 0023060 to 547b6f1 Compare August 1, 2023 16:47

devongovett reviewed Aug 1, 2023

View reviewed changes

LFDanLu requested changes Aug 2, 2023

View reviewed changes

fix(#4049): use overflow: hidden on popover containing a menu in HCM

e035bc5

majornista requested a review from LFDanLu August 3, 2023 16:47

LFDanLu mentioned this pull request Aug 4, 2023

HCM fixes for ComboBox and Statuslight #4850

Merged

5 tasks

Merge branch 'main' into Issue-4049

e670de7

css fixes for popover scrolling

57bc561

devongovett force-pushed the Issue-4049 branch from e4b0577 to 57bc561 Compare August 7, 2023 22:50

adobe deleted a comment from rspbot Aug 7, 2023

LFDanLu approved these changes Aug 7, 2023

View reviewed changes

LFDanLu added release ready for review labels Aug 7, 2023

Merge branch 'main' into Issue-4049

796778f

yihuiliao approved these changes Aug 8, 2023

View reviewed changes

Merge branch 'main' into Issue-4049

baf2ea0

devongovett merged commit 6795d2a into main Aug 8, 2023

devongovett deleted the Issue-4049 branch August 8, 2023 22:40

devongovett mentioned this pull request Aug 16, 2023

Picker: In windows OS, menu border is getting overlapped by scrollbar edges #4632

Closed

dannify removed release ready for review labels Aug 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(#4049): Express theme should render a border on Popovers and Dialogs with forced-colors: active #4050

fix(#4049): Express theme should render a border on Popovers and Dialogs with forced-colors: active #4050

majornista commented Feb 13, 2023 •

edited by LFDanLu

Loading

rspbot commented Feb 13, 2023

rspbot commented Feb 14, 2023

rspbot commented Feb 14, 2023

rspbot commented Feb 15, 2023

LFDanLu left a comment

majornista commented Feb 15, 2023

rspbot commented Feb 15, 2023

rspbot commented Feb 17, 2023

rspbot commented Feb 22, 2023

rspbot commented Feb 25, 2023

rspbot commented Aug 1, 2023

devongovett Aug 1, 2023

majornista Aug 2, 2023

LFDanLu left a comment

LFDanLu Aug 2, 2023

majornista Aug 3, 2023

majornista Aug 3, 2023

devongovett Aug 7, 2023 •

edited

Loading

rspbot commented Aug 3, 2023

rspbot commented Aug 7, 2023

LFDanLu commented Aug 7, 2023

rspbot commented Aug 7, 2023

rspbot commented Aug 8, 2023

rspbot commented Aug 8, 2023

rspbot commented Aug 8, 2023

fix(#4049): Express theme should render a border on Popovers and Dialogs with forced-colors: active #4050

fix(#4049): Express theme should render a border on Popovers and Dialogs with forced-colors: active #4050

Conversation

majornista commented Feb 13, 2023 • edited by LFDanLu Loading

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

rspbot commented Feb 13, 2023

rspbot commented Feb 14, 2023

rspbot commented Feb 14, 2023

rspbot commented Feb 15, 2023

LFDanLu left a comment

Choose a reason for hiding this comment

majornista commented Feb 15, 2023

rspbot commented Feb 15, 2023

rspbot commented Feb 17, 2023

rspbot commented Feb 22, 2023

rspbot commented Feb 25, 2023

rspbot commented Aug 1, 2023

devongovett Aug 1, 2023

Choose a reason for hiding this comment

majornista Aug 2, 2023

Choose a reason for hiding this comment

LFDanLu left a comment

Choose a reason for hiding this comment

LFDanLu Aug 2, 2023

Choose a reason for hiding this comment

majornista Aug 3, 2023

Choose a reason for hiding this comment

majornista Aug 3, 2023

Choose a reason for hiding this comment

devongovett Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

rspbot commented Aug 3, 2023

rspbot commented Aug 7, 2023

LFDanLu commented Aug 7, 2023

rspbot commented Aug 7, 2023

rspbot commented Aug 8, 2023

rspbot commented Aug 8, 2023

rspbot commented Aug 8, 2023

majornista commented Feb 13, 2023 •

edited by LFDanLu

Loading

devongovett Aug 7, 2023 •

edited

Loading