-
chrisfromredfin →
committed b3725372 on 2.0.x authored by
rkoller →
Issue #3488141 by rkoller, chrisfromredfin: Add aria-haspopup="dialog"...
-
chrisfromredfin →
committed b3725372 on 2.0.x authored by
rkoller →
- 🇺🇸United States apmsooner
I'm actually wondering if a custom form element could be created to make it also more flexible. I'm thinking sort of a split button style where a settings subform expands where you could modify some of the properties that would influence performance while browsing. It doesn't have to be saved anywhere although a global token settings form could be helpful to replace what token_tweaks module was doing.
- recursion_limit: This is currently hardcoded to 3 and restricts being able to see anything any deeper. This could be a dropdown ranging from 1 to x(whatever max makes best sense).
- global_types: This could be a checkbox
- entity_types: optionally limit via checkboxes
The subform could trigger an ajax event that updates the browser link itself. I'm doing something similar in a custom form but it would be nice if token module supported this so it could be used anywhere.
- 🇫🇷France just_like_good_vibes PARIS
Let's discuss this for 1.1.x branch.
title is a slot. Do we move it as a string prop?
in that latter case, it can be safely put inside ap
tag. - Issue created by @rkoller
- 🇺🇸United States Kasey_MK
Thanks for your thorough review, @rkoller! For what it's worth, my responses:
In general some work might be needed stylingwise
Agreed! I think I ended up changing styles in my own theme; it didn't occur to me that that might be an issue to solve in focal_point.
in regard of the behavior for screenreader users. first i consider it confusing that the focal point field is placed in different positions depending on if javascript is available or not
It does make sense to have it in the same position. If I recall correctly, I couldn't figure out how to move the field without JS.
i wonder when no javascript is available and the description is hidden why the white cross of the focal point is still shown? wouldnt it make sense to hide it as well?
I think it makes sense to show the crosshairs regardless, since they provide a visual confirmation of the values in the offset field(s) for those who can see it. I like that changing the values using either input automatically updates the other.
So i wonder if it wouldnt be the better choice to make the focal point field visible all the time for everyone in every context
I have no objection to that, personally. If I recall correctly, I was aiming to change as little as possible, but in general I'd think it would be much more accessible and easy to use to just give everyone access to the same tools instead of trying to guess exactly what tools any individual user might need and then anticipate how they'd find them.
In regards of the syntax used on the focal point field, having something like 50,50 along with instructions like "Specify the focus of this image in the form "leftoffset,topoffset" where offsets are in percents. Ex: 25,75 " is sort of challenging. would it be an idea, instead of having one input text field, having two input number fields. that way the keyboard user would be able to either type in the number or use the up and down arrow key changing the value, plus a failsafe could be added only allowing integer values with a minimum value of 1 and a maximum value of 100?
I like that idea, though it's been a long time since I looked at the code and I don't know how difficult that kind of change would be. At the very least, I'd try to make the instructions more human friendly, maybe replacing "leftoffset,topoffset" with "[left offset],[top offset]" or explaining it with a little less tech-speak.
I don't know if I'll get some time to take another crack at this, but if no one else picks it up before I get back here, I'll leave a comment to say I'm giving it another whirl.
- 🇩🇪Germany rkoller Nürnberg, Germany
Sorry for the late reply, your comment slipped through. i haven’t heard of the accesskey concept before. but i have taken a look now at the issue summary as well as the corresponding MDN page. the browser support looks also good: https://caniuse.com/?search=accesskey. from my understanding the accesskeys look like some sort of unification and an at least easier way to add a hotkey functionality to a page/site?
I only see two (and a half) problems with that approach. I’ve tried the example provided on https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/acce... (https://codepen.io/ermarus/full/YzmbxPg) and it has literally no effect at all on macOS (I’ve tried Safari and Firefox). And one problem that struck my eye for macOS, accesskeys uses the same modifier keys (control option) voiceover uses, so your admin toolbar example collides with the hotkey to start announcing the entire page with voiceover active. :/ (Dequeue University provides cheatsheets for the most common screenreaders: https://dequeuniversity.com/screenreaders/). And finally, even though you have given fixed hotkey, the modifier keys differ between operating systems and browsers. If someone has more than one computer and browser they are regularly using the cognitive load is high figuring out the correct set of modifier key for the current context (browser and operating system).Another point to consider, for admin_toolbar you went with the accesskey approach, while the tour module went with adding an eventlistener ✨ Add a hotkey to start a tour Fixed . For one you might end up with two or three different approaches across contrib adding hotkeys to the drupal admin UI, plus it is not sure that the choices contrib modules make for their hotkey don’t collide with of the choices the other modules?
So I wonder if, as a long term plan, the following approach would make sense. In the explorations during the competitive analysis for the a11y track for Drupal CMS, we’ve learned about the approach Joomla takes for hotkeys. You have a keyboard shortcut to open a dialog modal with a cheat sheet that contains the list of available shortcuts for the admin interface.
So our idea (nothing fully fledged out nor planneed) was to have a set of core shortcuts as well as to provide the opportunity for contrib modules to register their own hotkey or even the user to assign custom ones. that way you would have a central place/api for managing hotkeys for the admin ui. That way you would avoid hotkey collissions, you could have/define a consistent modifier key across browsers and operating systems, plus the list of registered hotkeys would be available via a cheatsheet. That would be my preferred long term solution.
For the short term, i would rather lean towards the approach the coffee and the tour modules have taken in regards of the modifier key - coffee uses alt d and tour alt t, aka a consistent single modifier key. cuz implementation wise the addition of the hotkey for the tour module was quick and easy for example via the eventlistener approach: https://git.drupalcode.org/project/tour/-/merge_requests/67.diff or would it be possible to change the modifier keys for the accesskey approach to a consistent one? cuz implementation wise accesskeys are even faster to add it looks like: https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/38.diff
- @rkoller opened merge request.
- 🇺🇸United States smustgrave
Still appears to be missing proposed solution section.
- Issue created by @rkoller
- 🇩🇰Denmark ressa Copenhagen
You're welcome, it's great that it looks good everywhere now :) About finding the problem, Gitlab is great like that -- I just had to find out which CSS file caused the problem (claro/css/components/tables.css) and then look under"History" for that file:
https://git.drupalcode.org/project/drupal/-/commits/11.x/core/themes/cla...
As you can see, the update was just reverted by @nod_, so an alternative solution will have to be found for the problem of big pieces of content pushing down some fields ... We could remove the
vertical-align: middle;
but it's probably safest to keep it in case other admin themes in the future does something similar ...Thanks for fast and great feedback, have a nice day.
- 🇩🇪Germany rkoller Nürnberg, Germany
nice you were able to dig up the issue that was causing the problem! I can confirm the focus outline is not obscured anymore in the first row neither. thank you!
i went ahead and manually commented out the four lines listed in the corresponding core commit https://git.drupalcode.org/project/drupal/-/commit/a11036104f from the issue you've reference ✨ Add vertical-align: top as default for table cells Active . Just wanted to test if it has any negative effect if those changes get temporarily rolled back. The focus outline is still not getting obscured again for the first row. So i think this MR looks good to go. - 🇩🇰Denmark ressa Copenhagen
Sounds great with the larger font, let's see what the maintainers say.
About the overlap. It's because all text strings are pushed to the top in Drupal 11 Claro, including the table headers "Name", "Token", and "Description" ... If you check the patch (click plain diff at the top), it should not affect the table headers.
It turns out that the cause was a recent update to Claro, aligning td's and th's vertically at the top. I didn't see it, because I wasn't using the latest Drupal 11 dev (nor Token, actually)... Lesson learned: Always use dev-version :)
I updated the patch, and it seems fine in Drupal 11 now, but maybe you can check? Thanks!
Automatically closed - issue fixed for 2 weeks with no activity.
- 🇮🇳India chhavi.sharma
@quietone The patch was intented for 10.1.x branch but in the 11.x branch, the test file is only 70 lines long and all the testing functions which were available in the 10.1.x branch are missing from 11.x branch. So the patch changes can not be applied to the test file directly. So should I add all the missing functions and lines of code to the test file which are not included in the 11.x branch?
- 🇮🇳India chhavi.sharma
@quietone I tried to add test file changes but the
UpdateSemvertestBase.php
contains 70 lines only but the suggested changes in the patch are at line 158 so I am not sure of the code in between those lines. That is why I didn't applied those changes manually.
- 🇮🇳India arunkumark Coimbatore
As per comment #39 created a new MR for 11.x version.
- 🇩🇪Germany rkoller Nürnberg, Germany
oh and the link size was bigger? it looked bigger but the last time i've checked the font sizes i'Ve found for the different columns were consistent (no idea why and where i've looked lol). but looks good after your last changes. with the typography slightly bigger it would be also easier to read for everyone. it would be ok from my end to keep it and lets see what maintainers think about it.
In regards of the overlap, i've recorded a short video, on the left is the latest safari 18.1 in the middle the latest version of firefox and on the right the latest version of edge, happens in each of the three browsers for me. and i am on macos sonoma 14.7.1. you are on another operating system perhaps (which might explain the different display)?
- 🇮🇪Ireland lostcarpark
Definitely agree that the search options currently take far too much space.
- 🇨🇦Canada mgifford Ottawa, Ontario
I'd love to see this brought into Core. This adds some missing semantics to our work.
- @arunkumark opened merge request.
- @rajeshreeputra opened merge request.
- First commit to issue fork.
- 🇩🇪Germany rkoller Nürnberg, Germany
In the following an incomplete list of components that might potentially need some work. it has to first checked if there is already an existing issue, if that is case case add this issue as the parent, otherwise create a new issue and add this issue as the parent as well. I add the component and the component in the drupal design syste i would suggest project browser should orient to. i first write the component in question project browser and then the suitaable section in the drupal design system it might orient to, sometimes i will add a comment in parenthesis)
Browse page:
Fieldset wrapping search field and filter section - fieldset/details (there is no component that combines local items with a fieldset)
Filter section - fieldset/details (fieldsets are usually with a plain white background - see the filter section on admin/content for example)
Sort
Sort by - Select (selects have a white background and the selected option while the select list is expanded is highlighted in dark blue, the dark border on hover "might" be broader)
module card - card ( in detail not completely applicable but the module card could at least orient in regard of the card border and padding)
Items per page - Select (The select is unstyled at the moment and is not meeting the minimum target size criterion of 24px by 24px)
Pager - Pager (only two questions shouldnt be there a first and previous link as well if the first page is active? at least it is the case in the design system? and the other question does the pattern exist for the period if there are more pages that numbers visible?)Details page:
not sure if there is a component to add at this point?
- Issue created by @rkoller
- 🇩🇪Germany rkoller Nürnberg, Germany
i am adding this issue as related since it is also about makeing the information more compact and condensed. this issue here is about the card view the linked issue about the list view of the browse page.
- 🇺🇸United States pfrilling Minster, OH
The markup is changing in https://www.drupal.org/project/project_browser/issues/3485747 🐛 The multi-category filter needs to be an actual set of labeled checkboxes Active . This will likely need a rebase once that fix lands.
- 🇺🇸United States chrisfromredfin Portland, Maine
Ha! Leslie has been screaming about this for a while, too! And we didn't have an issue for it. Curious if a lot of this space is coming from us or is default from Claro. I'm hoping it's us so we can change it rather than UNsetting Claro.
- 🇩🇪Germany rkoller Nürnberg, Germany
thank you! i'Ve just realized right now how many issues when testing stemmed from project browser immediately querying the d.o servers after a second or two of not typing. now requiring the press or click of the return key make things way more controllable and also less overwhelming.
- Issue created by @rkoller
- 🇺🇸United States chrisfromredfin Portland, Maine
Reviewed, manually tested, rebased, tests are passing (after a re-test of ye ole flaky testMultiplePlugins)... good to go!
-
chrisfromredfin →
committed 0597c082 on 2.0.x authored by
libbna →
Issue #3464794 by utkarsh_33, libbna, chrisfromredfin, rkoller,...
-
chrisfromredfin →
committed 0597c082 on 2.0.x authored by
libbna →
- 🇺🇸United States emptyvoid
Ok created a patch which fixes the client-side error and properly defines the meta data structure of the variant.
Though I've noticed to fix existing accordions I have to manually rebuild each either in a page or block and save it for the rendering to work as expected.
- 🇺🇸United States pfrilling Minster, OH
The code looks good to me. All the MR feedback looks to have been addressed.
- 🇩🇪Germany jan kellermann
I added a workaround for disabling Klaro! for CKE here: https://www.drupal.org/docs/extending-drupal/contributed-modules/contrib... →
- 🇺🇸United States emptyvoid
As of version 2.1 now I get the following errors in the JavaScript console
accordion.min.js?v=1.x:2 Uncaught Error: Accordion constructor argument domNode has direct descendant elements that do not match with H2-H6 [data-aria-accordion-heading] or DIV [data-aria-accordion-panel] as required. at new o (accordion.min.js?v=1.x:2:7767) at accordion.min.js?v=1.x:2:10828 at NodeList.forEach (<anonymous>) at accordion.min.js?v=1.x:2:10805
It would appear that the rendering of the input is somehow being overriden and the attributes are being stripped from the rendered accordion. The accordion still renders and the user can interact via keyboard and mouse.
But this error is posted to the console on page load.
Anyone familar with this and or how the new code highjacks the input rendering?
Automatically closed - issue fixed for 2 weeks with no activity.
- 🇺🇸United States davedg629
I tried the workarounds proposed in #8 and #26. They worked for some images, but not others and I couldn't figure out why.
I implemented the patch from #27 and it's working for all images I tested..
- 🇩🇰Denmark ressa Copenhagen
Thanks for the review @rkoller, and generally all your great work with Accessibility in Drupal, I very much appreciate it!
About font sizes ... you're right. The link size was bigger than the rest of the text ... I thought about it, and tried using padding instead, to increase the target size. And let's see what the admins thin about increasing the font size from 0.85em to 1em.
About the overlap, I can't replicate it ... it looks like the text is top-aligned in your picture. It looks fine in my Firefox, but maybe you are using another browser?
- @sourabhsisodia_ opened merge request.