.hidden class is overridden by .claro-details

This will need a test case showing the issue.

Also @Santosh_Verma if you're going to do a separate MR helps to say why your solution is preferred over the other one. Will need an issue summary update now for proposed solution.

Hi @smustgrave, I have provided some context for my MR on issue #4. I am happy to update the purposed solution but I waiting for review of my MR and get some feedback.

Hi,
I have tested both merge request (MR) solutions for the issue and found that both of them are effective. However, determining which solution is a better approach depends on the specific context.

1. In MR !3426, removing the "display: block" property resolves the problem for elements where "block" is the default property. However, this may potentially introduce issues in other cases wheres it's need block property.

2. In my opinion, the second merge request (MR) !3914 appears to be a safer approach. It retains the "display: block" property while utilizing the "hidden" class to override it when necessary. This avoids the potential unknown consequences that may arise from removing the "display: block" property altogether.

Just ran into this and can confirm the issue still exists.

As of https://www.drupal.org/docs/accessibility/hide-content-properly → the class hidden should work, especially on core themes ;)

Sadly there's a further case of this in claro for .button:

.button {
  display: inline-block;
  margin: var(--space-m) var(--space-s) var(--space-m) 0; /* LTR */
  padding: calc(var(--space-m) - 1px) calc(var(--space-l) - 1px); /* 1 */
  cursor: pointer;
  text-align: center;
  -webkit-text-decoration: none;
  text-decoration: none;
  color: var(--button-fg-color); /* 2 */
  border-radius: var(--button-border-radius-size);
  background-color: var(--button-bg-color);
  font-size: var(--font-size-base);
  font-weight: 700;
  line-height: 1rem;
  -webkit-appearance: none;
  appearance: none;
  -webkit-font-smoothing: antialiased; /* 3 */
}

Adding hidden class on a form button also doesn't work as the .hidden is weighted lower than .button with display: inline-block;

Setting priority to Major, as this makes hidden elements visible in Claro and is against the documented Drupal Standards ( https://www.drupal.org/docs/accessibility/hide-content-properly → )

In our themes, we always use the hidden class together with the !important statement.
IMO, this class definitely should override everything.

Irrespective of changing it on different places like, .claro-details.hidden, button and other elements. Its better to change it on hidden class.
Not continuing with MR, as both the MR are fixing for one element and the problem with hidden is global. I have adapted the different approach (using important on display: none itself), attached patch for same. Please review

The last submitted patch, 11: 3340644-11.patch, failed testing. View results →

Adding for consideration, regarding the risk factor mentioned in #9: Using !important on .hidden or [hidden] affects not only CSS overriding but also javascript behaviors, using jQuery or vanilla js won't work as expected, e.g. if something is set to:

[hidden] {
  display: none !important;
}

and given a DOM structure like:

<div id="my-element" hidden="true"> ... </div>

Then jQuery('#my-element').show(); or document.getElementById('my-element').style.display = 'block'; will not work. The only way it'll work is to toggle the class or the attribute and this will be a change in expectations of how something should work.

I have tried a different approach, create patch and interdiff for same. please review

If a new solution is taken an issue summary will be required.

Updated IS with new solution

Patch has failures.

Issue summary appears to still be missing pieces specifically User interface section. Please use full issue template.

Also was tagged for tests.

I verified the latest patch #14 and it applied cleanly. It fixes the issue mentioned in the ticket description and #9. Also, I tried to add the class hidden in few other elements also eg., image field and the field is getting hidden from FE as expected. Please see the attached screenshots for reference.
I have updated the User interface section in Issue summary with screenshots. Need to write tests.

Screenshots didn't get attached in the previous comment. Attaching it here again.

Approach mentioned in #14 and in the issue summary is not the correct approach to solve this issue. Because the css properties used are for the .visually-hidden class which are only hiding the element visually but are announced for the Assistive Technologies (AT - eg. Screen Reader).

The class for .hidden and the [hidden] attribute css property "display: none;" does hide the concerned Element visually and is not announced to the AT.

We have now the possibility to only keep the scope on the .claro-details class by using the :not() pseudo class.

Therefore we can use the following (added with the new merge request 13131 above):

.claro-details:not(.hidden, [hidden]) { ... }

This approach is more maintainable and sustainable without affecting other core files (keep in mind, that claro will be remove be gin without any claro dependencies soon).