Seven theme's details/summary focus style is broken/missing in some browsers.

The details summary:focus is being removed in core/themes/seven/css/base/elements.css line 163.

details summary:focus {
  outline: none;
  text-decoration: underline;
}

Seems like the below code is trying to underline the details summary but it stopped working in some browsers (for me OSX/Chrome).

Here is the original issue for Seven's details summary focus. #2368393: Fix focus effect on summary details

The attached patch removes the details summary:focus. style from the Seven theme and switches back to the default style. I am not sure this is the best solution but it might be the only solution if some browsers no longer support underlining the details summary.

This JSFiddle shows in Chrome and Safari on OSX that the details summary can no longer be underlined.

I feel our issue is related to When did Chrome start underlining links differently? but I can't find any concrete proof.

One workaround if we really want to underline the details summary onfocus is to wrap the summary text in a <span> tag and underline the <span> tag instead of the <summary> tag.

This JSFiddle shows that a span within the summary can be underlined in Chrome & Safari on OSX..

The attached patch is a proof of concept.

Thanks for the patches. I reviewed the code in both #9 and #10 and they are clear and I don't have any suggested changes. I would leave the decision to which approach is best to @andrewmacpherson or someone else on the accessibility team but I will do some testing of both of these and add some screenshots.

I tested both patches and see the blue outline from #9 and the underline from #10. In order to continue testing on other browsers, @andrewmacpherson or someone else on the accessibility team should decide which direction to go in.

This issue should probably be reviewed by the UX team because the simplest solution is to remove the underline and switch back to using the browser's default behavior and styling for focused details summary.

Adding the extra span tag would be permitted in a minor Drupal release but it feels like a hacky solution. Also, the Seven theme does not have a maintainer, so I am not sure who would make this decision.

Finally, we need to consider that the community is actively working on new Administrative UI theme and the Seven theme is going to be deprecated and replaced.

I do agree with Andrew that this is a major issue and should be resolved ASAP. Removing the custom underline style via the patch from #8 could be backported to 8.6.x. The patch from #9 would most likely have to be only applied to 8.7.x+ because changing the HTML markup for the detail widget could cause some regressions/issues, especially if people have extended the Seven theme.

I agree with @jrockowitz on the comments in #13. Definitely a UX/A11Y decision. Thanks for the details.

I was thinking that the UX team could weigh in if it okay to use browser's default styling for the details summary. If we feel adding the <span> tag is an okay solution, then we don't really need to have any UX review.

What is the span for in #10 - to make the selector more specific, or something else? I haven't been able to figure out the cause.

The issue that Chrome/Safari no longer support the underlining style for the <summary> tag but they do allow a <span> tag nested within the <summary> to be underlined.

I am tagging this 'Needs frontend framework manager review' so that we can get some initial input on is #10 an okay workaround, does this issue require a change record, and can the solution be back-ported to 8.6.x.

Thanks for the updates. If you need more testing, I'm happy to help out.

I am at DrupalGovCon Contrib Sprint and teaching someone how to review a patch.

For now, we are going to use simplytest.me.

https://simplytest.me/project/drupal/8.8.x?patch[]=https://www.drupal.or...

Here is a re-roll of the patch.

Here is the updated simplytest.me URL.

http://simplytest.me/project/drupal/8.8.x?patch[]=https://www.drupal.org...

I have tested this patch in Chrome and verified that it works.

I ran head-first into this mess over at #3102724-96: Improve usability of core compatibility ranges on available updates report. I discovered that the <details> on the /admin/modules page has no visual focus indication at all, searched for an existing bug report about that, and found this issue. I just uploaded a patch and a bunch of screenshots to #3102724 that seems to give a working cross-browser implementation of an underline (accomplished via a bottom-border, since text-decoration doesn't seem to work on <summary> on most browsers).

Now that I'm reading all this closely, I'm wishing I hadn't done all that. ;) The <span> inside the <summary> seems *way* better than the hacks I came up with.

I just tested this in combo with my prior patch in #3102724 and it's all working great, at least in Firefox, Chrome and Safari on a Mac.

Patch #22 in here is vastly simpler, easier to understand, and presumably much more reliable than the yucky alternatives I was fiddling with.

Bumping this to RTBC, in the hopes that @lauriii will sign-off on it ASAP and we can use this to solve the focus problems over at #3102724 (which is a critical task that's blocking a slew of other issues from being backported to 8.8.x and that will block the 9.0.0-beta1 release).

Thanks!
-Derek

Just adding that I tested this in IE11 to confirm that the solution does not conflict with with the functionality in collapse.js for browsers that don't natively support details/summary -- everything worked fine.

+++ b/core/themes/seven/css/base/elements.css
@@ -161,9 +161,11 @@ details summary {
 details summary:focus {
-  text-decoration: underline;
...
+details summary:focus span {
+  text-decoration: underline;
+}

This will cause some problems with contrib compatibility because there are contrib projects that are manually creating details summary elements. As a result, focus effect wouldn't be available even on browsers that used to receive it before. This would also make the behavior inconsistent between details elements.

That said, it could be a bigger win to fix this accessibility issue in most instances of details. I would like the accessibility maintainers to weigh in on this.

I checked if all details summary elements in core receive span child element and it seems like we have to apply this change to views-view-table.html.twig as well.

currently working on this .....

Please find the updated file and interdiff file ...... . @lauriii changes has been done according to #27...

@Rithesh BK re: #29 -- sadly, no, that's not the requested change at all. @lauriii said:

it seems like we have to apply this change to views-view-table.html.twig as well.

I'll re-do this, since this issue is blocking #3102724: Improve usability of core compatibility ranges on available updates report which in turn blocks a bunch of other stuff, including all the next releases of core.

I believe like so?

@lauriii: Do we want any comments in core/themes/seven/templates/views-view-table.html.twig about why we're overriding it?

Do we need anything else here to get this done?

Thanks,
-Derek

To address the other concern in #27:

This will cause some problems with contrib compatibility because there are contrib projects that are manually creating details summary elements. As a result, focus effect wouldn't be available even on browsers that used to receive it before. This would also make the behavior inconsistent between details elements.

Couldn't we just do this? Leave text-decoration: underline on details summary:focus? I tried it on Chrome and Firefox and didn't spot any problems. I know it's technically a duplicate style for some browsers, but visually it works fine and that seems less disruptive than the alternative.

Thoughts?

Thanks!
-Derek

Sorry for the noise, but I slightly missed restoring the style, causing a bigger diff than needed. This is better.

+++ b/core/themes/seven/css/base/elements.css
@@ -164,6 +164,9 @@ details summary:focus {
   text-decoration: underline;
...
+details summary:focus span {
+  text-decoration: underline;

This approach is great because this way we can ensure that this doesn't cause regressions with any markup 👍 Only downside of this is potential confusion when reading the CSS. This could be addressed with a comment explaining why we have "duplicated" this rule.

Let's also open a follow-up to explore if we could find a way to solve this which wouldn't require updating contrib markup.

Comments added.

I couldn't help myself and also added a note to the @file comment in core/themes/seven/templates/views-view-table.html.twig ;)

Follow-up: #3115222: Explore if underline focus style for details/summary can work without span

FWIW I don't think this actually blocks #3102724: Improve usability of core compatibility ranges on available updates report, but based on @andrewmacpherson's description the issue is really serious so we should try to fix it regardless of what it's blocking.

We changed the browsers core supports in 8.8, so prior to commit, we should test the browsers in our hardcoded browserlist.

Furthermore, we should check whether Claro has any of the same bugs. Claro's focus styling is quite different so it may not be an issue; however, other theme bugs from Seven did end up in Claro. So would be good to know if Claro is affected prior to fixing Seven.

Re: #37:

- Duly noted on not blocking #3102724
- Agreed on wider browser testing, I only looked at a couple.
- I can definitely confirm that Claro does not have this same bug. The bug is ultimately that not all browsers support text-decoration on details/summary elements. Claro's focus styles for details/summary are entirely different, so the lack of consistent support for text-decoration doesn't matter to Claro.

Cheers,
-Derek

Re: #27:

I checked if all details summary elements in core receive span child element and it seems like we have to apply this change to views-view-table.html.twig as well.

The #type => 'details' render element does more than spit out <details><summary>. Apparently IE11 doesn't support <details> properly (https://www.caniuse.com/#feat=details), so there's a JS polyfill that gets injected. Also some aria-related stuff. So doing this directly in the template means we lose all this goodness, and either this <details> from the views template will not work and be accessible on some browsers, or we have to duplicate all the stuff the render element does for us.

Quoting from andrewmacpherson in Slack:

So long as <details> is being added via Render Element API, then our polyfills get added automatically.

All this makes me wonder if views-view-table.html.twig should be trying to inject <details> directly at all. This is almost certainly out of scope here, but it seems we're making that worse by the changes I made in #31 and later to address @lauriii's concerns.

Should we:

A) Open a follow-up to change how views-view-table.html.twig handles the details so it's done via render element instead of directly in the template?

B) Open a follow-up to change views-view-table.html.twig so that it duplicates all the effort from the render element re: polyfills + Aria?

C) Re-scope this issue to handle all of the above?

D) Other?

