Follow-up to #665790: Redesign the status report page

Problem/Motivation

Disable Javascript on admin/reports/status and you see:

The icons are messed up but also all the description and other important information is hidden from the user. We hide the button depending on browser width and this results in things being hidden and the user not able to open when javascript is disabled - see http://recordit.co/yfoBYqBOeX

Proposed resolution

Fix twig templates and CSS so non-js and js versions look the same.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Priority: Normal » Major

What's actually worse that the messed up icons is that all the descriptions of the errors, warnings and checks go missing. Weird and funky.

alexpott’s picture

Issue summary: View changes
alexpott’s picture

Status: Active » Needs review
FileSize
2.11 KB

This fixes the hidden the messages. Still need to fix the icons.

alexpott’s picture

Issue summary: View changes
FileSize
524 bytes
2.62 KB

Patch attached fixes the padding and the non-js and js pages match. However I think the intent of

html:not(.details) .system-status-report__status-title {
  padding-left: 0;
}

Was to make the "checked" group have no padding because they have no icons. Maybe we want to fix that here. It could be follow-up I guess because the page looking the same for js and non-js is more important and a bug rather than a visual decision.

Daniel.Moberly’s picture

Status: Needs review » Reviewed & tested by the community

I came to the same conclusion as #5 and the patch fixes the issue properly. We should create a child issue I think to address the visual change. Agreed that its not as important as the bug fix.

Anonymous’s picture

We can save 0 padding for "checked" group before follow-up like:

-html:not(.details) .system-status-report__status-title {
+html:not(.details) .system-status-report__status-title:not(.system-status-report__status-icon) {

Also +1 to 'open' details by default for all width of devices (content first!), but maybe we can also change Drupal.behaviors.responsiveDetails because its logic will become obsolete with the current changes.

Daniel.Moberly’s picture

Submitting new patch with vaplas's suggestion on how to preserve the css rule for the "checked" group. This solves the core issue without affecting the layout.

Daniel.Moberly’s picture

Status: Reviewed & tested by the community » Needs review
nod_’s picture

Which browser? everything but IE and Edge support details/summary so content is accessible even if js is disabled.

Opening by default should be run through UX team too.

Anonymous’s picture

Thank you, @Daniel.Moberly. #8 patch looks good! Also I made a mistake about the Drupal.behaviors.responsiveDetails, forget it, please :)

#10: Chrome 58.0.3029.110 hide summary by default without 'open' attribute. Or did I not understand the question?

nod_’s picture

sorry question was for alexpott in #1.

As far as I understand the fact that OK requirements are hidden by default on that page is a feature, hence the need for checking in with UX folks. Could be a misunderstanding on my part, didn't actually check the patch locally. No issue about the fact that the open attribute show the details.

alexpott’s picture

@nod_ the problem is the requirements are hidden by default with no way to open them! No UX team wants that :)

alexpott’s picture

Issue summary: View changes
FileSize
2.62 KB

However even with the patch we still have problem because of screen re-sizing and hiding things. Here's a video of the problem we have because we're hiding the close and open button arrow - http://recordit.co/yfoBYqBOeX

I don't think we should do #8. This makes the JS and non-JS pages look different and the indent is a visual clue that this is the checked list. If we want to set the padding to 0 for things without icons we should open another issue and discuss it there. Therefore re-uploading #5.

droplet’s picture

Status: Needs review » Needs work

@nod_ has his point. I almost missed also

1. We designed responsive for mobile.
2. Details TAG is well-supported in all mobile browser.

As fallback:
(OPEN attr is meaningless when DETAILS is not supported natively.)

html.no-details:not(.js) .our-detail-fallback-class {
 // forced open
}

Sidenote:

In fact, IMO this responsiveDetails should only limit to the status report page. We have no well tests on other pages. (It was rushed to commit because of over 6xx comments or so. The CSS still have problems as I pointed out on that issue)

alexpott’s picture

Status: Needs work » Needs review
FileSize
619 bytes
3 KB

Here's the simplest fix for the problem shown in http://recordit.co/yfoBYqBOeX

@nod_ re being open by default - remember this only occurs when javascript is disabled which seems acceptable to me.

alexpott’s picture

@droplet same point I'm making to @nod_ - the details are closed when js is enabled. If we're saying we want non-js mobile experience to be the same then we need to fix the problem shown in http://recordit.co/yfoBYqBOeX differently.

Daniel.Moberly’s picture

Status: Needs review » Reviewed & tested by the community

I'm fine with addressing the CSS in a separate issue re: comment #14. Patch looks good to me.

droplet’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
1019 bytes

If we only want to address icons problem, please remove all other changes. If not, back to Needs work.

I attached a patch. This is fixed all @alexpott problems. But it's still Needs work. The Details Pattern used in Status Page is wrong. This never works on No Native Supported Browsers (eg. EDGE / IE / Older FF we still supported in D8.3)

The Details in CORE has a pattern to follow: #2792593: Details HTML element does not work without .details-wrapper

I'd re-title to "Fix Status Page Details Tag"..etc. I leave @alexpott to make the decision since he filed this report.

Thanks.

alexpott’s picture

@droplet no this does not fix the problems. See screenshot.

No handles to open the details and no text. This is a major bug for screenreaders. Please before posting new patches on the issue at least test the patch as the video http://recordit.co/yfoBYqBOeX shows.

With the patch attached (which is the same as in #16 I think we get all the desired behaviours. Please test it. The only people who are affected by the details being displayed are narrow width without JavaScript enabled. Non JS should start with it open because non-js are things like screenreaders which need the text regardless of screen width.

In order to test what is going on here you have to test both with and without JavaScript.

droplet’s picture

Status: Needs review » Reviewed & tested by the community

@alexpott,

Sorry. I guess I didn't clean the Twig cache (or rebuild JS / turn off JS) on some testings. I messed up my env.

OK. We can fix/tidy up other problems on followup

nod_’s picture

Just wanted to comment on:

because non-js are things like screenreaders

This is not true at all: http://www.brucelawson.co.uk/2011/javascript-and-screenreaders/

droplet’s picture

Status: Reviewed & tested by the community » Needs review

Sorry, one sec. I'm confused.

When to use aria-expanded, aria-pressed, details[open]

If we default to open. All these 3 values should be TRUE?

          // If user explicitly opened one, leave it alone.
          var $notPressed = $details
            .find('> summary[aria-pressed!=true]')
            .attr('aria-expanded', false);
          $notPressed
            .parent('details')
            .attr('open', false);

(Drupal.behaviors.responsiveDetails)

Can we do that?

alexpott’s picture

@droplet setting those in the template breaks the desired behaviour because then when you are on a narrow screen with javascript the text does not start hidden. Also setting "aria-pressed" on something where there is no button seems odd as the button is hidden when the screen is wider than 48em.

@nod_ good point about js and screenreaders. Still though making the description test hidden when js is disabled is undesirable.

droplet’s picture

FileSize
54.64 KB

(SKIP the IMAGE.)

@alexpott,

aria-* is not theme helper class.

1. Can we omit aria-* when DETAILS has [open]? (also checkout: admin/config/development/performance)
2. what if the following code defined on Twig and start with mobile size, with JS also

aria-expanded="true" aria-pressed="false"

NOW: Closed
EXPECTED: Opened

3. how to explain:

aria-expanded="true" aria-pressed="true"
aria-expanded="true" aria-pressed="false"

Does aria-pressed indicate HUMAN action (only)? which one indicate the expanded status?

droplet’s picture

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joelpittet’s picture

@droplet, is the issue you are trying to describe a separate issue from the CSS and HTML changes @alexpott made in this? AKA can this be a separate follow-up issue to address?

droplet’s picture

Status: Needs review » Reviewed & tested by the community

might be right. patch still apply

yogesh01’s picture

FileSize
0 bytes

Interdiff file for patch 2880703-19 and 2889703-15

yogesh01’s picture

FileSize
7.5 KB

Comment #30: I seem to have uploaded the wrong interdiff file, here is the correct one.

alexpott’s picture

@yogesh01 please read issue comments before posting an interdiff. #20 was a repost of #16. As per #28 @droplet's patch in #19 and concerns in #25 are actually a separate issue. So posting an interdiff of #16 to #19 is confusing. Thanks for trying to help though!

larowlan’s picture

Version: 8.4.x-dev » 8.5.x-dev

8.4.x will only receive critical backports now (ie same deal as #27 but one version more)

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

catch’s picture

Version: 8.6.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.6.x and cherry-picked to 8.5.x. Thanks!

  • catch committed 399e0b8 on 8.6.x
    Issue #2880703 by alexpott, droplet, Daniel.Moberly, yogesh01: Status...

  • catch committed 9d5a928 on 8.5.x
    Issue #2880703 by alexpott, droplet, Daniel.Moberly, yogesh01: Status...

Status: Fixed » Closed (fixed)

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