No empty option for views bulk form

Applied MR 2727 on 9.5.x.The empty option appears in node bulk import form after applying this patch.

Before patch:

After patch:

Uploading a tests-only patch and full patch that is an exact copy of the MR at this commit 81f044f9

Just trying to see the red/green pattern and didn't want to upload just a tests-only patch and have drupal mark it NW.

Reviewing the code

  protected function emptyActionMessage() {
    return $this->t('No action selected.');
  }

Is it possible to make this use the label of the field? For example if I change my dropdown to say "Operations" the expected error message I would think should say "No Operation selected".

Also think we need a test case for the error message.

Also since this is a UX change tagging for Usability check.

Since screenshots were provided in #8 that tag is not needed

Additional screenshots are not needed.

Thanks for the quick turnaround.

See the changes from #9 was addressed.

Marking as RTBC but will still need a usability review.

Very nice, +1 from me.

Believe https://www.drupal.org/project/drupal/issues/2381293 and https://www.drupal.org/project/media_entity/issues/2855233 can be closed in place of this.

Unfortunately the latest change on the RTBC does not work in languages that have different rules for capitalisation. Perhaps we should go back to #9. This type of stuff is really hard.

@J-Lee I like that idea. Could make that change?

Updated message around #16

I like the placeholder idea - but think the implementation should be tweaked. See review in gitlab,

Verified and tested #9 Patch.
Patch applied successfully and able see to the expected result.
Empty option appears in the node bulk import form after applying this patch.

@Saman Malik people are working on the merge request in gitlab. Re-uploading the patch from #9 is disruptive to that work. Yes in #15 I suggested going back to it but then things moved on and another idea was proposed. See #19... and all the commits on the MR. Hiding all the files so the MR is the only thing the shows.

We need a review of the MR's current state.

no worries, I think you need to update the 10.1.x branch from the fork to the latest commit. Otherwise it's possible there will be lots of noise in the merge request.

Applied patch #22 on drupal version 10.1.x successfully and working fine,
Refer to screenshot.

Thankyou

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.

Testing #27 as the MR for 10.1 seems empty. but believe #27 is what was intended for the MR. I see all the threads for the 9.5.x MR were resolved before #27 was made. Odd order but believe everything is there!

Testing the code on
Drupal 10.1.x on a standard install
Created a piece of content to make sure the bulk operation appears
Verify I see an empty option.

I closed both merge requests for clarity after verifying that they were identical to the patch.

I tested manually and confirmed the new empty option is available on the content, comment, and user bulk forms all work as expected, and that there is an error message that "No $foo were selected" If I try to submit the form without checking anything.

IMO this is a major usability bug, so promoting and retitling accordingly

