Views top dropbutton breaks when editing a view

Added a test for the Views UI Menu Tabs. (It does not tests the currently broken dropbutton.)

The test tries to verify that the views-tabs and the views-tabs--secondary CSS classes can be found on the view ui menu tabs element even before and after an AJAX refresh.

Removed some redundancy.

Technically I only applied the patch that I got from @lauriii.

#13 added a new test case for the extra actions dropbutton.

From @lauriii:

you need to make some docs fixes in the patch at least

the patch I sent is not complete

He's totally right.

1. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Theme/ClaroViewsUiTest.php
   @@ -0,0 +1,125 @@
   +   * Install the shortcut module so that claro.settings has its schema checked.
   +   * There's currently no way for Claro to provide a default and have valid
   +   * configuration as themes cannot react to a module install.
   

   This documentation seems unrelated.

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Theme/ClaroViewsUiTest.php
   @@ -0,0 +1,125 @@
   +    // Install Claro and set it as admin theme.
   +    $this->container->get('theme_installer')->install(['claro']);
   +    $this->config('system.theme')->set('admin', 'claro')->save();
   

   Are we able to use the new $defaultTheme property for setting Claro as the testing theme?

This addresses #17.

Nice work! I began by reading the test-only patch, and found some things that should probably be changed to make it more reliable and less verbose. 🤓

1. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Theme/ClaroViewsUiTest.php
   @@ -0,0 +1,121 @@
   +    $display_menu_tabs_css_selector = '#views-display-menu-tabs';
   +    $page = $this->getSession()->getPage();
   +    $display_menu_tabs = $page->find('css', $display_menu_tabs_css_selector);
   +    $this->assertNotEmpty($display_menu_tabs);
   +    $this->assertTrue($display_menu_tabs->hasClass('views-tabs'));
   +    $this->assertTrue($display_menu_tabs->hasClass('views-tabs--secondary'));
   

   This can basically be all one line:

   $display_menu_tabs = $this->assertSession()->elementExists('css', '#views-display-menu-tabs.views-tabs.views-tabs--secondary');
   

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Theme/ClaroViewsUiTest.php
   @@ -0,0 +1,121 @@
   +    $display_name_link = $page->find('css', '#edit-display-settings-top .views-display-setting a');
   +    $this->assertNotEmpty($display_name_link);
   +    $display_name_link->click();
   

   This could also be one line, since WebAssert::elementExists() returns the element in question:

   $this->assertSession()->elementExists('css', '#edit-display-settings-top .views-display-setting a')->click();
   

3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Theme/ClaroViewsUiTest.php
   @@ -0,0 +1,121 @@
   +    $this->assertSession()->waitForElement('css', '.js-views-ui-dialog');
   

   This is an incorrect use of waitForElement(). It returns a boolean, but does not actually assert that the element appeared. So, this needs to be:

   $this->assertTrue($this->assertSession()->waitForElement('css', '.js-views-ui-dialog'));

4. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Theme/ClaroViewsUiTest.php
   @@ -0,0 +1,121 @@
   +    $apply_button = $page->find('css', '.js-views-ui-dialog .ui-dialog-buttonpane')->findButton('Apply');
   +    $this->assertNotEmpty($apply_button);
   +    $apply_button->press();
   

   Again, this can be more assertive as a single line:

   $this->assertSession()->elementExists('css', '.js-views-ui-dialog .ui-dialog-buttonpane')->pressButton('Apply');

5. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Theme/ClaroViewsUiTest.php
   @@ -0,0 +1,121 @@
   +    // Wait for AJAX to finish.
   +    $this->assertSession()->assertWaitOnAjaxRequest();
   

   assertWaitOnAjaxRequest() is bad news and should not be relied upon. However, I see that in this case we probably will not be able to do anything else, so no need to change this. But in general, one should wait on the expected condition, rather than the more nebulous "an AJAX request has finished".

This should address all feedback from #23 😇

The split of claro_form_view_edit_form_alter() into the two new hooks looks great.

In the first, $form['displays']['top'] is replaced with $element, no other changes.
In the second, $form['displays']['settings']['settings_content']['tab_content'] is replaced with $element, no other changes.

1. +++ b/core/themes/claro/claro.theme
   @@ -521,69 +521,72 @@ function claro_form_media_form_alter(&$form, FormStateInterface $form_state) {
   +function claro_views_ui_display_top_alter(&$element) {
   ...
   +function claro_views_ui_display_tab_alter(&$element) {
   

   Technically that function signature should read &$build, ViewUI $view, $display_id

   I don't know that it matters, thought I'd call it out.

2. +++ b/core/themes/claro/claro.theme
   @@ -521,69 +521,72 @@ function claro_form_media_form_alter(&$form, FormStateInterface $form_state) {
   +  // We process the dropbutton-like element on views edit form's
   +  // display settings top section.
   +  //
   +  // That element should be a regular Dropbutton.
   +  //
   +  // After that the reported issue is fixed and the element is rendered with
   +  // the Dropbutton type, we just have to set it's '#dropbutton_type' to
   +  // 'extrasmall'.
   +  //
   +  // @todo: revisit after https://www.drupal.org/node/3057577 is fixed.
   

   I've never seen a docblock with newlines like that, but phpcs isn't flagging it, so I guess it's okay.

Adding reviewer credit.

Pushed to 9.0.x and 8.9.x.

If someone wants to take up either or both of the items in #26 in a followup, go for it.

I'll open a separate issue for cherry picking all the 8.9 Claro related commits to 8.8.

Pushed to 8.8.x. See #3088437: Cherry pick Claro to Drupal 8.8. for details.