Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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
Comment | File | Size | Author |
---|---|---|---|
#31 | interdiff-15-19.txt | 7.5 KB | yogesh01 |
#25 | c20170527_183736.png | 54.64 KB | droplet |
#20 | 2880703-15.patch | 3 KB | alexpott |
#20 | Screen Shot 2017-05-26 at 18.15.32.png | 87.59 KB | alexpott |
#19 | 2880703-19.patch | 1019 bytes | droplet |
Comments
Comment #2
alexpottWhat's actually worse that the messed up icons is that all the descriptions of the errors, warnings and checks go missing. Weird and funky.
Comment #3
alexpottComment #4
alexpottThis fixes the hidden the messages. Still need to fix the icons.
Comment #5
alexpottPatch attached fixes the padding and the non-js and js pages match. However I think the intent of
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.
Comment #6
Daniel.Moberly CreditAttribution: Daniel.Moberly commentedI 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.
Comment #7
Anonymous (not verified) CreditAttribution: Anonymous commentedWe can save 0 padding for "checked" group before follow-up like:
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.Comment #8
Daniel.Moberly CreditAttribution: Daniel.Moberly commentedSubmitting 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.
Comment #9
Daniel.Moberly CreditAttribution: Daniel.Moberly commentedComment #10
nod_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.
Comment #11
Anonymous (not verified) CreditAttribution: Anonymous commentedThank 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?
Comment #12
nod_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.
Comment #13
alexpott@nod_ the problem is the requirements are hidden by default with no way to open them! No UX team wants that :)
Comment #14
alexpottHowever 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.
Comment #15
droplet CreditAttribution: droplet commented@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.)
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)Comment #16
alexpottHere'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.
Comment #17
alexpott@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.
Comment #18
Daniel.Moberly CreditAttribution: Daniel.Moberly commentedI'm fine with addressing the CSS in a separate issue re: comment #14. Patch looks good to me.
Comment #19
droplet CreditAttribution: droplet commentedIf 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.
Comment #20
alexpott@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.
Comment #21
droplet CreditAttribution: droplet commented@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
Comment #22
nod_Just wanted to comment on:
This is not true at all: http://www.brucelawson.co.uk/2011/javascript-and-screenreaders/
Comment #23
droplet CreditAttribution: droplet commentedSorry, 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?
(Drupal.behaviors.responsiveDetails)
Can we do that?
Comment #24
alexpott@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.
Comment #25
droplet CreditAttribution: droplet commented(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
NOW: Closed
EXPECTED: Opened
3. how to explain:
Does
aria-pressed
indicate HUMAN action (only)? which one indicate the expanded status?Comment #26
droplet CreditAttribution: droplet commentedComment #28
joelpittet@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?
Comment #29
droplet CreditAttribution: droplet commentedmight be right. patch still apply
Comment #30
yogesh01 CreditAttribution: yogesh01 at Google Code-In commentedInterdiff file for patch 2880703-19 and 2889703-15
Comment #31
yogesh01 CreditAttribution: yogesh01 at Google Code-In commentedComment #30: I seem to have uploaded the wrong interdiff file, here is the correct one.
Comment #32
alexpott@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!
Comment #33
larowlan8.4.x will only receive critical backports now (ie same deal as #27 but one version more)
Comment #35
catchCommitted/pushed to 8.6.x and cherry-picked to 8.5.x. Thanks!