In #1252178-81: Add Modernizr to core we had a request to add boxshadow to Modernizr so that the default admin theme could safely replace outline:none with box-shadow. I'm not certain of the accessibility implications, but to avoid using that old issue for constant updates of the library, I've opened a new issue.

Patch incoming.

CommentFileSizeAuthor
#1 1987346-1-modernizr-boxshadow.patch11.17 KBrupl
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rupl’s picture

rupl
he/him
Primary language English
CreditAttribution: rupl commented
Status: Active » Needs review
FileSize
1987346-1-modernizr-boxshadow.patch11.17 KB

This patch adds boxshadow test, and it adds two new internal "utilities" that Modernizr requires in order to test CSS: testAllProps and testProps. They weren't required before because this is the first CSS test we're including in our build of Modernizr.

There are some whitespace changes but no other substantive changes are being introduced. The overall library version remains at 2.6.2, and no existing tests have received upstream updates.

I haven't actually tested this patch because I'm getting some insane PHP error that is totally beyond my comprehension when I load up my test D8 site. I'm pretty confident that this patch is clean, but it should be reviewed by someone who can install HEAD trouble-free.

ry5n’s picture

ry5n CreditAttribution: ry5n commented

I’m not used to scrutinizing Modernizr internals, but I tested this against the latest 8.x HEAD and the appropriate boxshadow class is added.

seutje’s picture

seutje CreditAttribution: seutje commented

I agree we need testProps and testPropsAll in our core build, but I'm not sure we want to invoke this test without a good reason for it.

The use-case outlined in #1252178-81: Add Modernizr to core is only for Seven, so wouldn't it be silly to penalize all those anonymous users for something that's required only in the administrative side of things?

There's also some weird indentation going on in there, but I assume that's from the build tool.

ry5n’s picture

ry5n CreditAttribution: ry5n commented

@seutje Yeah. It makes sense to move this test into Seven.

rupl’s picture

rupl
he/him
Primary language English
CreditAttribution: rupl commented
Issue tags: +JavaScript

agreed about boxshadow. I threw this together before seeing @jessebeach's comment. Will re-roll w/o boxshadow and leave Seven to implement Modernizr.addTest.

yes, build tool is the cause of the whitespace weirdness. better not to fix it for now, and besides we will remove un-minified lib before shipping D8

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

I'd say no to adding the test, even for Seven.

We don't use the prefixed version of box-shadow (required for older android browser versions). And the only other one that don't have support for it is IE8 (and opera mini but it has it's own thing to show focus and all). http://caniuse.com/css-boxshadow

Soooo, there you go.

rupl’s picture

rupl
he/him
Primary language English
CreditAttribution: rupl commented
Status: Needs review » Closed (won't fix)

I can see what you mean, and I also was unsure if replacing outline with box-shadow was accessible. Closed.

ry5n’s picture

ry5n CreditAttribution: ry5n commented

I beg to differ; I think there is an a11y issue here. IE8 doesn’t support box-shadow, but it will respect 'outline: none'. Without a modernizr class, the following CSS (proposed for Seven) will cause IE8 to display no native focus indicator and no custom one either. The border color will change, but for some buttons (blue) this will not be visible enough. If no box-shadow, I want to defer to the browser style.

.boxshadow .button:focus {
  outline: none;
  border-color: #40b6ff;
  -webkit-box-shadow: 0 0 0.5em 0.1em hsla(203, 100%, 60%, 0.7);
  -moz-box-shadow:    0 0 0.5em 0.1em hsla(203, 100%, 60%, 0.7);
  box-shadow:         0 0 0.5em 0.1em hsla(203, 100%, 60%, 0.7);
}
nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

CSS tests are expensive on the computational side of things. It'll be a huge performance impact on the long run.

I'd be fine if the test is in conditional comments (the irony). But not for everyone.

Just restore the outline on IE8, conditional comments are supported in core.

ry5n’s picture

ry5n CreditAttribution: ry5n commented

_nod OK, that works for me, although I’ll probably use conditional classes (add .lt-ie9 conditionally) instead of loading any JS.

ry5n’s picture

ry5n CreditAttribution: ry5n commented
Issue summary: View changes

Updated issue summary to include old issue number.