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 all commits
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
5 changes: 4 additions & 1 deletion 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 @@ -59,12 +59,14 @@ governing permissions and limitations under the License.
max-width: 100%;

max-height: inherit;

overflow: hidden;
outline: none;

--spectrum-dialog-padding-x: var(--spectrum-dialog-padding);
--spectrum-dialog-padding-y: var(--spectrum-dialog-padding);
--spectrum-dialog-border-radius: var(--spectrum-border-radius);

border-radius: var(--spectrum-dialog-border-radius);
}

.spectrum-Dialog--small {
Expand Down Expand Up @@ -271,6 +273,7 @@ governing permissions and limitations under the License.
width: 100%;
height: 100%;

border-style: none;
border-radius: 0;
}
/** @unofficial */
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 {
border: 1px solid transparent;
}
}
9 changes: 9 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 @@ -22,6 +22,8 @@ governing permissions and limitations under the License.
/* Distance between top and bottom of dialog and edge of window for fullscreen dialog */
--spectrum-dialog-fullscreen-margin: 40px;
--spectrum-dialog-max-height: calc(var(--spectrum-visual-viewport-height) * 0.9);

--spectrum-dialog-border-size: var(--spectrum-popover-border-size);
}

/* Used to position the modal */
Expand Down Expand Up @@ -73,6 +75,7 @@ governing permissions and limitations under the License.
min-width: var(--spectrum-dialog-min-width);

border-radius: var(--spectrum-dialog-border-radius);
border-width: var(--spectrum-dialog-border-size);
outline: none; /* Firefox shows outline */
pointer-events: auto;

Expand Down Expand Up @@ -143,3 +146,9 @@ 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;
}
}
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 @@ -10,6 +10,12 @@ OF ANY KIND, either express or implied. See the License for the specific languag
governing permissions and limitations under the License.
*/


:root {
--spectrum-dialog-border-color: var(--spectrum-alias-border-color-transparent);
}

.spectrum-Modal {
background: var(--spectrum-dialog-background-color);
}
border-color: var(--spectrum-dialog-border-color);
}
5 changes: 5 additions & 0 deletions 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 @@ -48,12 +48,17 @@ governing permissions and limitations under the License.

outline: none; /* Hide focus outline */
box-sizing: border-box;
overflow: hidden;

&.is-open {
@inherit: %spectrum-overlay--open;
}
}

.spectrum-Popover--withTip {
overflow: visible;
}

.spectrum-Popover-tip {
position: absolute;
--spectrum-popover-tip-size: var(--spectrum-popover-tip-width);
Expand Down
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;
}
}
10 changes: 10 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 @@ -63,6 +63,9 @@
display: flex;
flex-direction: column;

overflow-x: hidden;
overflow-y: auto;

border-radius: var(--spectrum-tray-full-width-border-radius) var(--spectrum-tray-full-width-border-radius) var(--spectrum-tray-border-radius) var(--spectrum-tray-border-radius);

/* Start offset by the animation distance */
Expand Down Expand Up @@ -94,3 +97,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;
}
}
6 changes: 6 additions & 0 deletions packages/@adobe/spectrum-css-temp/vars/express.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,3 +162,9 @@
.express.large {
--spectrum-switch-track-width: 36px;
}

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

Choose a reason for hiding this comment

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

why isn't one enough? what is it conflicting with? the reason I ask is because if it conflicts when there is only 1, then I suspect there exists a case where it will conflict with 3 levels of specificity, but I want to verify that

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Testing the ActionMenu story using the Express theme, the border does not render on the ActionMenu popover without this added specificity.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Additional context, .express.express overwrites --spectrum-popover-border-size to 0px at:

react-spectrum/packages/@adobe/spectrum-css-temp/vars/express.css

Line 151 in 7d4b6d0

--spectrum-popover-border-size: 0px; /* TODO: is this for all popovers or just menu/picker/combobox? */

In order to overwrite this again with forced-colors: active, we need to use the same level of specificity.

--spectrum-popover-border-size: var(--spectrum-alias-border-size-thin);
}
}