benjifisher → credited rkoller → .
hm took the time to install an old drupal 10 site and so being able to install media library media modify 1.x. that way i was able to apply the patch with composer patches. but on the node edit page still the global field value instead of the overridden one is shown. unfortunately the patch isnt fixing the problem outlined in the issue summary. :( i am setting the issue back to needs work.
To add to the discussion. There might be an approach to solve the fundamental problem of how to handle hotkeys, aka keyboard shortcuts in Drupal. Instead of solving that problem just in the context of Experience Builder, why not solve things consistently for core and contrib, in general - that approach would be more sustainable, beneficial, and future proof. Recent discussions in the accessibility track for Drupal CMS lead to
✨
Add an API for hotkeys to Drupal core
Active
. That proposal would provide a solid foundation for hotkey management in core and contrib, which is also the reason why the proposed API probably would not work as a contrib module. People new to Drupal could potentially be unaware that they have to install the module, and it would be rather challenging to establish a consistent, well-adopted pattern for the entire contrib space, plus it would be pretty unusual for core to integrate with a module in contrib.
The other option to mention might help to improve how keyboard users are able to navigate more easily on pages in the Drupal front and backend.
https://www.drupal.org/project/skipto →
integrates https://skipto-landmarks-headings.github.io/page-script-5/ into Drupal. It also exposes the page’s landmark regions and headings to sighted users by making them keyboard accessible (WCAG2.2 SC2.4.1). The SkipTo module is currently a work in progress, with no stable release yet. In interplay with well-thought-out and structured landmark regions, keyboard users would be able to navigate more flawlessly between the main regions of complex pages such as Experience Builder.
Those two suggestions might provide a solid foundation for keyboard usage and could be expanded and interwoven with approaches like the roving tabindex
(
#7 →
) and others, suggested on this issue. Another good resource to mention for inspiration is: https://www.w3.org/WAI/ARIA/apg/practices/keyboard-interface/
rkoller → created an issue.
rkoller → created an issue.
@katannshaw, @mgifford, @the_g_bomb, and myself discussed the matter back and forth for the last couple of days to weeks about how to tackle and, in particular, scope the problem at hand.
In summary we agree with most of the problems raised in this issue, but from our point of view it would be a reasonable step to chop the issue up, create an overarching meta issue, and have this issue rescoped and create a few additional follow up child issues alongside.
In regard to scope, it also has to be noted, during our testing it turned out, it is not only the file field affected, but also the image field, the formatted long text field and the formatted long text with summary field that are missing a required attribute.
In the following we would suggest three general steps how to chop up the problem - at least one of the steps might be subdivided even further.
1. Add the required attribute
We would advise to stay consistent with the field api in Core. Instead of going with aria-required=“true”
for the input elements for file and image fields, as suggested in
#22 →
, better go with required=“required”
that is used on all the other input elements. Problem is for div
elements (formatted long & formatted long with summary) that approach probably won’t work, there the aria-required=“true”
might be required.
In regard to scope it might make sense to rescope this issue and make it solely about adding the required=“required”
attribute to the image and file field and open another issue for adding aria-required=“true”
to the formatted long & formatted long with summary fields.
Next, the following two issues should be postponed until step one is fixed and all field types in Core use a required attribute.
2. Improve the asterisks in the aural interface
In the aural interface for for example VoiceOver the announced text for a field with the label plain text
currently is plain text *, required invalid data, edit text, main
.
The fix could be worked on in the already existing
🐛
Drupal should not use full CSS required marker in forms according to WCAG 2.0
Needs work
. As the proposed resolution, wrap the asterisk in a span
adding aria-hidden=“true”
so it is hidden from screen reader users for all available field types while still visible for sighted users.
3. Improve how required fields are communicated in general
As previously stated, required fields have a few inherent problems - Adam Silver outlined those in: https://adamsilver.io/blog/how-to-highlight-required-and-optional-form-fields/
Our suggestion would be to open another follow up child issue, that will probably require a lot of discussions and testing with actual users, but as a discussion starter the following approach could be suggested.
In node edit forms, based on the number of required and optional fields, the group of fields that is smaller in number should get the (required)
or (optional)
suffix appended to the corresponding field labels. That way it would be clearly communicated what the requirements for that node edit form are - the automatic approach. Another option might be to add a vertical tab to the edit page for a content type and provide the user options to set the to be discussed behavior of the node edit form in regard to required and options fields.
The following video https://www.youtube.com/watch?v=mvIaUHr2i5U covers most aspects of that discussion. It includes the aspects of the article Adam Silver posted, and discusses https://www.nngroup.com/articles/required-fields/ as well as https://www.deque.com/blog/anatomy-of-accessible-forms-required-form-fields/. The arguments in the nngroup article argue for example against 🐛 Forms with required fields marked by asterisk do not have text explaining what the asterisk means Needs work a related issue that was raised during the discussions for this issue - providing some description for the asterisks is considered rather pointless according to the nngroup, since most users don’t read the instructions at the top of a form.
But looking at all those resources shows two things, that the current state of how required fields are handled in Drupal has room for improvement and that there might be no one-for-all solution. Either way the issue for step three will require a lot of discussion and user testing.
the first issue i've noticed is that there are no focus outlines shown in safari. but the css is there. the focus outline is 2px dotted transparent and the focus box shadow then with the claro focus green. (other interface components in safari using the same properties are showing the outline, only the buttons in bpmn are not) . firefox and edge show the focus outline properly
for the rest i have to test some more (and i can also discuss the detail about buttons in tomorrows a11y track meeting for drupal cms as well).
jurgenhaas → credited rkoller → .
I've tested the steps to reproduce against 2.x-dev again. the problem still applies. changing the version to 2.x-dev. in regard to the patch. would it be possible to move it to a MR and does the changes apply to 2.x-dev as well or are there additional changes necessary?
i've restored the removed issue summary. why it got removed? next i will see if the issue still applies, and after that i'll take a look at the patch.
the_g_bomb → credited rkoller → .
the_g_bomb → credited rkoller → .
added one additional aspect/problem and a question i forgot to the issue summary
rkoller → created an issue.
alexpott → credited rkoller → .
benjifisher → credited rkoller → .
oh interesting, thanks for investigating! i am not a developer therefore i dont feel qualified to make a recommendations about the approach how to fix a certain problem :/ the only thing i can say, it at least sounds reasonable staying consistent and choose the same approach claro uses.
argh apologies, missed the conflict line :( was only focus on the version number. thanks for the explanation!
hm the MR is setting dealerdirect/phpcodesniffer-composer-installer
to 1.1.0
, but the actual fix in https://github.com/PHPCSStandards/composer-installer/pull/245 that got merged is targeted for the to be released 1.1.1
?
rkoller → created an issue.
updated the issue summary and added the detail about preventing conflicts with AT keys raised by @mgifford in #2 ✨ Add an API for hotkeys to Drupal core Active
rkoller → created an issue.
thank you! I've updated the issue summary to reflect the ui changes. and as said in the issue summary the contrast of 3.1:1 is meeting the requirement. so looks all good now.
thanks for the MR! i agree #949494 has a high enough contrast, but problem is that this color is not one of the available color variations for claro. the list of available variables is:
/* Gray variations. */
--color-gray: #232429;
--color-gray-900: #393a3f;
--color-gray-800: #55565b;
--color-gray-700: #75767b;
--color-gray-600: #828388;
--color-gray-500: #919297;
--color-gray-400: #adaeb3;
--color-gray-300: #c1c2c7;
--color-gray-200: #d3d4d9;
--color-gray-100: #dedfe4;
--color-gray-050: #f3f4f9;
--color-gray-025: #f9faff;
I've created 🐛 Top-level menu items in the footer region are too far away from their submenu items Active and added it to "other" for now - none of the other categories is a good fit for it. and i've also added ✨ back the ID and aria-labelled-by with JS Postponed
rkoller → created an issue.
Thank you for working on the issue! I've tested MR8, two observations:
1) It looks like the set radio buttons has no effect at all. no matter if i select main-only or full-dom, each time all the headings of the entire dom are displayed in skipto
2) the label for the first radio button (Only within the <main> element
) is broken, main is being hidden and you only see
Only within the
element
probably no need to use angle brackets. will have to think about the wording
that was a good idea! completely overlooked that possibility, i've tested now on 3.x-dev and i ran into the same issue. so the problem is out of the scope of this issue. probably a bug with voiceover and edge?
Thanks for the initial work! I've taken a look at MR7 and i am not sure if using javascript for adding a suffix is the right solution here. i guess the suffix itself could be solveable simply with plain php and the field api. by adding something like
'#field_suffix' => $this->t('percent', [], ['context' => 'skipto']),
to
$form['buttonSettings']['positionLeft'] = [
would solve things (except the wrong label for the number field, but that has to be fixed in
🐛
Improve the microcopy on the settings page
Active
- still havent had the capacity to work on it). imho it is always preferable to go with to write something out instead of using a symbol like %
for screenreader users. but talking of screenreader users, i've tested the solution in MR7 as well as my suggested one and neither is being announced when the numbers input gets into focus. I gotta do some more research if that is maybe a shortcoming in core in general. I'll set the issue back to NW.
This week, instead of discussing a particular issue, we have taken a general look at the skipto module. I wanted to get some feedback about its general direction. After there is no issue to comment on I use the comment here to provide a summary. These are the three issues that got created:
✨
Make the skipTo module configurable on a per user basis
Active
📌
Add a percentage suffix to the position option
Active
📌
Remove main-only option from heading checkbox list and add radio buttons instead
Active
The other detail about making div
and nav
lower case will be covered in the already existing
🐛
Improve the microcopy on the settings page
Active
. The rest of the raised issues will get issues upstream at https://github.com/skipto-landmarks-headings/page-script-5 :
- If the focus is moved from the last available heading to the about skipto.js the focus outline remains on the last heading
- the form of the landmark regions is hard to read due to the front loading of the landmark type and its potentially redundant nature - several landmark regions starting with for example
Navigation:
in a row - the indendation based on the heading level makes the heading list very hard to read - and the heading level is already communicated by the number
benjifisher → credited rkoller → .
we have discussed the issue in yesterdays a11y office hour. The other attendees, @katannshaw and @the_g_bomb, were also uncertain what the expected behavior would be with voiceover activated in regard to the start and end of dialog modals. I have asking over in a11y slack still on my to do list - hope i will get to it over the weekend. but i've remembered https://www.w3.org/WAI/ARIA/apg/patterns/dialog-modal/ and there safari with voiceover behaved the same like in safari.mp4.
i've pulled the latest changes for MR100 (see safari_latest.mp4). you managed somehow that tab and VO-arrow left and right behave the same in regard to reaching the end or the start of the dialog modal! wow! firefox behaves the same now like safari. the only thing, in edge i still have a "VO arrow trap" on the privacy policy link.
I like the consistency inbetween tab and VO arrow but it would be still good to find out what the expected behavior would be for actual screenreader users.
p.s. i completely agree that going with the dialog element would be the cleanest approach, but yep that should ideally happen upstream.
rkoller → created an issue. See original summary → .
updated the user interface changes section in the issue summary (uploaded another screenshot so the before and after are both in a dialog modal.
i not necessarily meant that the icon color should be lighter. i was just thinking out loud if there was some intention behind the lighter color choice for the default icon . but strictly speaking, if some information would be conveyed via a different color that would be against SC1.4.1, therefore a +1 for the consistent colors you've went with from my end. and i've also discussed that question in todays accessibility office hour with @katanshaw and @the_g_bomb and both agreed as well. overall this looks good to go imho. not sure if it would be ok for me to rtbc the issue, since i've opened it. maybe wait for a second opinion, but a +1 for RTBC from my end.
thanks @lostcarpark! that looks good, going with the same color like the rest of the icons is definitely fixing the color contrast issue (it has a contrast of 6.7:1 now). the only thing i wonder, was it the intention by going with a lighter grey for the default icon to differentiate from the bespoke ones so it "stands out" somehow? I have no strong opinion either way as long as the contrast is meeting the minimum requirement.
about the question if the original version should be removed, definitely a valid question, but i dont know. i would assume it would make sense to clean that up if it is not needed anywhere else anymore.
one addition, other dialog modals in core (tested admin/structure/types/manage/playground/fields
) when testing in safari and edge behave the same way like the safari.mp4 example does. firefox is close but also shows some unique odd behavior. when you are back at the beginning of the dialog with VO arrow left you then jump between the close button and the title instead of sticking to the close button. i will research the expected behavior for the voiceover cursor still, but that quick test proofs that something more is off in klaro within the scope of this MR or klaro in general compared to the behavior of drupal core.
thank you! i've tested MR100 on macOS Sequoia. the tabbing problem looks like solved now. but in addition to that i've also tested the stepper in voiceover (ctrl-option arrow left and right) within the dialog modal, but i am not entirely sure what the expected behavior should be. should it be as shown in safari.mp4 (better mute your audio cuz i am skipping through the interface fast which can be very distracting) so that it stops at the last and first element in the AOM or should it behave like with tabbing and jump from the last element to the first (tab) and from the first to the last (shift tab)? i am gonna do some more research (we have the a11y office hour in about an hour, gonna ask others there, and i am gonna ask over in the a11y slack if necessary as well - made me curious).
and there is another detail i've noticed, when testing the same in edge the voiceover stepper gets trapped within the privacy policy link (edge.mp4). as soon as the voiceover cursor is on the privacy link i cant step forward but also am unable to step backward, i am trapped within the link. and finally i've tested the same setup with firefox. at first i've thought it is behaving like safari, first with the tabbing and then with the voiceover stepper. but well, watch the video until the end, and you will notice the stepper is able when getting back to the close button to step past it outside the dialog modal and you then start stepping through the entire background of the dialog - you are actually leaving the AOM (firefox.mp4)
is that problem already covered in ✨ Add alternative link for opening Consent Manager Active ?
Sorry took a little longer than anticipated, but my eye problems have flared up again, making longer text where you have to focus a bit cumbersome. I have taken a look at MR199 and played around with the merge request. I’ve noticed a few things:
- It looks like there is some additional padding between the
Enabled services
heading and its table underneath - that way the heading looks pushed towards theAdd service
button and the gap between the heading and the table is way bigger for theEnabled services
compared to theDisabled services
. It is probably due tomargin-block-start
within the.tabledrag-toggle-weight-wrapper
class selector. I suppose due to the fact that the other pages that are using the enabled/disabled pattern don’t have the option to sort in consequence they also have don't have theShow row weights
link on top of the table, and the div that is containing that link has that margin causing the larger gap. - The drop buttons in the
Operations
column are way bigger than usual - they look like twice the height compared to the drop buttons on for exampleadmin/structure/display-modes/view
,admin/structure/view
, oradmin/content
. - The status checkbox pattern for enabling and disabling services works as expected, but it has a few downsides.
- It is introducing a new pattern for handling the enabling and disabling of services, that is breaking with the existing one users are already familiar with - the other pages are using an
Enable
andDisable
options on the drop buttons in theOperations
column. - With checkboxes in place you have one additional click for saving the configuration, after selecting/checking all the services you want to enable or disable.
- Those status checkboxes make you, the user, think. Depending on what you are trying to change, for disabled services you want to enable you might catching yourself thinking do i select the services to enable by ticking them, while the other way around if you want to disable services and you are still in that “selection mindset” you might leave those services checked you want to disable. Also due to the fact that the position of the service is only changed when the save button is clicked you might have both sections with checked and unchecked services. The cognitive load is high that way. :/
Save configuration
button label and theStatus<code> column header are decoupled semantically from the section headings <code>Enabled services
andDisabled services
.- And finally the width of the columns differs between the two tables.
- It is introducing a new pattern for handling the enabling and disabling of services, that is breaking with the existing one users are already familiar with - the other pages are using an
- If you enable or disable all available services, either way you are getting
No services have been created yet.
. This is sort of inaccurate since Klaro is shipping with 23 services by default.
In the following a few suggestion how to tackle the aforementioned points:
about point 1: If possible I would reduce the margin between the Enabled services
heading the associated table.
about point 2: The other pages using the enable/disable pattern are using the extra small variant size for drop buttons from the Drupal Design System. I would suggest to stay consistent in regard to the styling. →
about point 3: I would suggest to follow the pattern the other pages with the enable/disable pattern are using. Remove the status column and adjust the width of the remaining columns in-between the two tables. On the drop buttons of the Enabled services
section, keep Edit
the default option and add a Disable
option as secondary action. In the Disabled services
section make Enable
the default option on the drop button and move Edit
to the secondary actions. If you enable a disabled service it is directly moved to the enabled services section, same as the other way around.
The only slightly “odd” detail about that pattern is having those handles for changing the order of enabled services by dragging them vertically. My first impulse was testing if it would be possible to disable services by drag and drop. But it was clearly communicated that dragging a service to a different section doesn't work/is not intended AND the disabled services section doesn't have those handles.
about point 4: Uncertain if it might be too granular, but the most accurate approach would be to check if there are no services created yet and then show No services have been created yet.
in the enabled as well as the disabled services section. If there are services created, and none of them is enabled use something like There are no enabled services.
in the enabled services section, if none of them is disabled use something like There are no disabled services.
in the disabled services section.
rkoller → created an issue.
rkoller → created an issue.
I forgot to leave the direct link to the summary of the meeting in a comment here as well, it can be found at #3511972-42: Allow Composer and rsync location to be configured via the UI →
updated the link on the issue we've discussed so it is pointing to this issue instead of the one i've closed down.
updated the parent issue, moving to the other issue for the 6th of june
closing this issue for 📌 Drupal Usability Meeting 2025-06-06 Needs work and i also remove the parent issue to exclude it from the issue chain of ux meetings
benjifisher → credited rkoller → .
Created followup issues in the olivero queue ( 📌 The title of a dialog modal is getting truncated Active ) and the umami queue ( 📌 The title of a dialog modal is getting truncated in Umami Active )
rkoller → created an issue.
I've taken a look at the MR92 on macOS Sequoia with the latest Safari and VoiceOver. At first, both are things i've tried to get into a PR upstream in klaro-js a few weeks or already months back, but I've struggled to get the tooling working properly and then got distracted :(, but i guess in the long run it might still be reasonable to create a PR for those changes upstream. But for now it is already fixing the imminent issue within the context of drupal, therefore a double thumbs up for creating and working on the issue.
about point 1: semantically, it feels odd to have a link element that is styled as a link but with a role of button. I consider it cleaner to use links for what they are made for, to point the user to another page, while buttons are used to let the user do something on the current page. if the element should be a button then i usually prefer to also use an actual button element - except overriding the markup by klaro would complicate things and it would be a jumping through hoops. :/
aside the question if a link or a button element is used in regard to semantics, i wonder what type of button should be used styling wise: a secondary button, a button or an action link? (for context see the drupal design system ) . It also feels odd to have two primary buttons (decline and accept) on the dialog, shouldn't it be only one primary button there?
In regard to the aural interface there is another detail to note, all the three buttons are missing a context, if a person is navigating by for example the rotor and gets only announced the available buttons on a page it is close to impossible to figure out the context for each button, and it should be avoided that a user has to inspect the close proximity of the element to learn about the context (check rotor.mp4 - close your eyes and try to make sense of things without the visual context). but the points about the styling and the labeling are probably out of scope for this issue.
about point 2: There is the problem that the focus isn't contained within the dialog modal, you are able to tab in and out of the modal and into the background of the modal or into the browser window (see tabbing.mp4)
Left a brief comment on the issue #3370326-54: Refine labels and descriptions for field types → and i will push the changes we've discussed to the issue later today.
At 📌 Drupal Usability Meeting 2025-06-13 Active , we continued work on this issue, I will push some of the discussed changes later today. For the record, the attendees at the usability meeting were @benjifisher, @rkoller, and @simohell.
I’ve demo’ed the current state of the MR at the monthly NWDUG meetup last Tuesday. For the record the attendees I am aware of their nickname on d.o were: @asherry, @bernardm28, @dydave, @h2cm, @mr_scumbag, and @philipnorton42. All in all, the first impression was also a good one, an improvement over the current state. They had a few comments on certain field types and groups we’ve taken a look at, in no particular order:
- On the first step the hyphenation used on the descriptions was considered distracting and making it harder to skim
- It was suggested to move the bullet point Recommended for to the top of the bulleted lists. It was argued that what a field type is recommended for is the most important information people are interested in, and they could then still read on in case they want to know more in-depth information.
- In regard to the bullet point
Efficient storage
on for example short plain text it was asked does that imply that the other ones listed are not efficient then?
left a comment on the issue: #3511972-42: Allow Composer and rsync location to be configured via the UI →
and for the record the attendees of the meeting last friday were @simohell, @worldlinemine, and myself
Posted a comment on the composer and rsync issue: #3511972-42: Allow Composer and rsync location to be configured via the UI → ... and it looks like the file name issue still needs a comment.
Usability review
We discussed this issue at 📌 Drupal Usability Meeting 2025-05-30 Active . The recording of the meeting: https://youtu.be/18q8p7xz7VE. For the record, the attendees at the usability meeting were @benjifisher, @rkoller, and @simohell.
We revisited this issue at 📌 Drupal Usability Meeting 2025-06-06 Needs work . That issue will have a link to a recording of the meeting. For the record, the attendees at the second meeting were @rkoller, @simohell, and @worldlinemine.
We have taken a look at the new settings page on admin/config/system/package-manager
, the change to the check for composer on the status report page admin/reports/status
, and simulated a few scenarios by renaming composer and rsync binaries in the DDEV web container, and experimented with overriding those executable path settings via the settings.php file. A few observations:
- Having a status message that is reporting that the paths for
composer
andrsync
were automatically detected each and every time a user is visitingadmin/config/system/package-manager
raises many potential questions and might be confusing to the user. One might ask themselves is it necessary to access this settings page first, before package manager is able to autodetect composer and rsync, until then the two binaries remain unavailable to the site and package manager since the auto-detection was not trigged yet? Furthermore one would expect as soon as the two binaries were autodetected the status message wouldn’t show up ever again as long as the path and/or file name don’t change - why is something auto-detected and announced again and again after it was already detected and the executable path set for that location? Having a status message informing the user about a state change, a newly arisen situation, and/or a successfully completed task is the right purpose, but having a redundant message about a task that was already successfully accomplished feels counter productive and leaves a certain uncertainty about the persistence of that autodetected setting. - It seems the accuracy between the auto-detection on
admin/config/system/package-manager
and the composer test onadmin/reports/status
is divergent. Out of the box with/usr/local/bin/composer
available in the DDEV web containeradmin/config/system/package-manager
reports that composer is automatically detected, andadmin/reports/status
reports it checked successfully for composer returning2.8.9 (/usr/local/bin/composer)
. If you rename/usr/local/bin/composer
to/usr/local/bin/composerer
, reloadingadmin/config/system/package-manager
returns again that the path to composer was automatically detected, whileadmin/reports/status
is throwing an error on reload that composer is not found. If you now change the executable path onadmin/config/system/package-manager
to/usr/local/bin/composerer
and save, the status message again returns that composer was automatically detected, same for reloadingadmin/reports/status
, it reports that it successfully checked for composer returning2.8.9 (/usr/local/bin/composer)
. The only time you are able to trigger an error onadmin/config/system/package-manager
is by changing the executable path to/usr/local/bin/composer
while path and binary are still renamed to/usr/local/bin/composerer
. In consequence, the auto detection onadmin/config/system/package-manager
feels not trustworthy compared to the status check onadmin/reports/status
. - If you change the composer executable path from
/usr/local/bin/composer
to another binary that is available in the same directory within the DDEV web container, like/usr/local/bin/node
, the form is still getting validated successfully. - If you set
$config['package_manager.settings']['executables']['rsync'] = '/usr/bin/rsync’;
in thesettings.php
and change the rsync executable path to/usr/bin/rsyncer
onadmin/config/system/package-manager
the following two warnings show up twice:
Warning: Undefined array key "#parents" in Drupal\Core\Form\FormState->getError() (line 1176 of /var/www/html/repos/drupal/core/lib/Drupal/Core/Form/FormState.php). Drupal\Core\Form\FormState->getError() (Line: 166) Drupal\Core\Form\FormErrorHandler->setElementErrorsFromFormState() (Line: 126) Drupal\Core\Form\FormErrorHandler->setElementErrorsFromFormState() (Line: 126) Drupal\Core\Form\FormErrorHandler->setElementErrorsFromFormState() (Line: 26) Drupal\Core\Form\FormErrorHandler->handleFormErrors() (Line: 200) Drupal\Core\Form\FormValidator->finalizeValidation() (Line: 119) Drupal\Core\Form\FormValidator->validateForm() (Line: 585) Drupal\Core\Form\FormBuilder->processForm() (Line: 321) Drupal\Core\Form\FormBuilder->buildForm() (Line: 73) Drupal\Core\Controller\FormController->getContentResult() call_user_func_array() (Line: 123) Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 622) Drupal\Core\Render\Renderer->executeInRenderContext() (Line: 121) Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext() (Line: 97) Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 183) Symfony\Component\HttpKernel\HttpKernel->handleRaw() (Line: 76) Symfony\Component\HttpKernel\HttpKernel->handle() (Line: 53) Drupal\Core\StackMiddleware\Session->handle() (Line: 48) Drupal\Core\StackMiddleware\KernelPreHandle->handle() (Line: 28) Drupal\Core\StackMiddleware\ContentLength->handle() (Line: 32) Drupal\big_pipe\StackMiddleware\ContentLength->handle() (Line: 116) Drupal\page_cache\StackMiddleware\PageCache->pass() (Line: 90) Drupal\page_cache\StackMiddleware\PageCache->handle() (Line: 48) Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle() (Line: 51) Drupal\Core\StackMiddleware\NegotiationMiddleware->handle() (Line: 53) Drupal\Core\StackMiddleware\AjaxPageState->handle() (Line: 51) Drupal\Core\StackMiddleware\StackedHttpKernel->handle() (Line: 715) Drupal\Core\DrupalKernel->handle() (Line: 19) Warning: foreach() argument must be of type array|object, null given in Drupal\Core\Form\FormState->getError() (line 1176 of /var/www/html/repos/drupal/core/lib/Drupal/Core/Form/FormState.php). Drupal\Core\Form\FormState->getError() (Line: 166) Drupal\Core\Form\FormErrorHandler->setElementErrorsFromFormState() (Line: 126) Drupal\Core\Form\FormErrorHandler->setElementErrorsFromFormState() (Line: 126) Drupal\Core\Form\FormErrorHandler->setElementErrorsFromFormState() (Line: 26) Drupal\Core\Form\FormErrorHandler->handleFormErrors() (Line: 200) Drupal\Core\Form\FormValidator->finalizeValidation() (Line: 119) Drupal\Core\Form\FormValidator->validateForm() (Line: 585) Drupal\Core\Form\FormBuilder->processForm() (Line: 321) Drupal\Core\Form\FormBuilder->buildForm() (Line: 73) Drupal\Core\Controller\FormController->getContentResult() call_user_func_array() (Line: 123) Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 622) Drupal\Core\Render\Renderer->executeInRenderContext() (Line: 121) Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext() (Line: 97) Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 183) Symfony\Component\HttpKernel\HttpKernel->handleRaw() (Line: 76) Symfony\Component\HttpKernel\HttpKernel->handle() (Line: 53) Drupal\Core\StackMiddleware\Session->handle() (Line: 48) Drupal\Core\StackMiddleware\KernelPreHandle->handle() (Line: 28) Drupal\Core\StackMiddleware\ContentLength->handle() (Line: 32) Drupal\big_pipe\StackMiddleware\ContentLength->handle() (Line: 116) Drupal\page_cache\StackMiddleware\PageCache->pass() (Line: 90) Drupal\page_cache\StackMiddleware\PageCache->handle() (Line: 48) Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle() (Line: 51) Drupal\Core\StackMiddleware\NegotiationMiddleware->handle() (Line: 53) Drupal\Core\StackMiddleware\AjaxPageState->handle() (Line: 51) Drupal\Core\StackMiddleware\StackedHttpKernel->handle() (Line: 715) Drupal\Core\DrupalKernel->handle() (Line: 19)
. The warning is also shown if you set the executable path and the settings override to
/usr/bin/rsync
but set a none existing executable path for composer like/usr/local/bin/composerer
. So the warning seems only to show if either of the two executable paths is failing the form validation and one of the two fields is overridden in the settings.php.
The points we came up with:
- We had a clear consensus in both meetings to recommend replacing the status message with a text box (solves point 1). Instead of auto-detecting and creating “noise” every time you visit
admin/config/system/package-manager
, simply display the path the two binaries are available at and auto-detected in a text box. Only return an error message, with the same accuracy as experienced on the reports page (solves point 2), if package manager is unable to auto-detect the binary/binaries. That way the user is always in control and gets the reassurance everything is in place and package manager is fully operational. At the same time it is clearly communicated that it is possible to change/override the executable path(s) - for that the two fields should not use any placeholder text and be empty instead. - In addition to adding the text box, we would also suggest to change the title of the field set underneath from
Executable path
toCustom executable path
to further clarify that you are actually overriding the auto-detected defaults here and/or provide working executable paths in case one or both binaries are not available on the host. - We also had a clear consensus adding the check available for composer on
admin/reports/status
for rsync as well. We’ve already created a follow up issue for that: ✨ Add a runtime requirements test for rsync analogues to composer Active - The warning about the undefined error key in case settings are overridden via the settings.php should be fixed. (solves point 4)
- In the end we wonder if one of the following scenarios we’ve observer and/or discussed have any security implications:
- Adding a different binary, like for example node, for composer and/or rsync (see point 3)
- Adding an executable path to a binary in the sites public or private folder, a location where users are able to upload files.
If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.
the_g_bomb → credited rkoller → .
About the styling, i am not that savvy and familiar with Figma for being able to extract the exact specifics, but in case I interpret the current changes in the MR correctly then:
- the padding between a breadcrumb item and a breadcrumb separator is 0.5rem in the MR while in the styleguide it is set to 0.75rem same as for Claro without the MR
- the separator has the extend of 4px by 4px (width by height) while in the figma file it looks like 3px by 6px..
the increase in separator size got fixed. thanks! oh and the separator is still pointing upwards for RTL languages.
The reason why the pipeline is failing, the linter requires vertical-align
come before border-inline-end
, but vertical-align
is at the end. and looking at https://git.drupalcode.org/project/drupal/-/merge_requests/12046/diffs that also notifies that the file was modified in the source and the target branch - needs someone with write access to resolve that. and i guess the MR could need a rebase in any way, due to the rename of the SandboxManagerBase class in package manager, that is requiring me to uninstall project browser when i try to checkout this MR for testing.
and i am in the process of reviewing the latest changes by @ll66382, will post another comment later.
your latest commit did the trick. tested with the combination of claro and navigation module, gin and the navigation module, and gin, gin_toolbar and the navigation module, all in safari, firefox, and edge on macos sequoia, and in every scenario the toolbar is shown now! thanks!
the_g_bomb → credited rkoller → .
the_g_bomb → credited rkoller → .