search

backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 40 forks source link

[A11Y][UX] [DX] Replace the more/less toggle links with "details" HTML elements #5090

Open klonos opened 3 years ago

klonos commented 3 years ago

We currently use these more/less toggles in:

These currently are implemented as 90% similar, yet separate pieces of JS, spread across the files of the respective modules providing these admin pages. We should consider using <details> for these, plus some theme_more_less_toggle() function perhaps. That would make the code reusable, consistent, and allow it to be updated in a single place. We'd also inherit any usability/accessibility improvements of the standard HTML element.

For older browsers, we'd need to use a polyfill. Drupal 8+ core currently uses 2 polyfills for this:

indigoxela commented 3 years ago

I'd vote for the usage of <details>, but it would look slightly different. So I'm not 100% sure, details elements could fully replace all more/less toggle links.

olafgrabienski commented 3 years ago

Reminds me https://github.com/backdrop/backdrop-issues/issues/324.

klonos commented 3 years ago

...but it would look slightly different. So I'm not 100% sure, details elements could fully replace all more/less toggle links.

Here's a PoC PR, which includes #324: https://github.com/backdrop/backdrop/pull/3629

It's rough and needs polishing, but it works as expected and I'd love to get feedback before I work further on it.

There's many things to decide, but most of them belong in #324 - not here. The main things we need to decide here is do we need the arrows that exist for the more/less toggles in the modules listing page, or not (as in the site status report).

PS: if #5085 gets merged, I'll update the PR to convert the more/less toggle in the updates page into a <details> element as well.

indigoxela commented 3 years ago

Here's a PoC PR, which includes #324: backdrop/backdrop#3629

Why combining the core form item and the toggle PR? Doesn't that make it more likely, that someone kills this issue, too? :stuck_out_tongue_winking_eye:

Anyway, I had a quick look in the sandbox - hey, that replacement might actually work! I have to admit, I only checked the status page and needed a moment to realize, that the more/less thingies are now details elements.

klonos commented 3 years ago

Why combining the core form item and the toggle PR?

Because this is a PoC 😉 ...if people agree that this is a nice thing to have, then I'll work on #324 first, and then once that's in I'll return here to remove any bits of code that aren't needed (basically the '#type' => 'details' Form API implementation and the theme_details() implementation + form_process_details() and form_pre_render_details() functions).

...that replacement might actually work! I have to admit, I only checked the status page and needed a moment to realize, that the more/less thingies are now details elements.

Glad you like it 🙂 ...I'm doing some CSS trickery to prevent clicks on the entire details <summary> and only allow the more/less bit (added via data-* attributes, which makes it work without JS, and is also translatable). That makes these <details> elements behave the same way as those more/less links used to 😉

It was fun working on this PR actually!

jenlampton commented 3 years ago

I can't believe how seamless the change is.

The code looks great too, but I'm adding the needs-work label for the polyfil :) I found https://www.npmjs.com/package/details-polyfill which has a MIT license.

indigoxela commented 3 years ago

The code looks great too, but I'm adding the needs-work label for the polyfil :)

FTR: the details element has good support in browsers, except for IE and Opera Mini: https://caniuse.com/details

Here's the link to the polyfill repo on GitHub: https://github.com/rstacruz/details-polyfill

jenlampton commented 3 years ago

Related: https://github.com/backdrop/backdrop-issues/issues/324

stpaultim commented 3 years ago

We are triaging issues today to add the [AllY] tag for an upcoming Open Source Day sprint on accessibility issues. There is a suggestion that this change would bring with it accessibility improvements, so I'm adding the tag here.

klonos commented 3 years ago

I think that this has a good chance of getting in for 1.20. I plan to work on adding the polyfill over the next week/weekend.

klonos commented 3 years ago

I just carefully re-read this thread, and noticed this comment I made back in May:

this is a PoC 😉 ...if people agree that this is a nice thing to have, then I'll work on #324 first, and then once that's in I'll return here to remove any bits of code that aren't needed...

So basically this is blocked by (or depends on) #324.

klonos commented 2 years ago

This is not blocked any more, so I can resume work on it. Hoping to do that soon.

bugfolder commented 2 years ago

