🇺🇸United States @aaronpinero

aaronpinero → created an issue.

aaronpinero → created an issue.

If anyone is interested, I am attaching the patch I am using on my sites while this and related D.o issues are sorted. I am applying the patch against version 8.x-1.24 on Drupal 10.3.5.

I think there are two issues with the current approach, at least as I understand how it's being implemented:

(1) The text for the alert dialog label should, at minimum, be configurable. Personally, I'd like to see that text be represented as a heading or text within the alert dialog itself, rather than using an aria-label attribute.

(2) Not sure why the aria-describedby attribute is being removed. From what I see on MDN (https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/al...) and what I read elsewhere, the alert dialog should have BOTH the aria-describedby attribute and either an aria-labelledby or aria-label attribute.

I would suggest getting this fix out quickly to address the immediate a11y errors. A larger question is whether or not to use the alert dialog role at all. I've seen some best practice suggestions point to using role="region" or a section element with an aria-labelledby attribute (which is supposed to be treated as a region by default). But that should be a separate issue.

I don't know much about determining what tests are needed or how to write the necessary tests.

I asked ChatGPT to evaluate the patch and suggest tests and this is what I got as a reply:

To ensure the patch works as expected, here are some test cases you should add to cover key areas of functionality. These can be incorporated into Drupal's existing test suite, particularly in a unit test or functional test format.

### **1. Check the Presence of `view-mode--default` Class When `data-view-mode` is Missing**
You need to test the fallback behavior when the `data-view-mode` attribute is absent. This ensures that the class `view-mode--default` is added correctly.

- **Test Case:** Render content with a caption element **without** a `data-view-mode` attribute.
- **Expected Outcome:** The `class` attribute of the caption element should contain `view-mode--default`.

```php
// Test case simulating a caption element without a 'data-view-mode' attribute.
$this->assertContains('view-mode--default', $processed_node->getAttribute('class'));
```

### **2. Check the Correct `view-mode--[mode]` Class When `data-view-mode` is Present**
Test that when a `data-view-mode` attribute is present, the correct `view-mode--[mode]` class is added to the caption element.

- **Test Case:** Render content with a caption element that **has** a `data-view-mode` attribute (e.g., `data-view-mode="teaser"`).
- **Expected Outcome:** The `class` attribute of the caption should include `view-mode--teaser` or the specified view mode.

```php
// Test case for a caption element with a 'data-view-mode' attribute.
$this->assertContains('view-mode--teaser', $processed_node->getAttribute('class'));
```

### **3. Verify Existing Class Attributes are Preserved**
Ensure that existing classes in the `class` attribute are preserved and the new `view-mode--[mode]` class is appended correctly.

- **Test Case:** Render content with a caption element that has existing classes (e.g., `class="some-existing-class"`).
- **Expected Outcome:** The `class` attribute should include both the existing class and the new `view-mode--[mode]` class.

```php
// Test case for an element that has existing classes.
$this->assertContains('some-existing-class', $processed_node->getAttribute('class'));
$this->assertContains('view-mode--default', $processed_node->getAttribute('class'));
```

### **4. Test Nested Elements (e.g., `` Inside `` or `

- **Test Case:** Render content where a captioned element is nested inside an anchor tag (``).
- **Expected Outcome:** The class handling should work properly, ensuring the correct class is added to the appropriate node.

```php
// Test case for a caption element inside an tag.
$this->assertContains('view-mode--default', $parent_node->getAttribute('class'));
```

### **5. Ensure No Duplicates in the Class Attribute**
Test that no duplicate `view-mode--[mode]` classes are added, even if the `data-view-mode` is processed multiple times.

- **Test Case:** Process a node multiple times with the same `data-view-mode`.
- **Expected Outcome:** The `view-mode--[mode]` class should only appear once.

```php
// Test case to check for duplicate class names.
$this->assertSame(1, substr_count($processed_node->getAttribute('class'), 'view-mode--default'));
```

### **6. Test Integration with Other Filters (e.g., Filter Align)**
Since the comment mentions collaboration with other filters like `filter_align`, ensure that the patch doesn’t break any existing functionality where alignment filters might also manipulate the class attributes.

- **Test Case:** Apply the `filter_align` filter to the captioned element in conjunction with the `data-view-mode` modification.
- **Expected Outcome:** The class attributes for both the view mode and alignment are applied correctly.