Thanks,
-Derek

I would open a follow-up to discuss how to handle the polyfill issue with Views. It's also not a problem specific to Views since there are some other places as well where details instance are being initialized manually http://grep.xnddx.ru/search?text=%3Cdetails&filename= .

What I don't fully understand is how does this change cause a regression in the Views template, but not in the details template?

Re: follow-up: 👍 tagging for now, I'll open it later (about to go to sleep).

Re: "how does this change cause a regression in the Views template, but not in the details template?"

Sorry, I don't think I was clear. "This change" is adding <span> to the details template. There's no regression. It just helps the details template have a working focus style.

What I meant is that the act of working on #31 made me realize that the views template, which handles <details> directly, not via a render array, means the views template is already *not* getting the polyfills and therefore not consistently working on all browsers. Adding the <span> to the views template doesn't "cause a regression" -- it fixes the focus styles. But it made me realize we already have another bug, which is that we're missing the polyfills anywhere we're using <details> without using a render array.

We're not actually making anything "worse". I meant that to get the views template into shape (with a working focus style), we're manually adding a <span>. But maybe we should be converting it to use a render array for that <details>, which would then give us the <span> for free, plus get the polyfills injected properly.

Hope that all makes sense. Sorry for not being more precise in the first place.

Thanks,
-Derek

Created follow-up: #3119279: views-view-table.html.twig template directly uses details without render array and polyfills

Optimistically assuming #3119279: views-view-table.html.twig template directly uses details without render array and polyfills lands soon, we don't need any of the bloat/complication from #31 and this should be all we need to fix this bug.

Thanks @dww

Patch looks good to me, and follow-up issue #3119279 is already RTBC.

I didn't expect any browser incompatibilities with this but tested this with latest versions of Chrome, Firefox, IE 11, Edge, Safari and Opera and the underline was visible consistently on all browsers. 🙌

I adjusted the code style on the template, interdiff attached.

Committed 4569c10 and pushed to 9.1.x, 9.0.x and 8.9.x because this seems like a low risk issue that fixes a major accessibility issue. Thanks!

Maybe I found a regession regarding this issue for the field_group module. Using Seven, the horizontal tabs feature does not render the tab titles anymore, probably because the selector does not apply anymore.

See: https://www.drupal.org/project/field_group/issues/3104205#comment-13607521

Does field_group have to adapt to the additional <span> or does this belong here?

Thanks in advance, Anton