+++ b/core/modules/views/src/Plugin/views/field/BulkForm.php
@@ -441,6 +442,16 @@ protected function emptySelectedMessage() {
+    return $this->t('No %title option selected.', ['%title' => $this->options['action_title']]);

Since this is passing the title into a translated string, I manually tested it in Hindi:

Since it is a new string and not translated yet, I added a custom translation:

That seems OK.

1. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaOverviewTest.php
   @@ -108,6 +108,10 @@ public function testAdministrationPage() {
   +    // Get Action select field and set the media_delete_action.
   
   +++ b/core/modules/views/tests/src/Functional/Plugin/ViewsBulkTest.php
   @@ -64,6 +64,11 @@ public function testBulkSelection() {
   +    // Get Action select field and set the node_delete_action.
   
   @@ -82,6 +87,9 @@ public function testBulkSelection() {
   +    // Select the node_delete_action from the Action field.
   

   We don't need to be quite this... literal with issue comments. "Select the node deletion action" would be sufficient for both these comments. The code explains the rest.

2. +++ b/core/modules/views/tests/src/Functional/BulkFormTest.php
   @@ -143,6 +148,10 @@ public function testBulkForm() {
   +    // Test if an error message appears with a different title if no action is selected.
   

   This comment should wrap at 80 characters.

3. +++ b/core/modules/views/src/Plugin/views/field/BulkForm.php
   @@ -441,6 +442,16 @@ protected function emptySelectedMessage() {
   +  protected function emptyActionMessage() {
   

   This is a new method and as such should have a return typehint.

Since the comment formatting issues are relatively minor, I changed them on commit with the following diff:

diff --git a/core/modules/media_library/tests/src/FunctionalJavascript/MediaOverviewTest.php b/core/modules/media_library/tests/src/FunctionalJavascript/MediaOverviewTest.php
index 974a9f993d..c7bdb44d3a 100644
--- a/core/modules/media_library/tests/src/FunctionalJavascript/MediaOverviewTest.php
+++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaOverviewTest.php
@@ -108,7 +108,7 @@ public function testAdministrationPage() {
     $assert_session->elementExists('css', '#views-exposed-form-media-library-page')->submit();
     $this->waitForText('Dog');
 
-    // Get Action select field and set the media_delete_action.
+    // Select the "Delete media" action.
     $page->selectFieldOption('Action', 'Delete media');
     $this->waitForText('Dog');
 
diff --git a/core/modules/views/tests/src/Functional/BulkFormTest.php b/core/modules/views/tests/src/Functional/BulkFormTest.php
index 4e879e558c..0f4c53d7be 100644
--- a/core/modules/views/tests/src/Functional/BulkFormTest.php
+++ b/core/modules/views/tests/src/Functional/BulkFormTest.php
@@ -134,7 +134,7 @@ public function testBulkForm() {
     $this->drupalGet('test_bulk_form');
     $this->assertSession()->elementTextEquals('xpath', '//label[@for="edit-action"]', 'Action');
 
-    // Test if an error message appears if no action is selected.
+    // There should be an error message if no action is selected.
     $edit = ['node_bulk_form[0]' => TRUE, 'action' => ''];
     $this->submitForm($edit, 'Apply to selected items');
     $this->assertSession()->pageTextContains('No Action option selected.');
@@ -148,7 +148,8 @@ public function testBulkForm() {
     $this->drupalGet('test_bulk_form');
     $this->assertSession()->elementTextEquals('xpath', '//label[@for="edit-action"]', 'Test title');
 
-    // Test if an error message appears with a different title if no action is selected.
+    // The error message when no action is selected should reflect the new form
+    // title.
     $this->submitForm($edit, 'Apply to selected items');
     $this->assertSession()->pageTextContains('No Test title option selected.');
 
diff --git a/core/modules/views/tests/src/Functional/Plugin/ViewsBulkTest.php b/core/modules/views/tests/src/Functional/Plugin/ViewsBulkTest.php
index 4e17374d67..c37b5f4090 100644
--- a/core/modules/views/tests/src/Functional/Plugin/ViewsBulkTest.php
+++ b/core/modules/views/tests/src/Functional/Plugin/ViewsBulkTest.php
@@ -64,7 +64,7 @@ public function testBulkSelection() {
       'changed' => \Drupal::time()->getRequestTime() - 120,
     ]);
 
-    // Get Action select field and set the node_delete_action.
+    // Select the node deletion action.
     $action_select = $this->getSession()->getPage()->findField('edit-action');
     $action_select_name = $action_select->getAttribute('name');
     $this->getSession()->getPage()->selectFieldOption($action_select_name, 'node_delete_action');
@@ -87,7 +87,7 @@ public function testBulkSelection() {
       'title' => 'The third node',
     ]);
 
-    // Select the node_delete_action from the Action field.
+    // Select the node deletion action.
     $this->getSession()->getPage()->selectFieldOption($action_select_name, 'node_delete_action');
 
     // Now click 'Apply to selected items' and assert the second node is

Adding credit for @Abhijith S for manual testing, @J-Lee for work on the patch and MR, @alexpott for review, and @smustgrave for review, testing, triage, and taking the time to provide a test-only patch. (@gaurav-mathur repeated manual testing that had already been done by @Abhijith S, so I have not credited that.)

Committed and pushed to 10.1.x. Since it is a user interface change that could affect views and form alters etc. on existing sites, it is a minor-only fix I did not backport it to 10.0.x and 9.5.x.

diff --git a/core/modules/views/src/Plugin/views/field/BulkForm.php b/core/modules/views/src/Plugin/views/field/BulkForm.php
index 2edd801598..5f02efc457 100644
--- a/core/modules/views/src/Plugin/views/field/BulkForm.php
+++ b/core/modules/views/src/Plugin/views/field/BulkForm.php
@@ -448,7 +448,7 @@ protected function emptySelectedMessage() {
    * @return string
    *   Message displayed when no action is selected.
    */
-  protected function emptyActionMessage() {
+  protected function emptyActionMessage(): string {
     return $this->t('No %title option selected.', ['%title' => $this->options['action_title']]);

Actually I realized this doesn't return a string; it returns a TranslatableMarkup. Followup incoming. This is also wrong in another method on the same class.