```php
// Test case where another filter like 'filter_align' collaborates with this patch.
$this->assertContains('align-left', $processed_node->getAttribute('class'));
$this->assertContains('view-mode--default', $processed_node->getAttribute('class'));
```

### **7. Test with Multiple View Modes**
Ensure that the patch works with various view modes (e.g., `teaser`, `full`, `custom`).

- **Test Case:** Render content with different `data-view-mode` values.
- **Expected Outcome:** The class should change dynamically to match the view mode provided.

```php
// Test case for multiple view modes like 'teaser', 'full', etc.
$this->assertContains('view-mode--full', $processed_node->getAttribute('class'));
$this->assertContains('view-mode--custom', $processed_node->getAttribute('class'));
```

### Additional Considerations
- **Sanitization:** Ensure that any `data-view-mode` values are sanitized or validated to avoid injecting invalid class names.
- **Edge Cases:** Test behavior when the `data-view-mode` contains unexpected or malformed values, such as special characters.

### Summary
These test cases will ensure that your patch is robust and works as expected across a variety of scenarios, from simple view modes to more complex interactions with nested elements and other filters. You can use Drupal's `KernelTestBase` or `BrowserTestBase` for functional tests, depending on how deeply integrated the changes are with the rendered content and front-end behavior.

I am adding an updated patch. I encountered a situation where, as described in one of the earlier comments, the submenus open and then immediately close. I believe this is because the JavaScript is not using core/once as a dependency. Inspecting the menu in the browser showed that the event handler for toggling open the submenus was being added multiple times.

The updated patch uses 'once' to make sure this doesn't happen. I added a relationship to another old issue that also suggested using 'once' in the JavaScript.

I am marking this now as Needs Review.

I created a simple patch which clears up the issue in my case.

I've found the issue:

On line 620 of HierarchicalTaxonomyMenuBlock.php, the following check is performed:

if ($this->configuration['hide_block'] && !$vocabulary_tree) {
  return [];
}

However, the code then proceeds to build a $vocabulary_tree_array, only then excluding 'empty' terms, as seen on line 642:

if (!empty($this->configuration['exclude_empty_terms']) && empty($entities)) {
  continue;
}

The check to hide the block should be done on $vocabulary_tree_array after it's generated, not before it's generated.

aaronpinero → created an issue.

I visited this issue because I am experiencing inconsistent results with the module. Sometimes the JavaScript for the hierarchical taxonomy menu will load properly and sometimes it will not. That is, the JavaScript appears in the HTML in a script tag but none of the code inside is being executed. This makes me think that because Drupal behaviors are not being used, the JS is running before the menu block is fully rendered and none of the event handlers for opening and closing submenus are being applied.

I added the attached patch against the 2.0.x branch (created a new one since the last one is quite old) and it seems like the issue has been resolved.

aaronpinero → created an issue.

aaronpinero → created an issue.

Nevermind. I was able to find the feature to make this happen. It was just not an obvious option.

aaronpinero → created an issue.

I upgraded my instance to Drupal 10.3.1 and PHP 8.3 and the problem I was seeing went away. I guess whatever it was is fixed in the latest version of Drupal.

I created the attached patch against 8.x-3.12 to resolve all the deprecation errors in my case...

I am also seeing deprecation warnings on other line numbers:

PHP Deprecated: Using ${var} in strings is deprecated, use {$var} instead in /var/www/html/web/modules/composer/lazy/lazy.module on line 216

Deprecated: Using ${var} in strings is deprecated, use {$var} instead in /var/www/html/web/modules/composer/lazy/lazy.module on line 216
PHP Deprecated: Using ${var} in strings is deprecated, use {$var} instead in /var/www/html/web/modules/composer/lazy/lazy.module on line 304

Deprecated: Using ${var} in strings is deprecated, use {$var} instead in /var/www/html/web/modules/composer/lazy/lazy.module on line 304
PHP Deprecated: Using ${expr} (variable variables) in strings is deprecated, use {${expr}} instead in /var/www/html/web/modules/composer/lazy/lazy.module on line 304

Deprecated: Using ${expr} (variable variables) in strings is deprecated, use {${expr}} instead in /var/www/html/web/modules/composer/lazy/lazy.module on line 304
PHP Deprecated: Using ${var} in strings is deprecated, use {$var} instead in /var/www/html/web/modules/composer/lazy/lazy.module on line 305

