chore(picker): fix for overlaid picker in mobile #3835

Rajdeepc · 2023-11-29T16:31:58Z

Description

type=auto was taking up the root dialog component as its parent and rendering the sp-tray considering dialog as its root instead of the DOM.

Related issue(s)

[Bug]: swc-react picker is not working on an overlay in mobile only in Firefly #3834

Motivation and context

How has this been tested?

Test case 1
Do this:

Add the below code in picker.stories.ts and try to emulate it in a iphone simulatorwith xcode.

<overlay-trigger type="modal" open="click">
  <div slot="click-content" role="dialog">
    <sp-underlay style="z-index: 10000;"></sp-underlay>
    <div style="  position: fixed;top: 50%;left: 50%;transform: translate(-50%, -50%);z-index: 10001; background-color: yellow; padding: 20px">
      <div style="display: flex; flex-direction: column; gap: 40px">
        <sp-picker id="picker-m" size="m" label="Selection type" ${spreadProps(args)}>
          <sp-menu-item>Deselect</sp-menu-item>
          <sp-menu-item>Select inverse</sp-menu-item>
          <sp-menu-item>Feather...</sp-menu-item>
          <sp-menu-item>Select and mask...</sp-menu-item>
          <sp-menu-divider></sp-menu-divider>
          <sp-menu-item>Save selection</sp-menu-item>
          <sp-menu-item disabled>Make work path</sp-menu-item>
        </sp-picker>
        <sp-picker id="picker-m" size="m" label="Selection type">
          <sp-menu-item>Deselect</sp-menu-item>
          <sp-menu-item>Select inverse</sp-menu-item>
          <sp-menu-item>Feather...</sp-menu-item>
          <sp-menu-item>Select and mask...</sp-menu-item>
          <sp-menu-item>Save selection</sp-menu-item>
          <sp-menu-item disabled>Make work path</sp-menu-item>
        </sp-picker>
      </div>
    </div>
</overlay-trigger>

Screenshots (if appropriate)

Before Fix:

After Fix:

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Checklist

I have signed the Adobe Open Source CLA.
My code follows the code style of this project.
If my change required a change to the documentation, I have updated the documentation in this pull request.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.
I have reviewed at the Accessibility Practices for this feature, see: Aria Practices

Best practices

This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against main.

github-actions · 2023-11-29T16:34:36Z

Branch Preview

Visual regression test results

When a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:

github-actions · 2023-11-29T16:48:26Z

Tachometer results

Chrome

action-menu permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	640 kB	192.35ms - 196.55ms	-	unsure 🔍 -1% - +2% -1.93ms - +3.57ms
branch	623 kB	191.84ms - 195.41ms	unsure 🔍 -2% - +1% -3.57ms - +1.93ms	-

picker permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	506 kB	662.72ms - 678.50ms	-	unsure 🔍 -1% - +2% -7.72ms - +13.27ms
branch	493 kB	660.92ms - 674.76ms	unsure 🔍 -2% - +1% -13.27ms - +7.72ms	-

split-button permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	712 kB	1854.52ms - 1857.53ms	-	unsure 🔍 -0% - +0% -4.33ms - +0.55ms
branch	697 kB	1855.99ms - 1859.84ms	unsure 🔍 -0% - +0% -0.55ms - +4.33ms	-

Firefox

action-menu permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	640 kB	330.73ms - 341.11ms	-	unsure 🔍 -2% - +2% -6.55ms - +7.03ms
branch	623 kB	331.30ms - 340.06ms	unsure 🔍 -2% - +2% -7.03ms - +6.55ms	-

picker permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	506 kB	980.41ms - 1003.39ms	-	unsure 🔍 -1% - +2% -8.90ms - +14.86ms
branch	493 kB	985.91ms - 991.93ms	unsure 🔍 -1% - +1% -14.86ms - +8.90ms	-

split-button permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	712 kB	1568.49ms - 1571.95ms	-	unsure 🔍 -0% - +0% -3.37ms - +1.57ms
branch	697 kB	1569.36ms - 1572.88ms	unsure 🔍 -0% - +0% -1.57ms - +3.37ms	-

packages/picker/src/Picker.ts

Westbrook · 2023-11-30T03:06:31Z

https://96f1ac07a365f3271d6365f89114585a--spectrum-web-components.netlify.app/review/ This is an unexpected failure. We should confirm that your change didn’t cause this or if there’s a change to the timing of the test/element that is needed.

Rajdeepc · 2023-11-30T11:24:15Z

here’s a change to the timing of the test/element that is needed

I dont think this is failing due to change in type. I verified in a test PR. I can see the same VRT failures

jnjosh · 2023-11-30T13:49:59Z

packages/picker/src/Picker.ts

    public placement: Placement = 'bottom-start';

+    @property()
+    public overlayType: OverlayTypes = 'auto';


Should we update documentation somewhere on this property? I feel like the isMobile check where it changes to modal is a potential surprise to consumers. Can we also add/update any documentation for this property to communicate what it does?

I wasn’t clear about what I was hoping with this. I do not believe the user should have any control over this value.

