Fix label visibility and add wrapper container for exposed numeric/date filters with multiple form elements

1. +++ b/core/modules/views/src/Plugin/views/filter/NumericFilter.php
   @@ -114,6 +115,22 @@ function operators() {
   +      $form['operator']['#prefix'] = '<span class="views-filter-wrapper views-filter-' . Html::cleanCssIdentifier($this->options['expose']['identifier']) . '">';
   

   I'm wondering whether there is any way we could move that HTML into the template level? I guess no, because core/modules/views/templates/views-exposed-form.html.twig is not dealing with that level of detail at the moment.

2. +++ b/core/modules/views/src/Plugin/views/filter/NumericFilter.php
   @@ -186,9 +205,9 @@ protected function valueForm(&$form, FormStateInterface $form_state) {
   -        '#title' => !$exposed ? $this->t('Value') : '',
   +        '#title' => $exposed ? '' : $this->t('Value'),
            '#size' => 30,
   -        '#default_value' => $this->value['value'],
   +         '#default_value' => $this->value['value'],
   

   These changes aren't really strictly needed, right?

Strait re-roll before I start working on feedback. I'm moving to a normal priority. It should get fixed as it makes these exposed filters very ugly and is a regression from D7.

Some cleanup and addressing #15.2.

+++ b/core/modules/views/src/Plugin/views/filter/NumericFilter.php
@@ -230,10 +230,9 @@ protected function valueForm(&$form, FormStateInterface $form_state) {
-        '#field_prefix' => $exposed ? $this->t('and') : '', // Add separating label.

This doesn't seem necessary.

I did some manual testing of this and I think at this point, the test might be wrong. Pursuing that theory.

So, what was missing in the scenarios was when you had multiple, without an exposed operator. I've taken a pass updating the IS with what I think should be done. Essentially, we do what was done in D7.

Updates IS with screenshots of actual D8 site with changes from this patch. Tests aren't updated yet so will likely fail. Started out with a fresh patch by comparing against D7 and using a fieldset form wrapper for all but the simple single value, so no interdiff.

Fixed the last annoying thing in the filter admin form. It wasn't in sync with the exposed filter form and was confusing. Next up is fixing the tests. If someone gets to it before me, go ahead.

Overall, we're in a much better place. However, at some point, we should go through and clean-up the numeric filter form logic. It is rather more complicated than I think it needs to be.

The remaining failures are due to the form not getting loaded with all 3 input boxes on config form load. So when an administrator changes to 'between', the two extra Max/Min fields don't show up until you save and revisit the config form. I'll see if I can get time to fix that, but for me it is a trivial UX bug and the filters work. So if someone has interest, please proceed.

This last patch should fix up the schema config validation failure.

Patch doesn't seem to apply any more.

Patch was re-rolled. It was resolved merge conflicts related to the short array syntax, which replaces array() with [];
It was also fixed some simple issues related to coding standards.

Patch didnt apply to 8.4, figured that the test structure has changed to tests/src/Functional.

Submitting corrected patch.

am trying to look into the test failures and there is some problem with the code changes in this patch. For a grouped filter, when 'in between' is chosen as operator, only one form field is present, not two as expected on Drupal 8.4 .

Oops, not just for grouped filters, even for single filters, the in between filters are not working..

Ok this issue has been noted earlier in #35. It seems like some issue in the code with the way #states is being set.

Thanks,
Sukanya

Submitting updated patch and interdiff.
Lets see what the testbot thinks now.

Does the word "operator" need to appear in the UI? It's apparent from the content that it is an operator.

I would expect:

Salary
is less than

or

Salary
is between
min
and
max

"Operator" probably does belong as a label under the hood as a label for screen readers.

That said, since the field is aimed at site visitors, I wonder whether "Operator" is too technical, and, if it is going to show, whether it might better read "Comparison".

Although the Numeric/Date filters may highlight the problem, I think this is wider spread than any one filter type. In a general sense, there is no logical grouping of the operator and value forms that would make it intuitive for a screen reader or possible to target styling. FWIW, I also tend to agree with @Charles Belov in that the "Operator" label is unhelpful.

Trying to manipulate the operator form from within the value form also feels wrong to me. I've also been having trouble getting the patch to work correctly with Drupal since about 8.2 (although it applies cleanly).

Accordingly, I'm adding this patch as a potential alternative approach that is a bit more generic. There are a few of the existing code style fixes that are in the area where changes are targeted. I haven't invested in writing any tests yet — until an approach is decided on, it seems like it would be wasted effort.

Adjustments for test failure & filters that have value forms with multiple elements.

+++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
@@ -850,6 +849,17 @@
+      // When the operator and value forms are both in play, enclose them within
+      // a wrapper, for usability.
+      if (!empty($this->options['expose']['identifier'])) {

I wonder if a change notice is needed for this? Since it fixes a bug, probably not, but I suppose there is a chance custom themes have already worked around this and the wrapper might impact those developers.

As mentioned in #15, while nice, these coding standards fixes below are out of scope for this issue (and will be fixed when the relevant coding standards issue gets committed).

+++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
@@ -830,9 +830,8 @@
-   * Render our chunk of the exposed filter form when selecting
+   * Render our chunk of the exposed filter form when selecting.

+++ b/core/modules/views/src/Plugin/views/filter/NumericFilter.php
@@ -130,8 +130,9 @@ class NumericFilter extends FilterPluginBase {
-   * Provide a simple textfield for equality
+   * Provide a simple textfield for equality.

@@ -149,7 +150,7 @@ class NumericFilter extends FilterPluginBase {
-        // exposed and locked.
+        // Exposed and locked.

@@ -227,7 +227,7 @@ class NumericFilter extends FilterPluginBase {
-          '#value' => NULL
+          '#value' => NULL,

These coding standard cleanup changes, while nice, are probably out of scope

+++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
@@ -830,9 +830,8 @@
-   * Render our chunk of the exposed filter form when selecting
+   * Render our chunk of the exposed filter form when selecting.

+++ b/core/modules/views/src/Plugin/views/filter/NumericFilter.php
@@ -130,8 +130,9 @@ class NumericFilter extends FilterPluginBase {
-   * Provide a simple textfield for equality
+   * Provide a simple textfield for equality.

@@ -149,7 +150,7 @@ class NumericFilter extends FilterPluginBase {
-        // exposed and locked.
+        // Exposed and locked.

@@ -227,7 +227,7 @@ class NumericFilter extends FilterPluginBase {
-          '#value' => NULL
+          '#value' => NULL,

Whoops, good point — I missed the comment in #15. Here is the previous patch sans coding standards fixes.

I have this patch working from several months, thank you

Re-rolled.

Manually tested it and it works as expected. Code changes look goog to me.

+++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
@@ -880,6 +891,40 @@ public function buildExposedForm(&$form, FormStateInterface $form_state) {
+  protected function buildValueWrapper(&$form, $wrapper_identifer) {
+    if (!isset($form[$wrapper_identifer])) {
+      $form[$wrapper_identifer] = [

I'm not sure about this line: if I'm not mistaken, we are silently skipping the wrapper if one element with the same ID already exists. IMO this may result in poor DX/TX, as one may not realize why the wrapper is not showing up, without inspecting the form array structure. Given that such collision should happen only if some code is altering the form and injecting a conflicting element (the Views UI won't allow conflicting IDs), I'm wondering whether it would make sense to add an else branch throwing an exception.

Added before/after screenshots.

Unrelated fails

#66 needs answering.

1. +++ b/core/modules/views/src/Plugin/views/filter/NumericFilter.php
   @@ -260,17 +260,16 @@ protected function valueForm(&$form, FormStateInterface $form_state) {
   -        '#title' => !$exposed ? $this->t('Min') : $this->exposedInfo()['label'],
   +        '#title' => $this->t('Min'),
   

   Are we sure about losing the ability to customise this?

2. +++ b/core/modules/views_ui/tests/src/Functional/FilterNumericWebTest.php
   @@ -109,10 +109,10 @@ public function testFilterNumericUI() {
   -    $this->assertRaw('<label for="edit-age-min">Age between</label>', 'Min field label found');
   

   This label is set up above - $edit['options[expose][label]'] = 'Age between'; what happens to it now?

In regard to #6, that seems like a fair point. I'll toss an exception in there.

1. The label that used to be applied to the Min input element now adorns the wrapper (and is still based on configuration). You already cannot customize the Max label, so we're now making it consistent across both of those elements. Moreover, currently when you expose the operator, the Min input element is the only thing that gets the field label, so if the end user switches to "equal to" instead, the field label completely disappears from screen because it's tied to the Min element only, which presents poor UX. So it's true that there is a slight trade-off, but I'd argue that the gains substantially outweigh the loss.
2. I'll add in a check for that existing on the wrapper.

Here are those couple tweaks. As long as we're already touching those assertions, I went ahead and made deprecation adjustments.

Ah, now I'm sort of remembering why that bit was in there in the first place:

The wrapper has to exist to be able to put the input elements into it. If you've both exposed the field and the operator, then the method gets invoked twice. (see buildExposedForm() -- build the wrapper, put the operator in it, build the wrapper, put the field in it.) We don't want the previous contents of the wrapper to be wiped out on that second pass, hence why it had that check.

Just want to note, the patch in #76 fixed the issue we were having with Views exposed filters, operators, labels and grouping, with thanks.
One issue was it doesn't work with the "better_exposed_filters" module (maybe as we used the custom, shorter filter name in the view setting), but as we can use the default exposed filter plugin, it's fine, thanks.

One issue was it doesn't work with the "better_exposed_filters" module (maybe as we used the custom, shorter filter name in the view setting)

My guess is it doesn't work with BEF jquery date plugin?

I think this means this issue needs a change record. Aside from that, I think the patch in #76 has addressed the remaining issues from #71 and can be RTBC pending a CR.

I'm going to write the change record

Change record available https://www.drupal.org/node/2984152

#76 is working for me

I think this is now RTBC.

I move change record to 8.7.x

Failing test does not look related, let's try again.

I tested patch in #76 and I found no problems using it together with Better Exposed Filters.
The tests also pass now.

However, I tested patch in #76 together with patch in #166 here, which is also on the way into core: #2648950-166: [PP-2] Use form element of type date instead textfield when selecting a date in an exposed filter
When I apply the patches together the containing span for the filters are gone again, so this:

Becomes like this:

Thanks for the detailed report and screenshots.
While I that this conflict will have to be resolved eventually, I think this issue should be reviewed independently.
We can't anticpate how the other thread will be resolved, and it's not practical to maintain compatibility between multiple separate issues unless they are hard dependencies. Further, since this is a patch to the base class, maybe 2648950 should actually be postponed on this issue.

Regardless, if 2648950 is committed first, we'll have to update this patch.
If this patch is committed first, 2648950 will have to be updated.

I'm coming in late to this thread - does this approach make sense?
In general, I think that's the standard approach to take.
If so, please reset to "needs review"
Is there other context with this issue that I'm missing?

I don't know what is the best approach, but as it was already set to RTBC before the tests failed, I put it back again, and I guess we'll find out.

CR is at https://www.drupal.org/node/2984152 so removing that tag.

I'm not sure if "Needs usability review" is still valid or not. That was requested in #68 and not really addressed since. It seems like this is an improvement on an existing and obvious UI bug. I'd rather perfect not become the enemy of good, but if this needs more formal review/testing/sign-off, so be it. I'll leave it at RTBC and let a core committer decide.

Thanks,
-Derek

I applied the patch in #76 on a Drupal 8.6.2 site and it's working as expected. Thank you! Much improved UI.

I plan to bring this up at the weekly Usability meeting in about 4 hours (3:30 PM EST). Join us on the #ux channel in the Drupal Slack account if you would like to be part of the discussion.

At the Drupal Usability meeting we reviewed the suggested patches (participating Benji Fisher and Thomas Howell).

Change examined at "Structure" -> "Views" -> "Content"

Testing patch results:
Testing was a bit time consuming. We added the “changed” filter to the Content page for administration installed by the standard profile. Testing revealed that Tab order hasn’t changed with the addition of the patch.

Follow up questions:

1. Including everything in the field set is suspected to improve or maintain the current state of accessibility for this scenario but further testing is advised to confirm.
2. Could the alignment be improved for Bartik?

Conclusion:
Despite the possibility of a few outstanding improvements for alignment and labeling we feel that this patch is a clear usability win and we don’t view the absence of those potential improvements as blockers.

As @worldlinemine said, we discussed this at the Drupal Usability Meeting 2018-11-13 (recording on Youtube). This issue looks like a clear improvement.

I would like to see an accessibility review just in case there is a problem we did not notice.

One thing that slowed us down a bit was that the title did not make clear that the change only affects exposed filters with exposed operators. I see that the CR explicitly mentions the operator. I will update the title to clarify this, but I expect someone who has worked on this issue may want to refine my change.

+++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
@@ -880,6 +891,43 @@ public function buildExposedForm(&$form, FormStateInterface $form_state) {
+      // a wrapper, for usability. Also wrap if the value form is comprised of

I don't think we should say 'for usability' here, we should try to explain what the visual difference is or similar, or maybe just remove the 'for usability' bit - git blame will lead people to this issue for justification.

> the change only affects exposed filters with exposed operators

I don't think so, the change also affect cases where there is no exposed operator.
As example, if you use daterange fields, and choose the "Is between" operator the label look like this now:

And after applying patch in #76 it looks like this:

I am setting this back to NW because of the discussion in #96 and #97. I will update the remaining tasks in the issue summary.

More accurate title, minor edits to the summary (we already have all the screenshots we need, right?).

Any resolution in sight for this bug?

What are the chances of a fix being included in the next version?

I fixed the comment requested by @catch in #96.

I've re-rolled clean patches for 8.9.x, 8.8.x and 8.7.x. The 8.9.x and 8.8.x patches are identical, but for clarity I'm naming them and attaching them separately (to keep testbot, committers, and anyone applying these via composer on the right track).

I've re-reviewed the summary and I think it's totally clear. I'm not sure what else we can do to improve the description of what this is changing. Removing that tag.

Interdiff is confused, so I'm attaching a raw diff of #76 vs. the 8.7.x patch in here.

This is blocking #2648950: [PP-2] Use form element of type date instead textfield when selecting a date in an exposed filter, so I'd love to see it land soon. Ideally before 8.8.0-beta1. Not sure if it's a candidate for an 8.7.x backport.

Thanks!
-Derek

Bot is fully happy, as expected.
I also updated the CR: https://www.drupal.org/node/2984152
Added an after screenshot, clarified when the wrapper is added, fixed some typos + grammar errors, etc. I think it's ready to publish.

RTBC?

Thanks!
-Derek

p.s. Yes, this is really what core looks like without this fix:

Tempted to promote this to 'major UI bug', but I think that'd violate the priority guidelines:

Bugs that affect one piece of functionality and are self-contained are normal priority.
They do not impact the overall functionality of the software.
Examples of normal bugs:

• Bugs for site visitors that do not interfere with site use, for example, visual layout issues.

Anyway, certainly "important", if not officially "major". ;)

I wasted too much time fixing this on a client project. The patch looks clean & to the point, tests look good, change notice is also clear and mentions what needed to be done to update the code to hide #date_time_element so it is RTBC for me.

Yeah, I wouldn't say it is major but is certainly annoying. :D Thank you for picking it up.

Thanks for the review and RTBC!

Slightly better title to hopefully further clarify.

Updates to the UI changes section of the summary to be even more explicit.

Hope this lands soon... :)

Thanks,
-Derek

Great work on this! Important fix so hate to un-RTBC this but got a couple of things I think we need to answer:

1. +++ b/core/modules/views_ui/tests/src/Functional/FilterNumericWebTest.php
   @@ -108,11 +108,13 @@ public function testFilterNumericUI() {
   +    $this->assertSession()->responseContains('<label for="edit-age-max">Max</label>');
   +    $this->assertSession()->responseContains('<label for="edit-age-min">Min</label>');
   

   Can we make the test coverage so that it actually tests that the min/max elements are nested inside the wrapper and not just test that they are on the page?

2. +++ b/core/modules/views_ui/tests/src/Functional/FilterNumericWebTest.php
   @@ -108,11 +108,13 @@ public function testFilterNumericUI() {
   +    self::assertEquals(trim($this->cssSelect('#edit-age-wrapper--description')[0]->getText()), 'Description of the exposed filter');
   

   Core doesn't do self::assertEquals but $this->assertEquals (long story)

Do we need test coverage to assert that the wrapper isn't shown on non-two-value exposed filters? Or do we have existing coverage for that? did somebody check?

@Lendude Thanks for the review! Re: #110

1. Yes, probably. ;) Can probably be done pretty easily via xpath selectors.

2. Yeah, I sorta knew that. Any chance there's a link to that long story somewhere so I can understand it better? Regardless, easy to fix.

3.

Do we need test coverage to assert that the wrapper isn't shown on non-two-value exposed filters?

Probably. ;)

Or do we have existing coverage for that? did somebody check?

Dunno, didn't check. ;) I'll look now.

I'll take a stab at all of the above, stay tuned.

Cheers,
-Derek

This fixes everything from #110. Sorry for the delays, I needed to break for lunch. ;)

RTBC?

Thanks!
-Derek

p.s. To be explicit: This patch is adding the fieldset, so I don't see how there could be existing test coverage related to the fieldset anywhere else in core. Therefore, since there was no coverage of the "shouldn't need a fieldset" case in #103, it seems safe to assume there was no coverage of that. Hence, I added some to the bottom of the test case. ;)

p.p.s. It *does* seem kinda weird that we're testing all this inside a views_ui test, relying on the 'Update preview' button, etc, but a) this test already lived there and needed to be updated to change the labels it was looking for and b) seems better to just finish testing everything related to the labels + (conditional) fieldsets in the same test, instead of adding a whole new Functional test in core/modules/views/tests/... for this. Refactoring all these filter-related views tests is totally out of scope for this issue, so I'm just updating/fixing/expanding the existing test method, even though I don't personally think what's in #112 is ideal.

Thanks,
-Derek

The new test coverage looks great. Back to RTBC as #110 is addressed.

+++ b/core/modules/views_ui/tests/src/Functional/FilterNumericWebTest.php
@@ -108,11 +108,34 @@ public function testFilterNumericUI() {
+    $this->assertTrue(!empty($min_element_label));

Was thinking about these new test assertions, and to be even more careful/safe, assertCount()would be better. That would save us from a bug that introduced duplicate labels or something...

Super trivial change. I'm going to leave this as RTBC (but will queue the bot for full testing).

@dww nice!

The self:: vs $this discussion can be found here https://github.com/sebastianbergmann/phpunit/issues/1914

@Lendude Thanks for the link! Started skimming, but need to do other things right now. I'll read in detail ASAP.

Since this is blocking other issues, also tagging for "blocker".

Did a final pass to edit the CR for clarity:
Exposed numeric/date views filters with multiple elements now have a wrapper, update form structure

https://www.drupal.org/node/2984152/revisions/view/11619541/11622827

Currently written as if this will be backported to 8.7.x, which might not happen. So when that's published, be sure to get the version + branch right based on where this lands.

Thanks, everyone!
-Derek

I'm a bit concerned about the potential disruption for existing sites. I'm wondering if someone could be relying on the current behavior. We probably should at least document on the change record how someone would get the old behavior back if that's what they want.

Yeah, this change is indeed quite disruptive. But core's current behavior is so broken, we have to do something. If our policy ends up saying "people are probably already working around this bug, so we can't fix it", that seems really sad. So, we wait for a "major" version upgrade to fix it. But then we find out that our "major" version is also supposed to be "BC" + "non-disruptive" (except for removing dead code that wouldn't break anything if you've been keeping up with all the API changes in all the "minor" releases), so we can't fix it there, either. ;) Then us bug hunters despair at the state of Drupal and feel like we're trying to tame a wild beast with 2 hands and foot tied behind our backs... ¯\_(ツ)_/¯

I can't think of a particularly clean/easy way for sites to get the mess back and remove the fieldsets. This markup is deep inside views/src/Plugin/views/filter/FilterPluginBase.php and there's no twig template involved. Can the CR suggest that sites that want to restore the bug can apply the patch in reverse to re-break it? ;)

Also, I've learned from Andrew at #3078334: Datetime and Datelist elements should render as fieldsets that fieldset is the preferred solution for grouping related form items together for accessibility reasons. So we could probably consider this an accessibility improvement bugfix and relax our BC standards in this case. Tagging for an accessibility review. I just re-read the summary and I think it's all accurate for reviewing the bug + proposed resolution. Given how absurd the current markup is for sighted users, I can only imaging how confusing this would be for someone using a screen reader. :(

I'd be really sad to have to write a BC layer, an opt-in new setting to get a working UI, an upgrade path to set existing sites to "use-broken-UI-for-exposed-forms = TRUE", and upgrade tests, all so that folks that already fixed this bug don't have to let core fix it for them for real.

And, I freely admit this is disruptive, changes the form structure, custom form_alter on your exposed filters will stop working, etc, etc. We're totally in a bind. I feel your pain about wanting to minimize disruption to existing sites.

I'm going to wait to edit the CR until we get clarity on what we're recommending (if anything) for sites to do to restore the broken UI, and if this will be fixed in 8.9.x, 9.0.x, 10.x, never, etc. ;)

Cheers,
-Derek

I'm a bit concerned about the potential disruption for existing sites.

Fortunately, any site currently with a workaround should be using this patch, so the disruption would be minimal hopefully.

Hey dww, I have applied the patch on a local VM and I see no changes to my text view I made... willing to troubleshoot with you.
Here's a YouTube video of my screen recording.

I have tested core on a local virtual machine and confirmed there are several accessibility, and indeed UI/UX, issues when utilizing an exposed filter with exposed operators for date selection for the 'Authored on' and 'Published on' field filters. Specifically, there is no label generated at all in the unpatched scenario, and there is little to no indication that the date field is related to the filter. This patch from user dww addresses these items. Most helpful in this patch is that it places all the div elements of each filter in their own "fieldset" container and provides the label for the filter as a "legend". Using a screen reader after the patch was installed locally it is much better when navigating with the keyboard (tab and arrow keys). When using keyboard navigation the order is much more logical and all fields can be discerned better than before. I would encourage this patch to be adopted and further enhancements for accessibility and UI/UX to be made by the community. Specific enhancements that could be made would be a help text field as well as the description. An example of very good use of help text and descriptions can be found in the Webform module.

Thanks for the review @jweakley! Based on our Slack chat about this, I added a Key UI/UX/Accessibility bugs fixed by this issue section to the summary. Also, escalating this bug to major, since based on your review, the existing UI is basically unusable for folks using assistive technologies.

Still wondering what exactly I can/should put in the CR to address "how someone would get the old behavior back if that's what they want." per @lauriii in #118. Based on how this fix works, restoring the broken/inaccessible behavior is not easy, short of patch -p1 -R < 2625136-115.8_8_x.patch. ;)

Not sure if we can remove the 'Needs accessibility review' tag, yet. Perhaps one of the more "official" accessibility maintainers for core should comment here, too...

Thanks,
-Derek

p.s. Crediting @jweakley for all the effort spent reviewing/testing this for accessibility.

Hi,

I have checked the patch #2625136-115.8_8_x.

There are some problems with Bartik and the claro theme.

Below is an output screenshot before applying the patch for Bartik, Seven and Claro.

Below is an output screenshot after applying the patch for Bartik, Seven and Claro.

Thanks,
Ren

@rensingh99 re: #124:

A) Can you clarify what you think the problem is? Those screenshots look exactly as expected. A red box doesn't communicate your concerns. ;)

Are you worried that the fieldsets aren't pixel-perfectly styled? If anything, the Claro screenshot seems worse than the other two on that front. If we need to *also* add CSS tweaks to all the core themes to try to make this look "perfect", that seems a bit scope-creepy to me (and yet more potential to disrupt existing sites). In fairness, Claro didn't even exist when this issue and fix was written, and Claro is still experimental, so IMHO fixing Claro should be a follow-up issue in the Claro queue, not here. But perhaps the core committers would be more comfortable if we do all that work as part of this issue. TBD.

B) Your comment says "Below is an output screenshot before applying the patch for Bartik, Seven and Claro." twice. The 2nd was for after the patch, no? Maybe you want to edit your comment to avoid confusion?

Hi @dww

Thank you for your comment.

A) In Bartik, the number text field is not inline with the operator field. it's coming with some space(about 2px).

And in claro yes fieldset is not pixel-perfectly styled.

B) I have edited the comment #124.

Thanks,
Ren

I am using the patch on several sites and this works very well.
Everything that is not 100% perfect is fixed with a little css.
For me this is RTBC to prevent this to drag om forever.

A) As an accessibility bugfix, I think this should still be backported at least to 8.9.x. I suspect that given the potential disruption, it's not going to be landing in 8.8.x, but there's still time to fix 8.9.x and up. Good news is that the same patch applies to all the active branches now.

B) Re: Claro -- I think that needs to be a follow-up. Claro is still experimental, and a work-in-progress. There's a whole process to get designs approved. I don't think we should delay this fix while that unfolds.

C) Re: Bartik -- the spacing between text fields and selects is already a bit wonky, without this patch. That's nothing new. I don't think it makes sense to do a "pixel perfect" fix for this one case, and fixing it in general is way out of scope here. People who care about that have already fixed it for themselves. The form elements all have identical margins. The only difference is the height of the elements. Here's a screenshot of the /admin/content page on a clean 8.8.x install with Bartik set as the admin theme. You can see the same height differences:

D) Test fail on #115 for 8.8.x was a random fail:

There was 1 error:

1) Drupal\Tests\field_ui\FunctionalJavascript\ManageDisplayTest::testWidgetUI
WebDriver\Exception\CurlExec: Curl error thrown for http POST to http://chromedriver-jenkins-drupal-patches-43824:9515/session/2e90493961ddfbd2724749f8c87b00a0/execute with params: {"script":"(function (element) {\n    var event = document.createEvent(\"HTMLEvents\");\n\n    event.initEvent(\"drop\", true, true);\n    event.dataTransfer = {};\n\n    element.dispatchEvent(event);\n}(arguments[0]));","args":[{"ELEMENT":"0.6472767889402347-5"}]}

E) Inspired by #3128573: [meta] Replace assertions with more appropriate ones decided to fix one of the test assertions to be more appropriate. ;) See interdiff. However, since it's a reroll with a (very minor) change, I'm setting back to NR.

F) @jweakley Already spent a bunch of time reviewing this from an accessibility angle, so I'm removing the tag. This has had an accessibility review. The fix here is way more accessible than what we have in core right now.

Thanks,
-Derek

Makes sense to me, and code looks good. Only found this nit, which can be fixed on commit or ignored.

+++ b/core/modules/views_ui/tests/src/Functional/FilterNumericWebTest.php
@@ -113,11 +113,34 @@ public function testFilterNumericUI() {
+    $this->drupalPostForm(NULL, $edit, t('Apply'));
+    $this->drupalPostForm('admin/structure/views/view/test_view', [], t('Save'));
...
+    $this->drupalPostForm(NULL, [], t('Update preview'));

To my knowledge there is no need to use t() in tests, unless we're testing translations.

Re: #131: Thanks for the review! Re: nit -- that entire test is full of t().

A) Out of scope to fix it all here.
B) Seems better to remain consistent with the rest of the test than to only "fix" the parts of that test that are in scope for this bug fix.
C) We should either find (proving difficult) or open an issue about removing t() from tests that don't need them.

Thanks again,
-Derek

Re: #132.C: According to @longwave, that should be a child of #3113904: [META] Replace t() calls inside of classes ...

$ find core -type f -iname '*test*' | xargs grep '[^a-zA-Z]t(' | wc -l
4913

not 100% accurate but there are roughly 5000 instances of this to fix up

Re: #130.B and Claro. Preemptively opened #3133639: Fix Claro styles for exposed views filters wrapped in fieldsets so no one delays committing this for "Needs follow-up". ;)

RE #32 ah ok, yup, I agree on keeping the file consistent.
And yes we definitely should clean that up... if only to have core lead with the best practice. I've opened #3133726: [meta] Remove usage of t() in tests not testing translation for this.

We discussed this issue at the #3133883: Drupal Usability Meeting 2020-05-12.

There was some concern about how this will affect Claro, since Claro already puts all the exposed filters in a fieldset and nested fieldsets may not be the best solution. We agreed that any problems should be addressed in Claro instead of holding up this issue. I was planning to add a follow-up issue, but I see that was already done in #134. Thanks for that. And the issue already has secreenshots. @dww++.

Aside (numerology)

Since I use a Yellow Pig as my avatar, I have to comment when my favorite number, 17, come up. In #133 I see 4913, which is the cube of 17 and also the sum of its digits is 17.

@larowlan pinged me on slack for my opinion about the BC implications, so my 2 cents....

I feel we are in the clear here from a strict BC policy stand point, per https://www.drupal.org/core/d8-frontend-bc-policy :

# Render arrays (including form arrays)
While the Render and Form APIs themselves are stable, the exposed data structures used to build and cache pages and forms are not. We will change these structures where necessary to make usability improvements or add features in minor versions. This means alter hook implementations may need to be updated.

But from personal experience, Views exposed filters tend to get altered a lot more than most other render arrays, so I do share @larowlan's concern about BC here.

I like the idea of trying to do some sort of BC layer for render arrays, but personally don't feel we should hold anything up waiting for that, the policy as it stands seems pretty clear.

Thanks @Lendude
So on that basis I think we have consensus that we're going to need to do this, and the earlier the better. But we're also going to need to communicate the change here in a release-note snippet, as this is something people are going to be aware of in updating.

Can we get a release-note snippet added to the issue summary? Setting to NR for that. Please return to RTBC when added.

Added a draft release note.

I did a little copy editing of the release-note snippet. We are allowed more than one paragraph, aren't we? I am not sure I chose the best place to break the paragraphs. Maybe we should make it three paragraphs.

Thanks for the edits, @benjifisher, those look great to me. The guidelines say to be succinct, so I wouldn't expand it into 3 paragraphs if we can help it. If we need to say anything else, IMHO it should go into the CR, not this note.

Cheers,
-Derek

I was thinking of adding more linebreaks, not more words.

+++ b/core/modules/views/src/Plugin/views/filter/NumericFilter.php
@@ -277,17 +277,16 @@ protected function valueForm(&$form, FormStateInterface $form_state) {
-        '#title' => !$exposed ? $this->t('Min') : $this->exposedInfo()['label'],
+        '#title' => $this->t('Min'),
...
-        '#description' => !$exposed ? '' : $this->exposedInfo()['description'],

So we lose the ability to configure the label for a min, and a description - is that intentional?
This was flagged in #71 but I don't see it addressed.

#78 mentions that #76 addressed #71 but I don't see that in the interdiff at #76

Thinking some more about the disruption here - could we make this new behaviour opt-in via a state or configuration flag?

We could default it to TRUE for new sites, but make it an explicit decision for existing sites so that the act of enabling it could be accompanied by an audit of their existing views exposed forms, alter hooks and theme templates?

There is some precedence for making this opt-in, views.settings.ui.exposed_filter_any_label has options 'old any' and 'new any'

So we could add a config option to make it opt-in.

Re: #145 / #71:

1.

Are we sure about losing the ability to customise this?

The current behavior is IMHO incredibly weird with Between + Min/Max. We use the main (configurable) filter label for "Min", and force "And" as the label for Max (!?!). The operator always says "Operator", even though that's the first element you encounter. If you have multiple numeric/date filters exposed on the same form, it's even more WTFy. This is part of why this is a major accessibility bug (not to mention a major sighted-UI WTF).

We're not losing the ability to customize the filter label. Instead, we now *always* see it (huge win) and it's the label for the entire fieldset. That way everyone, sighted or not, can tell "all these elements belong to a filter called "Whatever you customized it to be". Then it's clear what "Operator", "Min" and "Max" (all hard-coded, unless we proceed with #3120627: [PP-1] Make views exposed filter operator labels configurable after this is done), are all referring to, and we never have a form element labeled "And".

2.

This label is set up above - $edit['options[expose][label]'] = 'Age between'; what happens to it now?

The filter label is now the fieldset legend:

+    $this->assertSession()->elementTextContains('css', 'fieldset#edit-age-wrapper', 'Age between');

Probably that assertion should be more specific so we know we're finding the label in the fieldset legend. Attached patch does so and the test passes locally.

To answer both questions with some pictures:

Before

Single exposed numeric/date filter

Two exposed numeric/date filters

After

Re: #145:

could we make this new behaviour opt-in via a state or configuration flag?

If we have to, I guess we could. As I wrote at #119, I truly don't like that idea:

I'd be really sad to have to write a BC layer, an opt-in new setting to get a working UI, an upgrade path to set existing sites to "use-broken-UI-for-exposed-forms = TRUE", and upgrade tests, all so that folks that already fixed this bug don't have to let core fix it for them for real.

Looking again at those before screenshots, I can't fathom why anyone wants/needs that behavior. Even if we jump through all these hoops for a "use the legacy broken filter UI", I don't think this is going to be considered patch-eligible, so why bother? Can't we just have a clean, minor-only fix, without technical debt and hassles, make folks stop working around this totally broken UI and let core fix it for them?

If we do this minor only with a change record, then it's possible that contrib or custom code altering the form array might break - however that's an allowable change in a minor release (especially to fix a bug).

If we add a bc layer:

1. If it defaults to 'on', then sites without the alters also won't get the bug fix unless they know where to find and disable the bc layer.

2. If it defaults to 'off', then all the sites that would have broken due to alters will still break, they'll just have the option to switch the bc layer on

3. Any contrib code altering the render array will still need to be updated for both the 'on' and 'off' case, but instead of only needing to support both cases until 9.0/8.9 is out of support, they'll need to support them until Drupal 10 when we remove the bc layer.

edited to add:
There's also a third option of delaying the bugfix until Drupal 10 - where we can unapologetically break bc. However, this would introduce an inconsistency between 9.LTS.x and 10.0.x, whereas we tried to keep 8.9 and 9.0 as similar as possible with the exception of major-only changes. Also contrib would have to support both versions for longer.

IMHO, 9.x is still so fresh that the BC layer would really be a bit over-the-top. If you are currently creating sites with 9.x, fact is that you probably run into all sorts of minor problems. In a real-life world, you also use contributed modules, some of them even need patches for D9, you have minor glitches literally everywhere. At least that's what I'm currently experiencing, building *actual* D9 sites.

Some form_alter hook not working anymore due to some changing elements is sth. you encounter all the time in Drupal one way or another - it's a rather easy problem to solve for someone who is using form_alter hooks in the first place. There is *much* worse that can happen to your site. I think it should be sufficient to just change it and publish a change record.

+1 for doing this in 9.1.0 with a CR and no BC layer. See the first paragraph of #148.

Great, that's 3 votes in a row for committing #147 as-is, without technical debt. Anyone willing to RTBC this? 😉 🤞

Thanks!
-Derek

We are already using three patch combo so this is ready in my books. Thanks, for all the work on this @dww.

Saving issue credits

Ok, on the basis of discussion above and comments from @catch - Committed 9fdb203 and pushed to 9.1.x. Thanks!

Our approach here is as follows:

- document the change in the release notes
- change notice

This is a tricky one, so I think doing it now rather than later in the 9.1 cycle maximizes our chances of finding issues.

Published the change record and decremented the PP count on the postponed issue.

This obviously won't be backported, for disruption reasons.