Deprecated: Using ${var} in strings is deprecated, use {$var} instead in /var/www/html/web/modules/composer/lazy/lazy.module on line 305
PHP Deprecated: Using ${var} in strings is deprecated, use {$var} instead in /var/www/html/web/modules/composer/lazy/lazy.module on line 312

Deprecated: Using ${var} in strings is deprecated, use {$var} instead in /var/www/html/web/modules/composer/lazy/lazy.module on line 312
PHP Deprecated: Using ${var} in strings is deprecated, use {$var} instead in /var/www/html/web/modules/composer/lazy/lazy.module on line 314

Deprecated: Using ${var} in strings is deprecated, use {$var} instead in /var/www/html/web/modules/composer/lazy/lazy.module on line 314
PHP Deprecated: Using ${var} in strings is deprecated, use {$var} instead in /var/www/html/web/modules/composer/lazy/lazy.module on line 314

Deprecated: Using ${var} in strings is deprecated, use {$var} instead in /var/www/html/web/modules/composer/lazy/lazy.module on line 314

aaronpinero → created an issue.

My first thought is to add a validation function to the replacement form checking to see if the tid of the existing term is the same as the tid of the new (replacement) term. If they are the same, return an error.

aaronpinero → made their first commit to this issue’s fork.

aaronpinero → created an issue.

Hi Peter,

Yes, I have a working module that I use for one website that I manage. The module still has the limitations I described in the original post, but it works for my limited needs, so I've not had the motivation to work on any of the proposed enhancements.

Thank you! I took a closer look at the original Sally code myself just to see what's going on here I think I understand why some of this was done the way it was done. I appreciate the effort to make it possible to run this with a CSP.

@mandclu making this configurable makes sense. I will work on that.

Thank you @cboyden for the 10.2.x patch. I have applied this to my v10.2.5 Drupal site using composer. The patch applied cleanly and appears to provide the expected a11y enhancement.

@itmaybejj Thank you!

aaronpinero → created an issue.

I created a patch for testing this by outputting a diff as follows:

% git diff 2.1.0-alpha1 3314213-add-open-by > ckeditor_details-3314213.patch

The problem with the current MR is that the merge request is for merging into 2.x (or maybe 2.1.x) and so the plain diff of the merge won't work for patching if you've installed the 2.1.0-alpha1 release of the module. The attached file worked for me when applied via composer where the plain diff of the MR did not. This should allow folks to test this change by patching the current module release.

One note: after patching the module, I found it necessary to re-save the text input format that is using the details widget. For some reason, the open attribute of the details element doesn't get immediately added to the allowed tags for the input format. After saving (with no changes) it was added and then the option to be open by default was available.

This worked for me on Drupal 10.2.5 using CKEditor 5 and CKEditor Detail Accordion 2.1.0-alpha1 patched with the attached patch via composer.

aaronpinero → created an issue.

I can also confirm that patch#21 seems to solve the problem in Drupal 10.2.5

I also appreciate having a patch available. I tested the patch in #77 and it worked for me in Drupal 10.2.4. I understand the desire to have folks follow the current practice of using a merge request, but it is also convenient to have the patch. I think we can encourage the following of desired practice without discouraging helpful contributions.

Nevermind, this does seem to work, sort of. It's just not documented anywhere how to do it. I think I've figured out how to make it work.

aaronpinero → created an issue.

With more experimentation, I realize now that facet order cannot be set at all through the facets admin interface. No matter what, the order of facets in the endpoint response seems to be alphabetical by key name. It would be nice if (a) the behavior was consistent and (b) the order of facets in the endpoint response respected the facet admin weight settings.

aaronpinero → created an issue.

@jacobupal TBH I never actually tried applying the change as a patch. I had only been testing the modified version of the module. However, you are correct. I also was unable to apply the patch.

If I had to guess, the part of the patch that's the problem is the change to js/build/detail.js. The change is essentially a replacement of the existing file. However, I suspect patch is trying to identify the existing line of code (one huge line in a minified JS file) and replace it with the new line of code (another huge line in a minified JS file). There could be little trivial differences in the minified code that are functionally insignificant but are throwing off the diff. Or maybe that one line of minified code is just too long.