However, we should be binding it to the sp-overlay as a property .type=${…} now that is dynamic so that it does not request paint/layout when changing unless required.

Adding as .type=${this.isMobile.matches ? 'modal' : 'auto'}

Westbrook · 2023-11-30T14:11:19Z

Please share why you think the change I referred to is not caused by this PR.

Rajdeepc · 2023-12-01T06:35:09Z

Please share why you think the change I referred to is not caused by this PR.

@Westbrook I dont find any visual differences on the first load of PR storybook and PROD storybook
PR: https://bug-overlay-picker--spectrum-web-components.netlify.app/storybook/?path=/story/picker--custom
PROD: https://opensource.adobe.com/spectrum-web-components/storybook/index.html?path=/story/picker--custom

Also I see all pass on the local VRT test on my branch for Picker. Seems like the VRT test is not picking up the hover state on picker items on CI

… into bug/overlay-picker

.circleci/config.yml

Rajdeepc · 2023-12-05T16:13:46Z

@Westbrook Mission Accomplished!!

Westbrook

Love it! Thanks for digging in on this one.

* chore(picker): fix for overlaid picker in mobile * chore: updated golden image cache * chore(picker): fix for overlaid picker in mobile * chore: updated golden image cache * chore(picker): added property for type * chore(picker): added type as a property * chore(picker): removing overlay type as a property * chore(picker): added triiger open decorator for custom picker * chore(picker): reverting golden hash --------- Co-authored-by: Rajdeep Chandra <rajdeepc@adobe.com>

chore(picker): fix for overlaid picker in mobile

c3176f5

Rajdeepc added bug Something isn't working Priority Component: Overlay Component: Picker labels Nov 29, 2023

Rajdeepc requested review from Westbrook and najikahalsema November 29, 2023 16:31

Rajdeepc linked an issue Nov 29, 2023 that may be closed by this pull request

[Bug]: swc-react picker is not working on an overlay in mobile only in Firefly #3834

Closed

1 task

chore: updated golden image cache

015e545

Rajdeep Chandra added 2 commits November 30, 2023 00:02

chore(picker): fix for overlaid picker in mobile

a35e915

chore: updated golden image cache

12f9d85

Westbrook reviewed Nov 30, 2023

View reviewed changes

packages/picker/src/Picker.ts Show resolved Hide resolved

chore(picker): added property for type

639d30d

Rajdeepc force-pushed the bug/overlay-picker branch 2 times, most recently from 03929b6 to 639d30d Compare November 30, 2023 11:08

jnjosh reviewed Nov 30, 2023

View reviewed changes

Rajdeep Chandra added 2 commits December 1, 2023 11:09

Merge branch 'main' into bug/overlay-picker

495e0e9

chore(picker): added type as a property

9bba169

chore(picker): removing overlay type as a property

cfd5c6d

Rajdeepc force-pushed the bug/overlay-picker branch from e1ff6eb to cfd5c6d Compare December 4, 2023 14:18

Rajdeep Chandra added 2 commits December 5, 2023 19:28

Merge branch 'main' of https://github.com/adobe/spectrum-web-components…

19c9532

… into bug/overlay-picker

chore(picker): added triiger open decorator for custom picker

41cef7a

Westbrook reviewed Dec 5, 2023

View reviewed changes

.circleci/config.yml Outdated Show resolved Hide resolved

chore(picker): reverting golden hash

a97530e

Rajdeepc requested review from Westbrook and jnjosh December 5, 2023 16:23

Westbrook approved these changes Dec 5, 2023

View reviewed changes

Westbrook merged commit 4aba1c6 into main Dec 5, 2023

Westbrook deleted the bug/overlay-picker branch December 5, 2023 16:33

chore(picker): fix for overlaid picker in mobile #3835

chore(picker): fix for overlaid picker in mobile #3835

Uh oh!

Conversation

Rajdeepc commented Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issue(s)

Motivation and context

How has this been tested?

Screenshots (if appropriate)

Types of changes

Checklist

Best practices

Uh oh!

github-actions bot commented Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Branch Preview

Uh oh!

github-actions bot commented Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tachometer results

action-menu permalink

picker permalink

split-button permalink

action-menu permalink

picker permalink

split-button permalink

Uh oh!

Uh oh!

Westbrook commented Nov 30, 2023

Uh oh!

Rajdeepc commented Nov 30, 2023

Uh oh!

jnjosh Nov 30, 2023

Choose a reason for hiding this comment

Uh oh!

Westbrook Nov 30, 2023

Choose a reason for hiding this comment

Uh oh!

Rajdeepc Dec 4, 2023

Choose a reason for hiding this comment

Uh oh!

Westbrook commented Nov 30, 2023

Uh oh!

Rajdeepc commented Dec 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Rajdeepc commented Dec 5, 2023

Uh oh!

Westbrook left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Rajdeepc commented Nov 29, 2023 •

edited

Loading

github-actions bot commented Nov 29, 2023 •

edited

Loading

github-actions bot commented Nov 29, 2023 •

edited

Loading

Rajdeepc commented Dec 1, 2023 •

edited

Loading