Consider also using this for the placeholder examples help text on the Layout configuration page admin/structure/layouts/manage/*/configure. See https://github.com/backdrop/backdrop-issues/issues/5399#issuecomment-1002820944 and preceding discussion.

klonos commented 2 years ago

This is ready for review again. Custom more/less toggles replaced with details element in the following places:

olafgrabienski commented 2 years ago

@klonos Are the arrow symbols new? At first sight, they look a bit prominent to me, especially on the admin/reports/status page which is already a quite noisy.

Another question: the details text on admin/structure/layouts/manage/home/configure is show examples / hide examples, i.e. in lower case. Before, it was upper case; why the change? Lower case looks strange with more than one word, in my opinion.

klonos commented 2 years ago

Are the arrow symbols new?

Right, I forgot about that 🤦🏼 ...so the more/less links in status page were using no icons, but the ones in the modules listing were using custom ones (crafted via CSS). Since we now have a consistent way to create these, and since they will be added in other places as well (layout placeholder examples, system updates etc.), we need to decide what we'd use. Options are:

... show examples / hide examples ...was upper case; why the change? Lower case looks strange with more than one word, in my opinion.

That was not intentional, and I have no preference I think. I've updated the PR to switch it back to title case.

bugfolder commented 2 years ago

Let me start by saying I really like the notion of regularizing how we do these expandable more/less things, and I think there's lots more places that would benefit from using them, so having a regular pattern to follow will be great!

That said, though, I think we're not quite there with this one for all use cases. On the module page (illustrated in the immediately preceding comment), it looks pretty good. On admin/structure/layouts/manage/home/configure, though, it looks like this (arrows highlighted):

details

Years of expanding fieldsets have taught me that when I see a right-pointing triangle at the left of a bit of text, I can click it and expand the text (or just click on the text itself), as, for example, on the Layouts listing page:

details 2

So I don't think we should show that right-pointing triangle if it's not going to be clickable. Or, perhaps better, can we make it clickable?

And then, related to that: that triangle is nicely bold, and visually is much stronger than the thin little up/down arrow at the far right. It feels like since that clickable thing potentially makes a significant change in the page, it warrants being a little beefier. Even the original up/down triangles were a bit small. Current PR:

triangles

Previous:

triangles 2

Another nice thing about the previous version is that the same isoceles triangle is used for both fieldsets and more/less toggles and dropbuttons. It would be nice to stay consistent with details elements and use the same shape triangle everywhere there is an expanding chunk of stuff:

3 triangles

klonos commented 2 years ago

So I don't think we should show that right-pointing triangle if it's not going to be clickable.

That default arrow from the <details> element is not meant to be shown - thats what the CSS I've added is meant to be doing, but I'm assuming that this must be a browser compatibility thing. In Chrome/Brave, it looks like this for me (no arrow on the left):

image

What browser are you testing on @bugfolder ? ...have you cleared caches?

...that triangle is nicely bold, and visually is much stronger than the thin little up/down arrow at the far right. ... it warrants being a little beefier.

I had it like so before: image

...but then I changed it because of the feedback provided by @olafgrabienski ...we need to decide on an icon that works well for all 😅 ...that's why I said:

Please play around with alternatives, and let me know which would be best to use.

klonos commented 2 years ago

Another nice thing about the previous version is that the same isoceles triangle is used for both fieldsets and more/less toggles and dropbuttons. It would be nice to stay consistent with details elements and use the same shape triangle everywhere there is an expanding chunk of stuff:

Right. I guess we might go back to that triangle I don't like 😅 (I wish there was a unicode character for it)

klonos commented 2 years ago

I wish there was a unicode character for it

There is!:

image

bugfolder commented 2 years ago

What browser are you testing on @bugfolder ? ...have you cleared caches?

Safari on Mac, and yes, cache is cleared (also, I turned off CSS aggregation on the sandbox so I could see where stuff was coming from).

Here's the HTML details from Safari:

From Clipboard

Looks like that triangle is the "-webkit-details-marker".

klonos commented 2 years ago

Safari on Mac, and yes, cache is cleared...

Thanks 👍🏼 ...I'm on a Mac too, but not tested on Safari. Sorry about that. I'll do some cross-browser testing and update the PR accordingly.

klonos commented 2 years ago

...in the past, when we needed to make a decision on icons (like for the admin bar etc.), it helped creating various PRs with different options that people could test. Then we'd post screenshots in separate comments on the issue, and people would vote with 👍🏼 or 👎🏼 ...would that help, or do people find multiple PRs annoying?

bugfolder commented 2 years ago

I'm happy to respond to either, but for just choosing the icon, posting screenshots of the various contenders would work for me.

klonos commented 2 years ago

Default <details> arrow removed now also on Safari. Fixed with backdrop/backdrop@9e649b6 (#3629)

klonos commented 2 years ago

PR updated once again:

There's still one minor annoyance remaining in RTL with how the toggle arrow needs to be to the left of the toggle text ...but I need a break.

Feedback is still welcomed, so please test things in the PR sandbox and tell me what you think.

klonos commented 2 years ago

PR now rebased to latest 1.x, to include the changes in the freshly-released v1.21.0 of core 😉

indigoxela commented 2 years ago

Cool, progress! :tada:

@klonos one question: do we really need a polyfill for IE8?

'browsers' => array('IE' => 'lte IE 8', '!IE' => FALSE),

THB in my own projects I'm beginning to forget about IE11, for sure I don't waste any effort on IE8 and older. :grin:

klonos commented 2 years ago

@indigoxela I've added that back in May, after this was suggested by @jenlampton (see https://github.com/backdrop/backdrop-issues/issues/5090#issuecomment-845480075). Happy to remove 👍🏼

I personally also think that IE8 is "dead". Most of the world has moved to using modern browsers, so this makes sense (and is in accordance with our principles re 80% etc.). Having said that, we have #214 to make an official decision, and last comments I see there have the following sentiment:

@klonos

I know that we've decided that this is a 2.x task, but perhaps we should reconsider(?)

@jenlampton

I think we can safely add new things without IE support now ...

So we just need to make it official, and please say the word, and I'll drop it 😉

bugfolder commented 2 years ago

Testing in Safari on Mac. There's a couple inconsistencies in visual language that would be nice to address.

On admin/modules/list, the disclosure triangle for fieldset is isoceles right triangle, while for details it's equilateral. Any reason we couldn't/shouldn't use the same triangle for both? And to a user, the disclosure arrow is sometimes on the left of its text, sometimes on the right (as in this example). Should we stick to the left for all?

test 1

Clicking on the "more >" adds a gray line between the toggle and the newly revealed text (presumably as a result of focus). It goes away if I click somewhere else. I don't think it should appear at all.

test 2

On admin/structure/layouts, the "How layouts work" looks exactly like a fieldset, but it has an equilateral triangle disclosure element. And it gets a gray outline box when toggled, unlike fieldsets. test 3

And on admin/structure/layouts/manage/home/configure, there's also the gray underline that appears upon clicking the toggle:

test 4

olafgrabienski commented 2 years ago

The gray line and the outline box seem to be browser-specific. I see them (blue, not gray) with Safari 15.2 on a Mac, but not with Chrome.

klonos commented 2 years ago

Thanks, I saw that line, but then realized that it is actually caused by the focus on the element. If you click elsewhere on the page, it goes away. If you start pressing the tab on the keyboard, you eventually get it back. I'll see if it can be removed via CSS, but even then, I'm not sure that we should do it (a11y) 🤔

The same border exists by default on <details> elements on Safari, without any custom CSS styling. You can test that here for example: https://www.w3schools.com/tags/tryit.asp?filename=tryhtml5_details

image

My assumption at the moment is that my custom CSS (suppressing the default marker/arrow specifically) is causing that blue focus indicator border to be "squashed" into a flat line.

indigoxela commented 2 years ago

I'm not sure that we should do it (a11y) ... The same border exists by default on <details> elements on Safari ...

IMO we should not always remove it. We had a similar discussion when adding the details FAPI element (if I recall correctly), and we decided to not hide it for keyboard navigating users.

details summary:focus:not(:focus-visible) {
  outline: none;
}

(That's in system.theme.css.)

klonos commented 2 years ago

Nice! 👍🏼 ...thanks for that @indigoxela 🙏🏼

bugfolder commented 2 years ago

So the conclusion is that we will live with the blue/gray underline, squashed into a flat line between the toggle words and the toggled stuff? This is a really useful idiom, so I suspect it will get used a lot (e.g., https://github.com/backdrop/backdrop-issues/issues/4839).

klonos commented 2 years ago

We might be able to get rid of it @bugfolder ...I'm sure that there is a CSS trick out there - just didn't have the time to research/test further.

bugfolder commented 2 years ago

Is is possible to put a class on the <summary> element only when the toggle is used inline at the end of a line? Then it could be removed via CSS in those circumstances where it would be squashed to a line, but allowed to show when it's mimicing a fieldset?

bugfolder commented 2 years ago

How about basing the outline on whether there's a custom more/less toggle? Suggested alteration in the PR for form processing and CSS.

klonos commented 2 years ago

How about basing the outline on whether there's a custom more/less toggle? ...

Great minds... 😉 ...still think that that outline is annoying in general, so my preference would be to fix it globally.

...Suggested alteration in the PR for form processing and CSS.

Thank you 🙏🏼 ...I'll have a look soon. If I don't manage to come up with a better (global) solution, then I think we'll go with something like that, so that we are not blocking this improvement on trivialities (a minor quirk that affects a browser used by less than 20% worldwide).

bugfolder commented 2 years ago

Thank you 🙏🏼 ...I'll have a look soon. If I don't manage to come up with a better (global) solution, then I think we'll go with something like that...

@klonos, have you had a chance to have a look? Would love to see this in the next release (and other issues could make use of it).

jenlampton commented 2 years ago

@klonos I saw you added the label "design" but I didn't see anywhere that there was a question specifically for a designer. IIRC The end result is for there to be no visual change. If that's not correct and we do need designer feedback, please add the label back and note here what's needed :)

yorkshire-pudding commented 2 years ago

@klonos - as you asked for feedback about standards, here is the design pattern for UK government. https://design-system.service.gov.uk/components/details/

bugfolder commented 1 year ago

@klonos, would love to see this get into core.

klonos commented 1 year ago

Thanks everyone for being patient with this while I am being distracted by other tasks.

@jenlampton re design changes, I think that we should try to keep them to a minimum, however today I noticed that there is an issue with the "more"/"less" text being indistinguishable from the requirement title in the status page when the title is a link: image

So maybe in certain cases the link needs to be styled as a button instead (quick mockup): image

klonos commented 1 year ago

Hey @bugfolder 👋🏼 ...I will (finally) start working on this again - sorry for the long delays.

Can I please get some feedback re the last comment here?

PS: re-adding the "design" label.

bugfolder commented 1 year ago

I will (finally) start working on this again

🙏😁🎉

maybe in certain cases the link needs to be styled as a button instead

What about putting an expansion triangle before the words more/less, the way it is in a fieldset or '#type' => 'details' element? So something like this:

Database tables need conversion ▲ more

Database tables need conversion ▼ less

olafgrabienski commented 1 year ago

So something like this:

[Database tables need conversion] [▲ more]

[Database tables need conversion][▼ less]

Yes, it would be nice to get this back! Btw, as far as I know in the meantime a "more" link has usually a right pointing triangle.

avpaderno commented 1 year ago

I find the up and down arrow not clear, in this context. At least, they do not make clearer what more and less are. Styling more and less as buttons as shown in this screenshot would be better.

screenshot

klonos commented 1 year ago

Thanks everyone for the feedback 🙏🏼 🙏🏼 🙏🏼

I was looking to find UI examples of more/less toggles out in the wild, in an effort to see if there is a current convention or standard, but there's not much. I was looking for sleek, simplistic implementations.

So, when it comes to styles and icons, I have found the following couple of implementations that appealed to me (it was obvious straight away what the toggle would do when clicked, which is the UX we'd ideally want to have):

I should note that while in these implementations the toggle triggering element is placed after the text that is being expanded/collapsed, we are looking to mimic things here by styling the <details> element. I don't think that it'd be easy to achieve this result in our case, but I'm willing to give it a go. Plus another thing: placing the toggle under/after the text would push the content that follows downwards, which would cause pages with many more/less toggles to be rather lengthier then now. Not necessarily a show-stopper, however we'd need to consider it (more scrolling = UX 👎🏼 ).

Other options that I'm considering to try:

Since these are a few different ideas, I might file separate PRs, so that we can have separate sandboxes to play with and decide on what works best. Unfortunately though, our PR sandboxes are currently broken (upstream issue as it seems - see #6139), so I may just post screenshots in the meantime.

yorkshire-pudding commented 1 year ago

@klonos - this is from the UK Government Digital Service Design Patterns https://design-system.service.gov.uk/components/details/

chrome_r2aDEKvHZi

Each pattern has been arrived out by extensive user testing though they do identify some possible issues