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.

Sign up for GitHub

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(#4049): Express theme should render a border on Popovers and Dialogs with forced-colors: active #4050

Merged
merged 10 commits into from
Aug 8, 2023
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
4f27e9d
fix(#4049): Express theme should render a border on Popovers and Dial…
majornista Feb 13, 2023
fab73b1
fix(#4049): popover should have overflow: hidden with forced-colors: …
majornista Feb 15, 2023
83885fb
fix(#4050): correct border on correct element with correct border-radius
majornista Feb 25, 2023
4e2fffd
fix(#4049): fix Popover overflow with max-height: 400px
majornista Feb 25, 2023
547b6f1
fix(#4049): updates per code review
majornista Mar 27, 2023
e035bc5
fix(#4049): use overflow: hidden on popover containing a menu in HCM
majornista Aug 3, 2023
e670de7
Merge branch 'main' into Issue-4049
LFDanLu Aug 7, 2023
57bc561
css fixes for popover scrolling
devongovett Aug 7, 2023
796778f
Merge branch 'main' into Issue-4049
LFDanLu Aug 8, 2023
baf2ea0
Merge branch 'main' into Issue-4049
yihuiliao Aug 8, 2023
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
Prev Previous commit
Next Next commit
fix(#4050): correct border on correct element with correct border-radius
  • Loading branch information
@majornista
majornista committed Aug 1, 2023
commit 83885fb0c7d99e0db22d9c13bbfd8473c1464f04
1 change: 1 addition & 0 deletions packages/@adobe/spectrum-css-temp/components/dialog/index.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -529,6 +529,7 @@ governing permissions and limitations under the License.
}
}
}

@media (forced-colors: active) {
.spectrum-Dialog:not(.spectrum-Dialog--fullscreenTakeover) {
Copy link
Member

Choose a reason for hiding this comment

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

is there a real reason to have this behind forced colors media query? or can this just be general?

Copy link
Collaborator Author

@majornista majornista Mar 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I guess it could be general, but it gets tricky, because we have dialogs that render within popovers, like .spectrum-Popover .spectrum-Dialog.

border-radius: var(--spectrum-dialog-border-radius);
Expand Down
5 changes: 0 additions & 5 deletions packages/@adobe/spectrum-css-temp/components/dialog/skin.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,3 @@ governing permissions and limitations under the License.
color: var(--spectrum-dialog-warning-icon-color);
}
}
@media (forced-colors: active) {
.spectrum-Dialog:not(.spectrum-Dialog--fullscreenTakeover) {
border: var(--spectrum-alias-border-size-thin) solid transparent;
}
}
6 changes: 6 additions & 0 deletions packages/@adobe/spectrum-css-temp/components/dropdown/index.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,3 +170,9 @@ governing permissions and limitations under the License.
flex-direction: row;
}
}

@media (forced-colors: active) {
.spectrum-Dropdown-popover {
overflow: hidden;
}
}
7 changes: 7 additions & 0 deletions packages/@adobe/spectrum-css-temp/components/modal/index.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,3 +143,10 @@ only screen and (max-device-height: 350px) {
transform: none;
}
}

@media (forced-colors: active) {
.spectrum-Modal:not(.spectrum-Modal--fullscreenTakeover) {
Copy link
Member

Choose a reason for hiding this comment

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

i think we can make this generic and give it a border-color variable that is transparent
then override that variable in the forced-colors media query to be the right color

border-style: solid;
border-width: var(--spectrum-alias-border-size-thin);
}
}
8 changes: 7 additions & 1 deletion packages/@adobe/spectrum-css-temp/components/modal/skin.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,10 @@ governing permissions and limitations under the License.

.spectrum-Modal {
background: var(--spectrum-dialog-background-color);
}
}

@media (forced-colors: active) {
.spectrum-Modal:not(.spectrum-Modal--fullscreenTakeover) {
border-color: transparent;
}
}
9 changes: 8 additions & 1 deletion packages/@adobe/spectrum-css-temp/components/popover/index.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,8 +145,15 @@ governing permissions and limitations under the License.
}
}


@media (forced-colors: active) {
.spectrum-Popover {
overflow: hidden;
/* With a popover that contains a menu,
the menu scrolls. Add overflow: hidden,
so the menu's scrollbar doesn't overflow the
the popover's border-radius in the corners. */
&:has(> [class*='spectrum-Menu-popover']) {
Copy link
Member

Choose a reason for hiding this comment

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

has is not supported everywhere yet https://caniuse.com/css-has

also i'd prefer not to do partial classname matches, it defeats the purpose of CSS modules and will break should we ever obfuscate the name by hashing the whole thing. this is not idle speculation, this is something we are about to do

do we need to pass a classname from this file down? or can this be made more general and exist outside of forced-colors and should just be on every popover? i suspect that it can based on my earlier comments

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can't have overflow: hidden globally on the popover, because it would hide the .spectrum-Popover-tip.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

:has() is supported in Chrome and Edge, which matter most for WHCM. I can use :has(> [role="menu"], > [role="listbox"]) instead of the class name.

overflow: hidden;
}
}
}
7 changes: 7 additions & 0 deletions packages/@adobe/spectrum-css-temp/components/popover/skin.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,3 +37,10 @@ governing permissions and limitations under the License.
}
}
}

@media (forced-colors: active) {
.spectrum-Popover {
--spectrum-popover-background-color: Canvas;
--spectrum-popover-border-color: CanvasText;
}
}
9 changes: 9 additions & 0 deletions packages/@adobe/spectrum-css-temp/components/rule/skin.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,12 @@ governing permissions and limitations under the License.
.spectrum-Rule--small {
background-color: var(--spectrum-rule-small-background-color);
}

@media (forced-colors: active) {
.spectrum-Rule {
forced-color-adjust: none;
Copy link
Member

Choose a reason for hiding this comment

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

I don't see this doing anything? at least, not in chrome's emulation of forced colors

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It should render the rule. Without forced-color-adjust: none;, the background-color will be ignored and the rule renders as transparent.

--spectrum-rule-large-background-color: CanvasText;
--spectrum-rule-medium-background-color: CanvasText;
--spectrum-rule-small-background-color: CanvasText;
}
}
7 changes: 7 additions & 0 deletions packages/@adobe/spectrum-css-temp/components/tray/index.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,3 +94,10 @@
border-radius: var(--spectrum-tray-border-radius);
}
}

@media (forced-colors: active) {
.spectrum-Tray {
Copy link
Member

Choose a reason for hiding this comment

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

i think you're missing an overflow: hidden (which should be general, not behind the media query) like the other popovers above?

border-width: var(--spectrum-alias-border-size-thin);
border-style: solid solid none;
}
}
7 changes: 7 additions & 0 deletions packages/@adobe/spectrum-css-temp/components/tray/skin.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,10 @@
.spectrum-Tray {
background-color: var(--spectrum-tray-background-color);
}

@media (forced-colors: active) {
.spectrum-Tray {
--spectrum-tray-background-color: Canvas;
border-color: transparent;
}
}