Not sure how to resolve that one. It would be better to just get this merged and have a new release. But obviously some RTBC needs to be done first.

@acbramley I wanted to make sure I understand what you were reporting in #55 and #59. This is important to me because this issue is blocking the upgrade of my Drupal websites to 10.2. I am not going to tell my content editors that I now have to take away what is really a basic, essential feature of the content editor.

It sounds like you were saying that, following the upgrade to Drupal 10.2, I will need to:

- re-save my text format configurations
- export the website configuration to xml
- manually update the editor.editor.x.yml files to set the list properties.styles to true and add

and

to the allowed tags
- import the configuration

It also sounds like I will need to repeat this process if, in the future, I need to change my text format configurations for some other reason besides list styles. Is that correct? Are there any other steps that are necessary to get this to work? Is there a new patch I need to apply or are these steps sufficient without a patch? Thanks for working through this.

---------

I very much hope some of this can be resolved via patch before 10.3. This is really basic functionality for the editor that should never have been missing in the initial release of 10.0. More than a year later, it would be nice to see this finally cleaned up.

I can see where someone might not want an unpublished or draft node to be updated. Perhaps there should be a user option on the Replace page for the user to indicate if unpublished nodes should be included/excluded from the replace.

Thank you @malcomio. This is great news.

aaronpinero → created an issue.

aaronpinero → created an issue.

Hi all. I would definitely like to move this issue along. It's been indicated that tests are needed to move this back to "needs review". I would like to help but I am a novice developer and the only tests I've ever written for a module were based heavily on provided examples of Unit and Kernel tests. I don't know a whole lot about selecting the appropriate tests and then writing them.

That said, I did a bit of digging into the media module code. For tests related to the thumbnail formatter (where the proposed patch is applied) there is a Kernel test (core/modules/media/tests/src/Kernel/MediaThumbnailFormatterTest.php) and a Functional test (core/modules/media/tests/src/Functional/FieldFormatter/MediaThumbnailFormatterTest.php). The Kernel test seems to provide only a test to verify settings of the field formatter. The Functional test appears to actually test the rendering of the formatted media field as it would be used in a node.

Based on what I'm reading in the code, the appropriate test addition for this change to the module would be to modify the Functional test that already exists to include some check of the rendered alt text vs the alt text stored in the reference media entity. Leaving aside (for the moment) the details of actually executing this change to the functional test, does this seem like a reasonable plan? Are there additional tests that would need to be included that I haven't considered?

Thank you everyone who took the time to review this. I appreciate it.

Okay, I have provided an updated patch that, to reset a sort view, executes the deletion of rows from the db table but doesn't write any new data. I think this is more in keeping with the solution proposed in the original issue.

I realize I provided an incorrect solution. I will update the patch so, to reset, the table rows are not written to the database in the reset handler.

I am adding a patch which seems to, at least in my limited manual testing, provide the solution. I updated the .module file to add a draggableviews_views_reset handler function. This function is a near exact copy of the draggableviews_views_submit function, except all the weight values are set to 0 when data is written to the database. I also updated the draggableviews_form_alter to add the reset button.

It would be great if a maintainer could take a look at this. I think this is an easy and sensible addition to the module.

aaronpinero → created an issue.

aaronpinero → created an issue.

@deborahblessy Thank you for your suggestion regarding loading the jQuery library from CDN vs. requiring the library in composer. That might have fixed the issue. I need to do a more thorough check, but a quick test seems to indicate the console errors are gone.

The original problem description had some typos because I copied the text from a screen capture. I guess OCR still has issues in 2023.

I tried applying patch #43 to the beta6 release via composer. The patch applied cleanly, but my pages using the views reference field displayed the WSOD. The log recorded the following error:

