Problem/Motivation

Seeing this both in Webform tests on next minor and our own tests.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork webform-3563385

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

berdir created an issue. See original summary.

berdir opened merge request !789

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review

Looks like this happens because the referenced core issue has been fixed. I explicitly went with a version check here, because that will make it easier to remove this block once webform requires 11.3 at some point in the future.

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented

Why is it comparing against 11.2.99? Is that supposed to be 11.2.9?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

11.3-rc2 (which is the current 11.3 release) as well as 11.3.x-dev are considered to be lower than 11.3.0, so I'm using 11.2.99 (which will pretty much guaranteed never exist) as a version check: https://3v4l.org/onI8I

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented
Parent issue: » #2983248: [accessibility] Improve element, details, and fieldset description handling

This code doesn't make any sense to me. Am I misreading it? The only substantive thing that it does is run an unset(). That only happens if $element['#attributes']['aria-describedby'] is not empty, $element[$key]['#attributes']['aria-describedby'] is empty, and these two things are === to each other. That can never happen anyway.

This code was added in commit ed5fc4a for #2983248: [accessibility] Improve element, details, and fieldset description handling.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Yeah, your assessment sounds valid. I suspect the empty check on $element[$key]['#attributes']['aria-describedby'] should have been a !empty(), to prevent the error that we're running into now with 11.3.

I was first going to suggest to just remove the whole thing, but actually, I think it makes sense to fix the logic. Because as I suspected, WebformElementCompositeTest is an explicit test for this described-by logic and currently asserts the wrong result because it's not being fixed.

Instead of having to another add version check in the test and asserting different output, I changed the empty() to !empty(), changed the version check there to a @todo. This should now pass on all core versions.

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented

With your change, this removes aria-describedby from the child elements if it is the same as the aria-describedby on the parent element. That makes sense as a thing to do, but I don't see how it fits with the comment which is about aria-describedby the refers to not existing element ID. It only removes if it points to the same thing that the parent still points to. Still this appears to be an improvement and I'm inclined to merge it.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

I think I don't understand your comment.

The result of my change is that it results in the same behavior in 11.2 and earlier as core now produces by default in 11.3+. I believe that was always the intent of this code, which is why it referenced the core issue. The test change also passes on 11.3 when the code is removed, and eventually when you require ^11.3, you can drop it.

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented

The existing comment says "Some aria-describedby refers to not existing element ID." The code could unset $element[$key]['#attributes']['aria-describedby'] but only if that is the same as $element['#attributes']['aria-describedby']. So, if the former is referring to a "not existing element ID", then the latter will still refer to it. This code will not remove the reference to a "not existing element ID".

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

That comment is not an explanation of what the code does, it's the title of that core issue back in 2018 which has been changed in comment #9 there: #2839344-9: Broken aria-describedby in radios and checkboxes

vladimiraus’s picture

vladimiraus
Primary language English
Location Brisbane 🇦🇺
commented
Status: Needs review » Reviewed & tested by the community

Thank you for your contributions. 🍻
Applied and tested in D11.3.
The warning disappeared.

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented

Something is not right with the test. The comment says "Check radios 'aria-describedby' with wrapper description." but with this chanage there is no test at all for aria-describedby.

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented
Status: Reviewed & tested by the community » Needs work

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented
Title: Drupal 11.3: Undefined array key "aria-describedby" in webform_process_options() » Correct conditional in aria-describedby workaround
Status: Needs work » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.