- ๐ซ๐ทFrance mably
Hi, we just had an accessibility done on some of our websites by the french company Temesis and they suggested the fix proposed by @nyph1337 in comment #5 for the Webform help tooltips. Looks like the
aria-describedby
attribute is required.@jrockowitz Do we need to create a new issue for this or could this one just be reopened?
- Issue created by @Ddt Trung
- ๐บ๐ธUnited States nicxvan
At a quick glance that looks right: also if you want to link an issue in a comment where the issue status color comes up can you use:
[#issuenumber]like this โจ Add support for tables with two headers in views table display Needs work
- ๐บ๐ธUnited States theMusician
Thanks for the feedback. I agree that the undefined array key error is because the views did not get the update to set grouping_label_element to null.
The ViewsConfigUpdater code requirement makes sense to me as being necessary. On a separate issue, https://www.drupal.org/node/2770835 โ , the test update never fires even though I wrote a function in ViewsConfigUpdater but I was not aware you have to also call the function in updateAll(). The documentation improvements for how to use ViewConfigUpdater is a great idea.
Here is a rough idea of what I think is needed. @CarlyGerard also involved with this ticket walked me through what this may end up requiring. In the protected function addGroupingLabel() I can't quite figure out what the handler represents. The parameter defines it as a display handler, so I think a view style is represented here as the handler_type "style" and that is also the plugin_id? These ideas came from one of the issues you referenced, thank you for the pointer - https://git.drupalcode.org/project/drupal/-/blob/96d21a4603df23080a34051....
/** * Performs all required updates. * * @param \Drupal\views\ViewEntityInterface $view * The View to update. * * @return bool * Whether the view was updated. */ public function updateAll(ViewEntityInterface $view) { return $this->processDisplayHandlers($view, FALSE, function (&$handler, $handler_type, $key, $display_id) { $changed = FALSE; if ($this->addGroupingLabelElement($handler, $handler_type, $view)) { $changed = TRUE; } return $changed; }); } /** * Add Grouping Label to views without one. * * @param array $handler * A display handler. * @param string $handler_type * The handler type. * @param \Drupal\views\ViewEntityInterface $view * The View being updated. * * @return bool * Whether the handler was updated. */ protected function addGroupingLabelElement(array &$handler, string $handler_type, ViewEntityInterface $view): bool { $changed = FALSE; // Add grouping label element to existing views. if (($handler_type === 'style') && isset($handler['plugin_id'], $handler['type']) && $handler['plugin_id'] === 'style' && $handler['type'] === 'Grid' || 'HtmlList' || 'GridResponsive' || 'DefaultStyle' && !isset($handler['style']['grouping_label_element'])) { $handler['style']= ['grouping_label_element' => NULL]; $changed = TRUE; } return $changed; }
You asked, why the allowed grouping elements would not be restricted to just heading tags like the views pager update. For the grouping use cases, grid, HtmlList, GridResponsive it seemed hard to predict that one would only ever group by heading. Using labelELements, https://git.drupalcode.org/project/drupal/-/merge_requests/7351/diffs#11..., makes it so available elements do not need to be maintained in this code as well as the views.settings. I concur that the most common use case would be headings but I am not confident that there would never be a need to group by another element.
Thanks again for the feedback, the questions, and the guidance.
- ๐บ๐ธUnited States joegl
I was able to test sending the request a Referer header and it worked successfully.
In
core/modules/media/src/OEmbed/ResourceFetcher.php
starting on line 69:$response = $this->httpClient->request('GET', $url, [ RequestOptions::TIMEOUT => 5, 'headers' => [ 'Referer' => 'https://MYURL.COM', ] ]);
I then checked the response with and without the header. With the header it returned the title (and a number of other metadata). It also include the title attribute on the
iframe
. Without the header it was missing the metadata and had no title attribute on theiframe
.I will see if I can't work on actual patch/MR with the actual site URL instead of it hard-coded, but I wanted to test the path forward first. This might fit into its own issue at this point because of the specific scenario?
- ๐บ๐ธUnited States joegl
tl:dr; Unlisted or domain-restricted Vimeo videos do not return metadata (including the title) in the oembed API results unless the URL is sent as a Referer header.
I've noticed the title attribute missing from the inner
iframe
when using the oembed content in Drupal to display videos.From what I can tell, the core media module is getting the
iframe
code directly from the Vimeo API response using theiroembed.json
. The request is made incore/modules/media/src/OEmbed/ResourceFetcher.php
. A request to the Vimeo API looks like this:https://vimeo.com/api/oembed.json?url=https%3A//vimeo.com/VIMEO_VIDEO_ID
In reviewing the Vimeo API documentation for oembed's, this appears to be an issue with the video privacy settings. The videos on our sites have domain-level privacy enabled on Vimeo -- this means the videos can only be embedded and viewed on the configured domains. In the "Embedding videos with domain-level privacy" section, it says (paraphrasing):
For videos with the whitelist privacy setting โ that is, videos with domain-level privacy โ The oEmbed request returns a simplified response containing no private metadata.
The metadata excluded includes the "title" attribute.
The solution according to the documentation is to send the URL as the Referer header in with the request (and a curl example is given).
curl -e http://example.com https://vimeo.com/api/oembed.json?url=https:%2F%2Fvimeo.com%2F286898202
I'm not exactly sure how to build this into the `ResourceFetcher`, but that seems to be the solution for this (if possible).
- last update
1 day ago Composer error. Unable to continue. - ๐จ๐ฆCanada Liam Morland Ontario, CA ๐จ๐ฆ
Rebased. All comment threads resolved.
- last update
2 days ago Patch Failed to Apply - last update
2 days ago 15 pass - @rajeshreeputra opened merge request.
- First commit to issue fork.
- @gauravvvv opened merge request.
- ๐ฎ๐ณIndia Gauravvv Delhi, India
Gauravvvv โ made their first commit to this issueโs fork.
- @yashrode opened merge request.
- ๐บ๐ธUnited States kwiseman
Personally, I think it makes sense to narrow the scope of this issue to just the screenreader accessibility and move the design part to its own issue, given that design for the toggle state doesn't exist yet.
I created a merge request on a new branch that's caught up with 1.0.x and has the same changes.
Also, I forgot to mention in #3 that I manually tested with VoiceOver on a Mac in Firefox. Manual testing with other screen readers and/or browsers is probably necessary.
- @kwiseman opened merge request.
- ๐บ๐ธUnited States kwiseman
kwiseman โ changed the visibility of the branch 3446591-improve-styling-and to hidden.
- ๐บ๐ธUnited States bnjmnm Ann Arbor, MI
Patch testing will be deactivated in 32 days โ so it's best to do all of this in Gitlab.
-
+++ b/core/core.libraries.yml @@ -680,6 +680,14 @@ drupal.tabledrag.ajax: + - core/jquery + - core/drupal + - core/drupalSettings + - core/once + - core/drupal.displace
Currently none of these dependencies are used, but core/drupal probably should be used so Drupal behaviors are used instead of DomContentLoaded
-
+++ b/core/misc/tableheader.js @@ -0,0 +1,32 @@ + Sticky Header
Not translated
-
+++ b/core/misc/tableheader.js @@ -0,0 +1,32 @@ +document.addEventListener('DOMContentLoaded', function () {
Was this a GPT suggestion? GPT loves DOMContent loaded ๐. Drupal behaviors would be used for something like this
-
+++ b/core/misc/tableheader.js @@ -0,0 +1,32 @@ + const stickyHeaderElement = document.getElementsByClassName('views-table')[0];
Since you're just getting the first match
document.querySelector()
is cleaner -
+++ b/core/misc/tableheader.js @@ -0,0 +1,32 @@ + const checkbox = document.querySelector('[data-drupal-toggle-sticky-header]');
The theme function might be overridden and not have this selector. While it is ultimately up to the developer to make sure these things are in place, you have the return value 2 lines above and could reference whatever is returned by stickyHeaderCheckbox
-
+++ b/core/misc/tableheader.js @@ -0,0 +1,32 @@ + checkbox.addEventListener('change', function () {
Drupal uses arrow syntax
-
+++ b/core/misc/tableheader.js @@ -0,0 +1,32 @@ + if (checkbox.checked) { + if (!stickyHeaderElement.classList.contains('sticky-header')) { + stickyHeaderElement.classList.add('sticky-header'); + } + } else { + if (stickyHeaderElement.classList.contains('sticky-header')) { + stickyHeaderElement.classList.remove('sticky-header'); + } + }
All this logic can be just one line by using toggle()
-
- ๐ฎ๐ณIndia yash.rode pune
Re-created the tableheader.js for making the sticky optional. Added the sticky header checkbox before the sticky header.
need to be cleaned and use local storage to handle the page refresh. - ๐ฉ๐ชGermany rkoller Nรผrnberg, Germany
hm i am not sure if changing the styling from the regular button grey to the primary button blue for the pressed button would be the right choice. might be potentially confusing :/ technically there doesn't exist an actual toggle state for the drupal design system unfortunately yet. :( the following comment and the comments in the following in this thread https://drupal.slack.com/archives/C1AFW2ZPD/p1704456082112179?thread_ts=... are about introducing a toggle state to claro. the switch between the grid and list view would actually another example to be named in the list of examples in this linked discussion.
I wonder if it would make sense to narrow the scope for this issue and make it only about screenreader accessibility and move the design to a dedicated issue and maybe ping @ckrina in that regard, bringing the new toggle state for the drupal design system back on the map? what do you all think?
and would it be possible that you open a MR for this issue fork. that way it would be easier to apply the patch for me since i rely only on the composer patches based workflow still. would have to manually test the changes in voiceover.
- @joachim-desarmenien opened merge request.
- ๐ณ๐ฑNetherlands Dobefu
This has been fixed in https://git.drupalcode.org/project/iconify_field/-/merge_requests/10, and will be in the next release. Thank you for shopping with us
- ๐ณ๐ฑNetherlands Dobefu
Interesting, it appears that my tools did not pick it up. I'll go for the simpler solution for now, since #3450660 โจ Option to add attibutes in twig function Active will allow for better configuration when it's done.
- ๐ณ๐ฑNetherlands Dobefu
I've fixed the issue in https://git.drupalcode.org/project/iconify_field/-/merge_requests/9. I'll create a new release at the end of this weekend.
- ๐ณ๐ฑNetherlands Dobefu
I do like the solution with the image mask, but it does break the selection outline of the elements in the CKEditor. I'll try a slightly different approach to solve both issues
- ๐ณ๐ฑNetherlands Dobefu
Ah, that is something that I haven't yet tested properly. I will look into this after supper
- Issue created by @strnbrg
- Issue created by @joachim desarmenien
Automatically closed - issue fixed for 2 weeks with no activity.
- Issue created by @strnbrg
The Needs Review Queue Bot โ tested this issue.
While you are making the above changes, we recommend that you convert this patch to a merge request โ . Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)
- @gauravvvv opened merge request.