AssertionError: When a #lazy_builder callback is specified, no properties can exist; all properties must be generated by the #lazy_builder callback. You specified the following properties: #attached. in assert() (line 334 of /var/www/html/web/core/lib/Drupal/Core/Render/Renderer.php)
#0 /var/www/html/web/core/lib/Drupal/Core/Render/Renderer.php(334): assert(false, 'When a #lazy_bu...')
#1 /var/www/html/web/core/lib/Drupal/Core/Render/Renderer.php(204): Drupal\Core\Render\Renderer->doRender(Array, false)
#2 /var/www/html/web/core/modules/views/src/Plugin/views/field/EntityField.php(934): Drupal\Core\Render\Renderer->render(Array)
#3 /var/www/html/web/core/modules/views/src/Plugin/views/field/FieldPluginBase.php(1170): Drupal\views\Plugin\views\field\EntityField->render_item(0, Array)
#4 /var/www/html/web/core/modules/views/views.theme.inc(238): Drupal\views\Plugin\views\field\FieldPluginBase->advancedRender(Object(Drupal\views\ResultRow))
#5 [internal function]: template_preprocess_views_view_field(Array, 'views_view_fiel...', Array)
#6 /var/www/html/web/core/lib/Drupal/Core/Theme/ThemeManager.php(287): call_user_func_array('template_prepro...', Array)
#7 /var/www/html/web/core/lib/Drupal/Core/Render/Renderer.php(433): Drupal\Core\Theme\ThemeManager->render('views_view_fiel...', Array)
#8 /var/www/html/web/core/lib/Drupal/Core/Render/Renderer.php(204): Drupal\Core\Render\Renderer->doRender(Array, false)
#9 /var/www/html/web/core/modules/views/src/Plugin/views/field/FieldPluginBase.php(1746): Drupal\Core\Render\Renderer->render(Array)
#10 /var/www/html/web/core/modules/views/src/Plugin/views/style/StylePluginBase.php(777): Drupal\views\Plugin\views\field\FieldPluginBase->theme(Object(Drupal\views\ResultRow))
#11 [internal function]: Drupal\views\Plugin\views\style\StylePluginBase->elementPreRenderRow(Array)
#12 /var/www/html/web/core/lib/Drupal/Core/Security/DoTrustedCallbackTrait.php(101): call_user_func_array(Array, Array)
#13 /var/www/html/web/core/lib/Drupal/Core/Render/Renderer.php(788): Drupal\Core\Render\Renderer->doTrustedCallback(Array, Array, 'Render #pre_ren...', 'exception', 'Drupal\\Core\\Ren...')
#14 /var/www/html/web/core/lib/Drupal/Core/Render/Renderer.php(374): Drupal\Core\Render\Renderer->doCallback('#pre_render', Array, Array)
#15 /var/www/html/web/core/lib/Drupal/Core/Render/Renderer.php(204): Drupal\Core\Render\Renderer->doRender(Array, false)
#16 /var/www/html/web/core/modules/views/src/Plugin/views/style/StylePluginBase.php(716): Drupal\Core\Render\Renderer->render(Array)
#17 /var/www/html/web/core/modules/views/src/Plugin/views/style/StylePluginBase.php(582): Drupal\views\Plugin\views\style\StylePluginBase->renderFields(Array)
#18 /var/www/html/web/core/modules/views/src/Plugin/views/style/StylePluginBase.php(473): Drupal\views\Plugin\views\style\StylePluginBase->renderGrouping(Array, Array, true)
#19 /var/www/html/web/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php(2169): Drupal\views\Plugin\views\style\StylePluginBase->render(Array)
#20 /var/www/html/web/core/modules/views/src/ViewExecutable.php(1538): Drupal\views\Plugin\views\display\DisplayPluginBase->render()
#21 /var/www/html/web/core/modules/views/src/Plugin/views/display/Block.php(131): Drupal\views\ViewExecutable->render()
#22 /var/www/html/web/core/modules/views/src/ViewExecutable.php(1635): Drupal\views\Plugin\views\display\Block->execute()
#23 /var/www/html/web/core/modules/views/src/Element/View.php(81): Drupal\views\ViewExecutable->executeDisplay('block_3', Array)
#24 /var/www/html/web/core/modules/views/src/Plugin/Block/ViewsBlock.php(59): Drupal\views\Element\View::preRenderViewElement(Array)
#25 /var/www/html/web/core/modules/block/src/BlockViewBuilder.php(171): Drupal\views\Plugin\Block\ViewsBlock->build()
#26 [internal function]: Drupal\block\BlockViewBuilder::preRender(Array)
#27 /var/www/html/web/core/lib/Drupal/Core/Security/DoTrustedCallbackTrait.php(101): call_user_func_array(Array, Array)
#28 /var/www/html/web/core/lib/Drupal/Core/Render/Renderer.php(788): Drupal\Core\Render\Renderer->doTrustedCallback(Array, Array, 'Render #pre_ren...', 'exception', 'Drupal\\Core\\Ren...')
#29 /var/www/html/web/core/lib/Drupal/Core/Render/Renderer.php(374): Drupal\Core\Render\Renderer->doCallback('#pre_render', Array, Array)
#30 /var/www/html/web/core/lib/Drupal/Core/Render/Renderer.php(446): Drupal\Core\Render\Renderer->doRender(Array)
#31 /var/www/html/web/core/lib/Drupal/Core/Render/Renderer.php(204): Drupal\Core\Render\Renderer->doRender(Array, false)
#32 /var/www/html/web/core/lib/Drupal/Core/Template/TwigExtension.php(479): Drupal\Core\Render\Renderer->render(Array)
#33 /var/www/html/web/sites/default/files/php/twig/64f8e0a2e7225_page.html.twig_e7hh4y0wifva4a_ScRFDQ36Sd/zfFVEjIYHqdRX1anIHqxRCGpcFxi4utbCkf16f__ry0.php(147): Drupal\Core\Template\TwigExtension->escapeFilter(Object(Drupal\Core\Template\TwigEnvironment), Array, 'html', NULL, true)
#34 /var/www/html/vendor/twig/twig/src/Template.php(405): __TwigTemplate_4c484e285e34c1c517dfce0f56403aae->doDisplay(Array, Array)
#35 /var/www/html/vendor/twig/twig/src/Template.php(378): Twig\Template->displayWithErrorHandling(Array, Array)
#36 /var/www/html/vendor/twig/twig/src/Template.php(390): Twig\Template->display(Array)
#37 /var/www/html/web/core/themes/engines/twig/twig.engine(55): Twig\Template->render(Array)
#38 /var/www/html/web/core/lib/Drupal/Core/Theme/ThemeManager.php(384): twig_render_template('themes/custom/h...', Array)
#39 /var/www/html/web/core/lib/Drupal/Core/Render/Renderer.php(433): Drupal\Core\Theme\ThemeManager->render('page', Array)
#40 /var/www/html/web/core/lib/Drupal/Core/Render/Renderer.php(204): Drupal\Core\Render\Renderer->doRender(Array, false)
#41 /var/www/html/web/core/lib/Drupal/Core/Template/TwigExtension.php(479): Drupal\Core\Render\Renderer->render(Array)
#42 /var/www/html/web/sites/default/files/php/twig/64f8e0a2e7225_html.html.twig_K7e0YUqNYNYFXZjaSejrhWCNJ/F9GmlgEUaQj5BFA8BJiV1mhKLHdT20aW1lYKJfOlhzk.php(87): Drupal\Core\Template\TwigExtension->escapeFilter(Object(Drupal\Core\Template\TwigEnvironment), Array, 'html', NULL, true)
#43 /var/www/html/vendor/twig/twig/src/Template.php(405): __TwigTemplate_e64b960fb8704841d8453d0d3f187c3c->doDisplay(Array, Array)
#44 /var/www/html/vendor/twig/twig/src/Template.php(378): Twig\Template->displayWithErrorHandling(Array, Array)
#45 /var/www/html/vendor/twig/twig/src/Template.php(390): Twig\Template->display(Array)
#46 /var/www/html/web/core/themes/engines/twig/twig.engine(55): Twig\Template->render(Array)
#47 /var/www/html/web/core/lib/Drupal/Core/Theme/ThemeManager.php(384): twig_render_template('themes/custom/h...', Array)
#48 /var/www/html/web/core/lib/Drupal/Core/Render/Renderer.php(433): Drupal\Core\Theme\ThemeManager->render('html', Array)
#49 /var/www/html/web/core/lib/Drupal/Core/Render/Renderer.php(204): Drupal\Core\Render\Renderer->doRender(Array, false)
#50 /var/www/html/web/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php(162): Drupal\Core\Render\Renderer->render(Array)
#51 /var/www/html/web/core/lib/Drupal/Core/Render/Renderer.php(580): Drupal\Core\Render\MainContent\HtmlRenderer->Drupal\Core\Render\MainContent\{closure}()
#52 /var/www/html/web/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php(163): Drupal\Core\Render\Renderer->executeInRenderContext(Object(Drupal\Core\Render\RenderContext), Object(Closure))
#53 /var/www/html/web/core/lib/Drupal/Core/EventSubscriber/MainContentViewSubscriber.php(90): Drupal\Core\Render\MainContent\HtmlRenderer->renderResponse(Array, Object(Symfony\Component\HttpFoundation\Request), Object(Drupal\Core\Routing\CurrentRouteMatch))
#54 [internal function]: Drupal\Core\EventSubscriber\MainContentViewSubscriber->onViewRenderArray(Object(Symfony\Component\HttpKernel\Event\ViewEvent), 'kernel.view', Object(Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher))
#55 /var/www/html/web/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php(142): call_user_func(Array, Object(Symfony\Component\HttpKernel\Event\ViewEvent), 'kernel.view', Object(Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher))
#56 /var/www/html/vendor/symfony/http-kernel/HttpKernel.php(174): Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch(Object(Symfony\Component\HttpKernel\Event\ViewEvent), 'kernel.view')
#57 /var/www/html/vendor/symfony/http-kernel/HttpKernel.php(81): Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object(Symfony\Component\HttpFoundation\Request), 1)
#58 /var/www/html/web/core/lib/Drupal/Core/StackMiddleware/Session.php(58): Symfony\Component\HttpKernel\HttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#59 /var/www/html/web/core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php(48): Drupal\Core\StackMiddleware\Session->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#60 /var/www/html/web/core/modules/ban/src/BanMiddleware.php(50): Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#61 /var/www/html/web/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php(48): Drupal\ban\BanMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#62 /var/www/html/web/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php(51): Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#63 /var/www/html/vendor/stack/builder/src/Stack/StackedHttpKernel.php(23): Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#64 /var/www/html/web/core/lib/Drupal/Core/DrupalKernel.php(718): Stack\StackedHttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#65 /var/www/html/web/index.php(19): Drupal\Core\DrupalKernel->handle(Object(Symfony\Component\HttpFoundation\Request))
#66 {main}

@Nishant2312 based on your description, I do not think you configured your menu correctly to illustrate the problem. Perhaps this could be better explained over a screen sharing session on Slack. I would be happy to schedule something if you feel it would be helpful.

aaronpinero → created an issue.

Thank you!

Thank you @hemangi-gokhale for the specific recommendations. I have addressed the points listed in your message and updated the 1.0.x branch accordingly.

aaronpinero → created an issue.

@Shiraz_Dindar this is not correct. The changes appear to have been merged into the -dev branch, but they are not in the tagged 3.11 release. There needs to be a new tagged release (3.12?) that incorporates the changes.

The patch in #16 applied cleanly using composer, fixed the compatibility of the module with D10 (as per upgrade status module) and seems to preserve the functionality of the module. So RTBC+1

Patch #34 does not cleanly apply to the latest beta (6). I didn't check #40. I've been depending on patch #34 for quite a while now, it would be great if we could get an updated patch (or, better yet, something permanent merged).

Thanks.

I may have been successful in adding UI for the CKEditor Details widget allowing users to set the default open state of the details element.

With the update to the module, if the user clicks on the details widget in the editor, the user will see a balloon popup with a switch button. The switch button is labeled "open by default" and the switch is default off. If the user toggles the switch, the details element will be open by default when the content is viewed.

The balloon popup will not be displayed when the user is editing the summary or text within the details wrapper element. The user needs to click on the grey area surrounding the summary and wrapper for the balloon to appear.

Since there hasn't been any response to this request in many months, I am trying to do it myself.

The update I am looking to make will expose a toggle switch when a details widget is selected in the editor, allowing the using to indicate if the details widget should be open by default or not.

If anyone's interested, I am working on the CKEditor part of this update outside of Drupal. This allows me to focus on the functionality of the CKEditor plugin without worrying about the Drupal integration. The code-in-progress is at https://github.com/aaronpinero/cke5_dev

Okay, the dev branch now includes the update to the ContentReviewNotificationSettingsForm to use dependency injection of the State.

Can we get this merged and a new release created? Looking at this patch, the update was simply to change the supported Drupal versions in the .info.yml file -- a truly benign update.

@vinaymahale the line exceeds 80 characters issue has been resolved on the 1.0.x branch, along with several other similar error notices.

@scott_euser thank you for this suggestion. Since there was only one place where I was using the Drupal::messenger() service, I removed it.

I also adjusted the two main functions in the .module file (the hook_cron() and simple_content_notifications_prep()) to perform a check for the enabled status configuration value before doing anything else. This way nothing happens if that part of the module function is not enabled by the administrator.

To help make this more clear, I also adjusted the config forms so that the enable/disable option is the first item in each configuration form.

The changes are committed to the dev branch.

Can a new release be created that includes the D10 compatibility updates?

@scott_euser yes, that seems fine.

@scott_euser I am thinking that I might, while working on the other issue, split this into two configuration options so logging for each type of notification (content updates vs content review) can be enabled/disabled separately. Right now, the option makes it seem like this option is just for logging with content update notifications.

I've reviewed the patch and it mostly looks good. There are two things that I need to address:

- there is call to the config service in the hook_cron function that can be removed since the date is going to now be stored in state.
- there needs to be accounting for the case where there is no saved state.

@scott_euser thank you for this work. I will do my best to review this today or tomorrow so as to get the module updated quickly.

This issue has been resolved in the latest release.

Setting to needs review to get some other eyes on this.

Existing tests for the StringFilter in the Views module all have to do with whether or not use of the exposed filter produces the expected result set. I don't know what sort of assertion one might use to to test this change other than adding a very simple test that checks to see if the input string longer than 128 characters is truncated. This is easy enough to test manually, so I don't know that it's necessary to add an automated test.

If no one has a differing viewpoint, this could be RTBC.

As suggested in #7 and #10, I have created an updated version of the patch that has comments.

The parent issue has proposed a fix that would address this issue "further up the chain" from Views. However, that fix is against Drupal 11.x, so we do need a fix applied for Drupal 9.5 (as this will be around for a while yet). The parent issue also suggests (option b) to set the maxlength to to 256 instead of 128 and we should probably go with that, providing a fix against Drupal 9.5.x

Looking at this issue, it seems like the following specific tasks will help get it to RTBC:

1. rewrite of the issue summary using the standard template. Also, generalizing the summary since this is about a search input but the character limit affects views generally.

2. agreeing on the patch that sets maxlength to 255. This seems to be a responsible setting (as opposed to null) and matches the aforementioned title character limit.

3. Check to see what tests are available for this module and if tests are adequate or need to be updated.

These notes are mostly for #novice #contribution #DrupalConPittsburgh2023

@vishal.kadam I addressed the items you listed.

For the info.yml file, I set the core version requirement to ^9 || ^10. The addition of ^10 is in an open issue on the module for automated D10 compatibility fixes. The addition of ^10 to the core version requirement was the only compatibility fix listed.

I also removed the unused methods from the Forms. I was previously under the impression that these methods needed to be declared in order for the Form controller to work, regardless of whether it was used. However, I see in manual testing that the forms work fine without the unused method being declared.

aaronpinero → created an issue.

I am uploading a new patch because the current one doesn't apply cleanly through composer with Drupal 9.5.9. There was an update in 9.5.9 to the media thumbnail formatter that need to be accounted for. That said, the patch is VERY similar.

One thing I noted about the site after the patch: if the image is missing alt text, an empty alt attribute is added to the image. Not sure if that's a desired outcome... would it be better to leave off the alt attribute entirely if there's no text available?

I tested the patch with Drupal 9.5.8 and it appears to solve the issue. I am seeing alt attribute information on a media image thumbnail where previously there was none.

This seems like a pretty basic and important accessibility problem. I just noticed it today when doing a WAVE report on my website. This issue was the first error reported. I thought it was a content author mistake. It's a surprising oversight for the Media system. Can we bump this up to critical?

A new release has been created which includes the update.

Thanks for adding this. I will work on getting the change merged in.

Uploading new patch with previously mentioned accessibility recommendations.

I wonder if it would be useful to follow this accessibility guidance:
https://a11y-guidelines.orange.com/en/articles/technical-iframe/

I would consider the google tag manager iframe to be a technical iframe (as opposed to an iframe that is providing content to the user. For accessibility compliance and for hiding the iframe from the accessibility API, it is recommended that:

- the iframe has a title that identifies it as a technical element rather than a content element
- it has the attribute aria-hidden="true"
- it has tabindex="-1"