benjifisher → credited rkoller → .
Thanks a lot for all the work! I went through everything and tried to review as much as i was able to, a few comments and questions.
- most of the changes look like those to satisfy the linter by adjusting indentation and or adding commas at the end of arrays.
- explicitly setting array() isnt necessary? thought it would be, that was the reason i'Ve added thoses type. but is it only necessary for functions like
public function buildForm(array $form, FormStateInterface $form_state): array {
but unncessary and or wrong within functions in the cases you've corrected?
'#options' => array(
- a good call renaming generate_skipto_config_json to skipto_generate_config_json. looks more consistent
- and also good call to wrap the landmark labels into t() functions. i am using my OS with english due to the fact that using voiceover in german is unbearable and i was under the impression that those landmark labels would use the english labels as well in consequence ^^. but i'Ve rechecked right now and they are localized. we have to check if there are official translations for those landmarks by the W3C cuz i wouldnt necessarily use landmarks used by a certain screen reader solution in a certain language. meaning, translations might differ inbetween screen readers in the worst case?
- the only thing i am uncertain and unable to check are the use statements you have removed in a view files. is there some automated test to check which are obsolete? or is that a manual task?
so aside the use statements the MR would be RTBC from my end. the use statements i am unable to assess so it is up to you ^^ but guess it should be fine (manually tested aside running phpstan).
benjifisher → credited rkoller → .
the_g_bomb → credited rkoller → .
benjifisher → credited rkoller → .
the_g_bomb → credited rkoller → .
@grienauer noted that aside removing the css asset from the libraries.yml, it "might be" a good idea to also remove the css assets in the first place as well. ^^ completely forgot about that detail.
i've assigned the issue to myself and already created a MR.. Will work on an initial draft the next few days...
Turns out the skipto.css file was unnecessary to add and that additional css was causing all those issue. the three issues listed in the issue summary as well as the problems with the plus suite are solved with MR3. i was only unable to reproduce the problem with the status messages so far but odds are it might be solved as well.
I have to add I also noticed two more issues, but those were only one time and i was unable to reproduce so far, but i'll add them in the context of this issue cuz the "might" be at least related as well:
- Installing the plus suite with layout builder plus, navigation plus, and edit plus alongside skipto made the topbar not showing up and the edit button in the navigation sidebar was shown neither. after several installs and uninstalls both coexisted but it felt flaky.
- the skipto button and the expanded menu collided somehow with status messages and the display of the two in combination was off.
The MR needs a rebase after ✨ Use modal in add new field flow Active went in
benjifisher → credited rkoller → .
I've pushed the remaining changes we've discussed over the course of the last few weeks. The are still strings on the google sheet that still need to go in:
- I was not sure how to add a help text to the second step for selection lists. the proposed microcopy for the help text is:
Each list item has a Name, with limited formatting, and a Value. Values that are in use cannot be changed, since they are stored in the database. The Names can be edited later. - it has to be kept in mind that this issue needs additional work after ✨ Use modal in add new field flow Active goes in, cuz at the moment the h1 on the page isnt changed and adjusted but it has to (and we also prepared suggestion for what could/should be used in the title of the dialog modal.
A few more details about the current state i've noticed applying the changes and going through the interface again:
- for some field groups there is the problem on the second step that the order of options is alphabetical but it would be better to order them in a different manner. for example for
plain textthe labels weretext(plain)andtext(plain, long), so short was first and the more complex second. with the new labelsshort textandlong text,long textcomes first. - for the long text option for formatted text it might make sense to make a characteristic for text areas more explicit.
short textstates that text fields are oneliners whilelong textjust says it uses text areas for input instead. it might be reasonable to clearly communicated that text areas are multi-line? - I've removed the periods at the end of each bullet point on the reference field options. we havent used periods on the bullet points for any other fields.
- would it be possible to add a bullet point to the
otherreference field? it looks odd without any. - on date and time field types it is not entirely clear what the examples refer to. are those the format date and time has to be entered or is it the example in what way dates and time are stored in the database - it might be misinterpreted.
thank you! that is definitely beyond my skill level to figure out on my own :D will add the rest of the strings tomorrow. need to get some sleep first.
not sure if the issue can be closed. according to #3467181-15: Navigation module button icons are obscured when in Windows High Contrast → the introduction of the icon api in 📌 Navigation leverage icon core API Needs work solved the problem, so it will be fixed with 11.2. but problem is, would there still need to be a fix for 10.x, since gin is still supporting it, and it doesnt look like that the icon icon api will be backported?
benjifisher → credited rkoller → .
replaced the link to 🐛 Navigation block titles have a too low color contrast Active , since it got closed by 🐛 Increase the color contrast of the navigation block title Active
?
if gin is using the exact same shade of grey as claro then grey 800 would be --admin-toolbar-color-gray-800: var(--drupal-admin-color-gray-800, #4f5661); and that is color contrast of 1.9:1 against rgb(42,42,45) ... while the issue you have linked went to --admin-toolbar-color-gray-700: var(--drupal-admin-color-gray-700, #6d7583); which is 3.1:1. #d2d3d3 is just --gin-color-text which is identical to the menu items. but as @ckrina pointed out on the other issue the block title should be an unobstrusive label to a nav block and should not be using the identcial coloring.
i've tried to understand the problem, but i am unable to reproduce the problem in the first place unfortunately. i've ticked "Add close button to the Klaro! dialog" but for me the close button is still in the header section of the modal and not in the footer like outlined in the issue summary. could you provide some more instructions who to reproduce the problem? i am using 3.x.-dev of klaro and i am unable to reproduce the scenario you've outlined? thank you?
I am not sure if aria-expanded would be my attribute of choice here. aria-expanded is usually associated with expanding and collapsing elements in close proximity to the toggle button. https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Referenc... also lists the option of elements displayed or hidden, but still aria-expanded is more associated with cases like a details element and the expanded or collapsed summary element- where both are in close proximity.
in this case you have a dialog modal that is displayed in the center of the visual viewport, not in close proximity to the link/button in the bottom right corner. at the moment klarojs hasnt set up the dialog modal properly, it has neither an aria-modal attribute nor is using a dialog element (something we wanted to propose upstream in the klarojs queue on github). so at the moment a screenreaderuser would have the aria-expanded="true" attribute accessible (but it is not anywhere near in close proximity in the dom), but as soon as the aria-modal attribute would be in place or the dialog modal is being used, everything outside the dialog modal element is being excluded from the accessibility object model (AOM) and in consequence the aria-expanded="true" would be unavailable and inaccessible.
my preferred attribute of choice in this scenario would be the aria-haspopup="dialog". that way, if a user is getting the customize link/button into focus the aural interface would be Customize, dialog pop up, button. that way it would be crystal clear to the user that by pressing that link/button a dialog modal would open up. the expectations would be set in a very clear manner. a page for testing the attribute can be found here: https://a11ysupport.io/tests/html/aria/aria-haspopup_examples.html ... the support isn't 100% yet, there is an older overview https://a11ysupport.io/tech/aria/aria-haspopup_attribute as well as a more recent writeup here https://www.matuzo.at/blog/2023/aria-haspopup/ - in general it is good but the main offenders with only partial and or lacking support are narrator and talkback
i've discussed the topic with @mgifford and he agreed to my points, his only worry was also that the support for certain screenreaders is not perfect yet. at the end he added: "We are always working to the future with work on Core. aria-haspopup will get better support"
p.s. but would probably the best choice to make those changes in regard of aria-haspopup as well as aria-modal/dialog element upstream on the klarojs queue
jan kellermann → credited rkoller → .
pushed a commit extending the support to drupal 10 as well (tested on a drupal 10 install and all good) . as written on slack, i would created a release now and then discuss which issues need to be opened in the skipto drupal queue, the drupal core queue, and which in the skipto queue upstream. but i wouldnt make it a full release but rather an alpha1 release at first.
one thing i wonder after changing the target in the title to drupal 11, would it make sense to support certain minor versions of drupal 10 as well? would simply have to expand the info.yml. i only dont know how to test which drupal 10 version would be supported as well?
rkoller → created an issue. See original summary → .
thank you! i've manually tested on macOS Sequoia 15.3.1 with Safari 18.3, latest Firefox, and latest Edge - the voiceover output is the same in all three browsers. it looks like the prefixed abbreviations got already removed for default menu items with icons without the MR applied, but menu items without an icon still show the abbreviation:
With the MR applied those abbreviations get also hidden/removed for menu items that do not have an icon:
so overall that looks good to go. since the MR is following the approach suggested by @finnsky in #5 🐛 Remove prefixed abbreviations from top-level menu items Active i think it would be ok to set the issue to RTBC.
benjifisher → credited rkoller → .
benjifisher → credited rkoller → .
The issue was raised in the #bugsmash channel on the drupal slack. i've retested on drupal 11.x with macos 15.3.1, safari 18.3, in voiceover. - tested on a node in the article content type. i am sort of unable to reproduce the problem with the subject field of a comment, instead to me it looks like more of a problem with the ckeditor field used for the comment body field.
In the video (comment.mp4) i've first added something to the subject field then tabbed to the body field and added some text there as well, then i've shift tabbed back to the subject field and started deleting characters. those deleted characters were not announced. but as soon as i started entering characters again in the subject field either after deleting everything or after one or two characters only the newly entered characters are being announced. if i then delete again characters with backspace in the subject field, those characters are getting announced now as well in contrast to when i shift tabbed back into the subject field. i've tabbed into the body field then and tried to add or delete any characters there, none of that was announced. so from my point of view it seems more like a problem with the body field?! firefox and edge are announcing every character entered or deleted in the subject as well as the body field correctly.
not sure if those changes are reasonable and enough in scope and or valid but at least the errors are fixed and the config inspector lists the schema as 100% validatable now.
will try to provide an initial MR
thejimbirch → credited rkoller → .
the_g_bomb → credited rkoller → .
grienauer → credited rkoller → .
a heavy +1 for bringing SkipTo to Drupal 11
and your proposed solution on the MR you've created with #222330 for the navigation block title color would have a color contrast of 1.1:1 against the background (rgb(42,42,45)) of the navigation sidebar in gin.
for core and the navigation component there is already 🐛 Navigation block titles have a too low color contrast Active so 🐛 Increase the color contrast of the navigation block title Active is a duplicate of that. but i've opened the issue in here because you have a dark mode for gin that does not exist for core. and the shade of grey that is used for the navigation block title in gin is the same in light and dark mode. it is right to fix the color contrast for the navigation block title in core as well. but still the darker grey that will be picked as the solution for 🐛 Navigation block titles have a too low color contrast Active might then not work with the dark background in gin (and being mindful about gin would be out of scope for 🐛 Navigation block titles have a too low color contrast Active ). it might work but still out of scope and why i think it is reasonable having a dedicated issue.
benjifisher → credited rkoller → .
i am taking a look now, was swamped the last few weeks with finishing the draft for gin.
took an initial stab trying to improve the issue summary making it more actionable and instructional.
the_g_bomb → credited rkoller → .
i agree probably it might be not only about the color contrast but also about the font size, a detail we've touched before in yesterdays discussion. and i've updated the issue summary and added a pointer what the navigation block title is in the screenshot.
just as an fyi, the images are not focusable, only the three elements on the image, the checkbox, the edit button and the remove button.
@rahuly072 your theme settings are ok, but you also have to active the forced colors mode. your screenshots show that you are just in dark mode. you have to activate forced colors mode either in your operating system in case you are on windows or in chromium based browsers https://devtoolstips.org/tips/en/emulate-forced-colors/
rkoller → created an issue. See original summary → .
rkoller → created an issue.
rkoller → created an issue.
rkoller → created an issue.
rkoller → created an issue. See original summary → .