Problem/Motivation

off-canvas.theme.css has a CSS rule that intends to override jQuery UI's default styling:

.ui-dialog.ui-dialog-off-canvas {
  ...
  border: 0 solid transparent;
  ...
}

However, this specific override is not actually applied because jQuery UI's style rule wins, as can be seen in the "before" screenshot.

Proposed resolution

A naive implementation would be to make the off-canvas rule more specific.

Are there any other options?

Remaining tasks

Discuss, review.

User interface changes

The off canvas dialog won't have the 1px white border anymore.

API changes

Nope.

Data model changes

Nope.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amateescu created an issue. See original summary.

amateescu’s picture

Status: Active » Needs review
FileSize
936 bytes
26.02 KB
26.42 KB

Here's my naive attempt at fixing this problem :)

Before:

After:

I didn't send the patch for testing because it's a css only change.

andrewmacpherson’s picture

Status: Needs review » Needs work
Issue tags: +Accessibility, +high contrast

Having a 1px border is actually helpful. Who says it shouldn't be there? It conveys the extent of the settings tray, and reinforces a sense of "this is a separate area for admin tasks".

Without this, we'd be relying on the background-color alone to convey the extent of the settings tray. But when a Windows high-contrast theme is being used, background colours are removed, so you won't get the "separate area" visual affordance.

If we decide the 1px white border should not be there, it would be a good idea to keep a 1px solid transparent border in place. When a Windows high-contrast theme is in use, this will become a visible 1px border, so the "separate area" will still be conveyed, even though background colours have been stripped out.

Note there are lots of other issues with core themes breaking in Windows high-contrast mode, described in #2894237: Make core themes more robust in Windows High-Contrast mode. For an example of how a 1px transparent boxer helps, see the dilog screenshots in comment 3 of that issue.

amateescu’s picture

Status: Needs work » Needs review
FileSize
1.11 KB

Thanks, @andrewmacpherson, for the very thorough and helpful explanation!

My initial assessment that the border should not be there was based on the existing core style, which had border: 0 solid transparent;, but it wasn't applied because the CSS selector was not specific enough.

However, I fully agree that the intent was most likely to remove 1px white border, and your suggested solution makes perfect sense :)

amateescu’s picture

Title: The off-canvas dialog should not have a 1px white border » The off-canvas dialog should have a 1px transparent border

Retitling the issue based on #3 / #4.

GrandmaGlassesRopeMan’s picture

Version: 8.5.x-dev » 8.6.x-dev
Component: javascript » settings_tray.module
Status: Needs review » Reviewed & tested by the community

Alright. This does change the stable theme, but it's so minor I think that's alright. 👍

lauriii’s picture

andrewmacpherson’s picture

Some screenshots from Windows with the high-contrast black theme in use. No patch, patch 2, and patch 4.

  1. Before patching, IE11 and Firefox have a different appearance - the border is visible in Firefox, but not in IE11. (This is the case whether whether a high-contrast theme is in use or not.)
  2. After patch #2, the border is not visible in Firefox or IE11 in high contrast mode. This is the bad outcome I described in #3
  3. After patch #4, the 1px border is visible in high-contrast mode. This works in IE11, Firefox, and Edge. It conveys the "separate area" like I wanted in #3, and as a bonus it normalizes the appearance between IE and Firefox, which were different before patching.

Chrome and Opera don't adjust to Windows high-contrast themes at all, that's beyond our control.

Here's what we get from patch #4 (IE 11, high-contrast):
Settings tray in IE11 using Windows high contrast theme. A 1px border spearates the settings tray from main content.

I'm agnostic about whether we have the 1px white border in the full-colour space, but if we get rid of it, patch #4 is the best way.

lauriii’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/misc/dialog/off-canvas.theme.css
@@ -4,9 +4,9 @@
-.ui-dialog.ui-dialog-off-canvas {
+.ui-widget.ui-dialog.ui-dialog-off-canvas {
...
-  border: 0 solid transparent;
+  border: 1px solid transparent;

+++ b/core/themes/stable/css/core/dialog/off-canvas.theme.css
@@ -4,9 +4,9 @@
-.ui-dialog.ui-dialog-off-canvas {
+.ui-widget.ui-dialog.ui-dialog-off-canvas {
...
-  border: 0 solid transparent;
+  border: 1px solid transparent;

We have to increase the weight of the selector to be able to override the JQuery defaults. Could we create a separate selector for the border with the higher weight to not increase the weight for all the CSS properties?

amateescu’s picture

Status: Needs work » Needs review
FileSize
1.47 KB
1.57 KB

@lauriii, sure thing! And thanks for reviewing :)

Not sending the patch to the testbot because it's functionally equivalent to the previous one.

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

I guess this can go back to RTBC.

Looks fine to me anyway 😉

  • lauriii committed abac90f on 8.6.x
    Issue #2976287 by amateescu, andrewmacpherson: The off-canvas dialog...
lauriii’s picture

Status: Reviewed & tested by the community » Fixed

Thank you for making the changes @amateescu and @andrewmacpherson for the a11y review! This looks great. ✨

Committed abac90f and pushed to 8.6.x. 🚀Thanks!

andrewmacpherson’s picture

Adding the related accessibility plan I mentioned in #3, so it shows up there.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.