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.

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

bug: item borders effectively invisible in md dark mode #29386

Closed
3 tasks done
aparajita opened this issue Apr 23, 2024 · 4 comments · Fixed by #29398
Closed
3 tasks done

bug: item borders effectively invisible in md dark mode #29386

aparajita opened this issue Apr 23, 2024 · 4 comments · Fixed by #29398
Assignees
Labels
package: core @ionic/core package type: bug a confirmed bug report

Comments

@aparajita
Copy link

Prerequisites

Ionic Framework Version

v8.x

Current Behavior

Maybe I'm missing something, but when using the Ionic dark mode palette in md mode, the separator lines for an ion-item within ion-list and ion-accordion become effectively invisible in dark mode.

In the case of an ion-item inside an ion-list, the border color is #222222, which is almost no contrast with the item backgound.

In the case of ion-accordion, the border color is set to --ion-color-shade, which is set to var(--ion-color-light-shade, #d7d8da) !important by the .ion-color-light class on the ion-item, which resolves to #1e2023, which is way too dark to be visible against the background.

I'm seeing this on the web and on an Android device.

Screenshot 2024-04-23 at 1 11 37 PM Screenshot 2024-04-23 at 1 11 57 PM

Expected Behavior

I would expect the borders to be lighter than the background with sufficient contrast to be visible to the naked eye, as they were in Ionic 7.

Steps to Reproduce

ion-accordion > ion-item reproduction: https://stackblitz.com/edit/motuzy-w4g1t1?file=src%2Fmain.ts
ion-list > ion-item reproduction: https://stackblitz.com/edit/ryxtvq-ns4z3v?file=package.json

Code Reproduction URL

https://stackblitz.com/edit/ryxtvq-ns4z3v?file=package.json

Ionic Info

From stackblitz:

Ionic:

Ionic CLI : 7.2.0

Utility:

cordova-res : not installed globally
native-run : not installed globally

System:

NodeJS : v18.18.0
npm : 10.2.3
OS : Linux 5.0

Additional Information

No response

@liamdebeasi
Copy link
Contributor

liamdebeasi commented Apr 23, 2024

We likely need to adjust the --ion-border-color value (or similar) for items in lists. I can reproduce the same behavior in Ionic 7: https://stackblitz.com/edit/motuzy-kmn4fz?file=package.json,src%2Fmain.ts,src%2Ftheme%2Fvariables.css,src%2Fenv.d.ts

The accordion has this problem because it has color="light" and we use the shade variant for the border.

@aparajita
Copy link
Author

Thanks for taking a look.

@liamdebeasi
Copy link
Contributor

Here's a dev build that should fix the contrast issue for Items without the color property: 8.0.1-dev.11713911458.1ad5f4cf

I'll need to take a closer look at how best to handle the case where color is set. For most colors in the palette, using the shade for the border is fine since we lighten the base color in dark mode. This is not the case for the light color in dark mode (and similarly, the dark color in light mode) since the base color actually gets darker.

@sean-perkins sean-perkins added package: core @ionic/core package type: bug a confirmed bug report labels Apr 26, 2024
github-merge-queue bot pushed a commit that referenced this issue Apr 29, 2024
Issue number: resolves #29386

---------

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->

The item border does not have sufficient contrast in dark mode making
the border almost invisible.

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- Removed the `--ion-border-color` in the MD dark palette. Instead,
we're using the [theme
default](https://github.com/ionic-team/ionic-framework/blob/dc1172a8411f71227b713a6d7addca91537ea438/core/src/themes/ionic.theme.default.md.scss#L31).
## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!--
  If this introduces a breaking change:
1. Describe the impact and migration path for existing applications
below.
  2. Update the BREAKING.md file with the breaking change.
3. Add "BREAKING CHANGE: [...]" to the commit description when merging.
See
https://github.com/ionic-team/ionic-framework/blob/main/docs/CONTRIBUTING.md#footer
for more information.
-->


## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->


The earliest reference to the offending line Brandy and I could find was
[here](#18713 (comment)).
@liamdebeasi
Copy link
Contributor

Thanks for the issue. This has been resolved via #29398, and a fix will be available in an upcoming release of Ionic Framework. This PR fixes the issue where the wrong border color was used on an item without the color prop in dark mode.

The other issue is a bit tricky to automatically solve for every use case. In some cases we need to use the tint for the border color, and in others we need to use the shade. This decision needs to be based on color contrast which we don't have a great way of automatically determining at runtime. Ideally we'd be able to use color-contrast which would choose the value that yields the higher contrast. However, the browser support for this is limited to Safari 15+ as of writing.

As browser support improves for color-contrast we'll be happy to consider adding support for this automatically. Here is a workaround you can implement in your application to resolve the issue for now:

@media (prefers-color-scheme: dark) {
  ion-item.ion-color-light::part(native) {
    border-color: var(--ion-color-tint);
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: core @ionic/core package type: bug a confirmed bug report
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants