Problem/Motivation

Discovered while investigating this bug in Claro #3067085: Views top dropbutton breaks when editing a view. The cause was narrowed down to the use of form alter for the use of hook_form_view_edit_form_alter(). On the initial load, the UI is rendered with the alterations, but since the ajax re-generated markup isn't generated using form API, the alter hook isn't triggered. This is documented in Drupal\views_ui\ViewEditForm and there's already hook_views_ui_display_tab_alter to work around this problem. The hook isn't being triggered in themes so it cannot be used by Claro.

Proposed resolution

Trigger hook_views_ui_display_tab_alter in themes as well so that Claro and other themes styling Views UI can use it. Since this is only for the tab contents, we have to also add a new alter hook hook_views_ui_display_top_alter for altering the top region.

Remaining tasks

  1. Get confirmation this is not a Claro issue
  2. Review
  3. Commit.

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bnjmnm created an issue. See original summary.

bnjmnm’s picture

The test largely recreates the situation that surfaced the issue in Claro. It uses form_alter within a .theme module that instructs menu_local_task elements in the Views UI to use a custom template. This custom template should always be the one used, but when unpatched the default template is used on ajax rebuilds.

The last submitted patch, 2: 3067386-2-test-only.patch, failed testing. View results

ryankavalsky’s picture

Patch #2 works for me! Thank you!

Wim Leers’s picture

Title: Template suggestions for elements in the Views UI form not preserved on ajax rebuild » Template suggestions for elements in the Views UI form not preserved on AJAX rebuild
Issue summary: View changes
Related issues: +#3067085: Views top dropbutton breaks when editing a view
FileSize
648 bytes
  1. +++ b/core/modules/views_ui/src/ViewEditForm.php
    @@ -677,14 +693,16 @@ public function rebuildCurrentTab(ViewUI $view, AjaxResponse $response, $display
    +    $views_ui_controller = new ViewsUIController($this->viewsData);
    +    $build = $views_ui_controller->edit($view, $display_id);
     
         // Regenerate the main display area.
    -    $build = $this->getDisplayTab($view);
    -    $response->addCommand(new HtmlCommand('#views-tab-' . $display_id, $build));
    +    $main_display_area = $build['edit']['displays']['settings']['settings_content']['tab_content']['details'];
    +    $response->addCommand(new HtmlCommand('#views-tab-' . $display_id, $main_display_area));
     
    -    // Regenerate the top area so changes to display names and order will appear.
    -    $build = $this->renderDisplayTop($view);
    -    $response->addCommand(new ReplaceCommand('#views-display-top', $build));
    +    // Regenerate the top area for changes to display names and order.
    +    $views_display_top = $build['edit']['displays']['top'];
    +    $response->addCommand(new ReplaceCommand('#views-display-top', $views_display_top));
    
    +++ b/core/modules/views_ui/tests/themes/views_test_classy_subtheme/views_test_classy_subtheme.theme
    @@ -0,0 +1,19 @@
    +function views_test_classy_subtheme_form_view_edit_form_alter(&$form, FormStateInterface $form_state) {
    +  // Changing template to make sure suggestion persists on ajax rebuild.
    +  foreach (Element::children($form['displays']['top']['tabs']) as $tab) {
    +    $form['displays']['top']['tabs'][$tab]['#theme'] = 'menu_local_task__views_ui';
    +  }
    +}
     
    

    😲 This is a pretty big change.

    So time to dig into this! 🤓 I did manually reproduce the need for this by adding

    function seven_form_view_edit_form_alter(&$form, FormStateInterface $form_state) {
      // Changing template to make sure suggestion persists on ajax rebuild.
      foreach (\Drupal\Core\Render\Element::children($form['displays']['top']['tabs']) as $tab) {
        $form['displays']['top']['tabs'][$tab]['#theme'] = 'menu_local_task__views_ui';
      }
    }
    

    to seven.theme.

    The original page load contains this HTML:

    <!-- THEME DEBUG -->
    <!-- THEME HOOK: 'menu_local_task__views_ui' -->
    <!-- FILE NAME SUGGESTIONS:
       * menu-local-task--views-ui.html.twig
       x menu-local-task.html.twig
    -->
    <!-- BEGIN OUTPUT from 'core/themes/classy/templates/navigation/menu-local-task.html.twig' -->
    

    And after changing a filter in the view, I see this in the AJAX response:

    <!-- THEME DEBUG -->
    <!-- THEME HOOK: 'menu_local_task' -->
    <!-- BEGIN OUTPUT from 'core/themes/classy/templates/navigation/menu-local-task.html.twig' -->
    

    So evidently the current render logic in Views UI is losing form alterations. Which is also the analysis in the issue summary.

  2. +++ b/core/modules/views_ui/tests/themes/views_test_classy_subtheme/templates/navigation/menu-local-task.html.twig
    @@ -0,0 +1,27 @@
    +<li{{ attributes.addClass('tabs__tab', 'js-tab', is_active ? 'is-active', is_active ? 'js-active-tab') }}>
    +  {{ link }}
    +  {% if is_active and level == 'primary' %}
    +    <button class="reset-appearance tabs__trigger" aria-label="{{ 'Tabs display toggle'|t }}" data-drupal-nav-tabs-trigger>
    

    🤔 All this complexity can go away — we should make this just a <li> plus that <span> with text that we assert. The point is not the complexity of the template, but the fact that one or the other template is loaded. So let's keep the test as simple as possible to not distract from that :)

  3. +++ b/core/modules/views_ui/tests/themes/views_test_classy_subtheme/templates/navigation/menu-local-task.html.twig
    @@ -0,0 +1,27 @@
    +      {% include "@claro/../images/src/hamburger-menu.svg" %}
    

    🐛 This Claro-specific stuff should definitely not end up in core.

  4. +++ b/core/modules/views_ui/tests/themes/views_test_classy_subtheme/views_test_classy_subtheme.info.yml
    @@ -0,0 +1,6 @@
    +name: 'Theme test subtheme'
    ...
    +description: 'Test theme which uses test_basetheme as the base theme.'
    ...
    +base theme: classy
    

    🐛 This needs cleaning up :)

  5. +++ b/core/modules/views_ui/tests/themes/views_test_classy_subtheme/views_test_classy_subtheme.theme
    @@ -0,0 +1,19 @@
    +  // Changing template to make sure suggestion persists on ajax rebuild.
    +  foreach (Element::children($form['displays']['top']['tabs']) as $tab) {
    +    $form['displays']['top']['tabs'][$tab]['#theme'] = 'menu_local_task__views_ui';
    +  }
    

    🤔 AFAICT this is adding a theme suggestion, not merely ensuring it persists across AJAX rebuilds.

    Just the act of adding this should already result in it persisting across AJAX rebuilds, because otherwise, what's the point?

    IOW: this code looks good, but I think the comment needs to change.

  6. ⚠️ The only alternative solution is to not let themes add this template suggestion but to … let Views UI already add this template suggestion. That way, we do not fix the root cause bug, but we also change far less by introducing this super tiny new "feature". It doesn't seem contentious at all to do that. So perhaps that's the simpler and hence better approach? Patch attached.
lauriii’s picture

+1 to the proposed solution in #5. It's an easy fix and will unblock Claro. Let's open a follow-up to solve the actual issue that form alter changes doesn't persist after ajax makes changes to the page.

huzooka’s picture

I'm checking this.

huzooka’s picture

Status: Needs review » Needs work

After comparing the HTML output of #2 and #5, I realized that #5 changes the elements after a rebuild, and the attributes like 'data-drupal-selector', 'formnovalidate' and even the CSS ids of the input[type="submit"] elements aren't added to the output anymore.

I feel this a bit dangerous. And since, for Claro, we have to alter the wrapper element of these menu_local_tasks as well, and find a solution for the fake dropbuttons as well, #5 isn't enough there.

I think that we should fix the actual issue.

Views display top HTML tree, default output and the output after a rebuild with patch #2:

<div class="views-display-top clearfix" id="views-display-top" data-drupal-selector="edit-displays-top">
  <div class="dropbutton-wrapper dropbutton-multiple">
    <div class="dropbutton-widget">
      <ul id="views-display-extra-actions" data-drupal-selector="edit-displays-top-extra-actions" class="dropbutton">
        <li class="edit-details dropbutton-action">
          <a href="/admin/structure/views/nojs/edit-details/content/page_1" class="views-ajax-link">Edit view name/description</a>
        </li>
        <li class="dropbutton-toggle">
          <button type="button">
            <span class="dropbutton-arrow">
              <span class="visually-hidden">List additional actions</span>
            </span>
          </button>
        </li>
        <li class="analyze dropbutton-action secondary-action">
          <a href="/admin/structure/views/nojs/analyze/content/page_1" class="views-ajax-link">Analyze view</a>
        </li>
        <li class="duplicate dropbutton-action secondary-action">
          <a href="/admin/structure/views/view/content/duplicate">Duplicate view</a>
        </li>
        <li class="reorder dropbutton-action secondary-action">
          <a href="/admin/structure/views/nojs/reorder-displays/content/page_1" class="views-ajax-link">Reorder displays</a>
        </li>
        <li class="delete dropbutton-action secondary-action">
          <a href="/admin/structure/views/view/content/delete">Delete view</a>
        </li>
      </ul>
    </div>
  </div>
  <h2 class="visually-hidden">Secondary tabs</h2>
  <ul id="views-display-menu-tabs" class="tabs secondary">
    <li data-drupal-selector="edit-displays-top-tabs-page-1" class="tabs__tab is-active">
      <a href="/admin/structure/views/view/content/edit/page_1" data-drupal-link-system-path="admin/structure/views/view/content/edit/page_1">Page<span class="visually-hidden">(active tab)</span></a>
    </li>
    <li class="add">
      <a href="#"><span class="icon add"></span>Add</a>
      <ul class="action-list" style="display:none;">
        <li class="first">
          <input class="add-display button js-form-submit form-submit" data-drupal-selector="edit-displays-top-add-display-attachment" formnovalidate="formnovalidate" type="submit" id="edit-displays-top-add-display-attachment" name="op" value="Attachment">
        </li>
        <li>
          <input class="add-display button js-form-submit form-submit" data-drupal-selector="edit-displays-top-add-display-block" formnovalidate="formnovalidate" type="submit" id="edit-displays-top-add-display-block" name="op" value="Block">
        </li>
        <li>
          <input class="add-display button js-form-submit form-submit" data-drupal-selector="edit-displays-top-add-display-embed" formnovalidate="formnovalidate" type="submit" id="edit-displays-top-add-display-embed" name="op" value="Embed">
        </li>
        <li>
          <input class="add-display button js-form-submit form-submit" data-drupal-selector="edit-displays-top-add-display-entity-reference" formnovalidate="formnovalidate" type="submit" id="edit-displays-top-add-display-entity-reference" name="op" value="Entity Reference">
        </li>
        <li>
          <input class="add-display button js-form-submit form-submit" data-drupal-selector="edit-displays-top-add-display-feed" formnovalidate="formnovalidate" type="submit" id="edit-displays-top-add-display-feed" name="op" value="Feed">
        </li>
        <li class="last">
          <input class="add-display button js-form-submit form-submit" data-drupal-selector="edit-displays-top-add-display-page" formnovalidate="formnovalidate" type="submit" id="edit-displays-top-add-display-page" name="op" value="Page">
        </li>
      </ul>
    </li>
  </ul>
</div>

Views display top HTML tree after a rebuild with patch #5:

<div class="views-display-top clearfix" id="views-display-top">
  <div class="dropbutton-wrapper dropbutton-multiple">
    <div class="dropbutton-widget">
      <ul id="views-display-extra-actions" class="dropbutton">
        <li class="edit-details dropbutton-action">
          <a href="/admin/structure/views/nojs/edit-details/content/page_1" class="views-ajax-link">Edit view name/description</a>
        </li>
        <li class="dropbutton-toggle">
          <button type="button">
            <span class="dropbutton-arrow">
              <span class="visually-hidden">List additional actions</span>
            </span>
          </button>
        </li>
        <li class="analyze dropbutton-action secondary-action">
          <a href="/admin/structure/views/nojs/analyze/content/page_1" class="views-ajax-link">Analyze view</a>
        </li>
        <li class="duplicate dropbutton-action secondary-action">
          <a href="/admin/structure/views/view/content/duplicate">Duplicate view</a>
        </li>
        <li class="reorder dropbutton-action secondary-action">
          <a href="/admin/structure/views/nojs/reorder-displays/content/page_1" class="views-ajax-link">Reorder displays</a>
        </li>
        <li class="delete dropbutton-action secondary-action">
          <a href="/admin/structure/views/view/content/delete">Delete view</a>
        </li>
      </ul>
    </div>
  </div>
  <h2 class="visually-hidden">Secondary tabs</h2>
  <ul id="views-display-menu-tabs" class="tabs secondary">
    <li class="tabs__tab is-active">
      <a href="/admin/structure/views/view/content/edit/page_1" data-drupal-link-system-path="admin/structure/views/view/content/edit/page_1">Page<span class="visually-hidden">(active tab)</span></a>
    </li>
    <li class="add">
      <a href="#"><span class="icon add"></span>Add</a>
      <ul class="action-list" style="display:none;">
        <li class="first">
          <input class="add-display button js-form-submit form-submit" type="submit" name="op" value="Page">
        </li>
        <li>
          <input class="add-display button js-form-submit form-submit" type="submit" name="op" value="Attachment">
        </li>
        <li>
          <input class="add-display button js-form-submit form-submit" type="submit" name="op" value="Block">
        </li>
        <li>
          <input class="add-display button js-form-submit form-submit" type="submit" name="op" value="Embed">
        </li>
        <li>
          <input class="add-display button js-form-submit form-submit" type="submit" name="op" value="Entity Reference">
        </li>
        <li class="last">
          <input class="add-display button js-form-submit form-submit" type="submit" name="op" value="Feed">
        </li>
      </ul>
    </li>
  </ul>
</div>
tim.plunkett’s picture

  1. +++ b/core/modules/views_ui/src/ViewEditForm.php
    @@ -677,14 +693,16 @@ public function rebuildCurrentTab(ViewUI $view, AjaxResponse $response, $display
    +    $views_ui_controller = new ViewsUIController($this->viewsData);
    

    To be safer, this should use ClassResolver to instantiate this. That way if ViewsUIController's constructor changes, we don't need to care.

  2. +++ b/core/modules/views_ui/src/ViewEditForm.php
    @@ -677,14 +693,16 @@ public function rebuildCurrentTab(ViewUI $view, AjaxResponse $response, $display
    -    $build = $this->getDisplayTab($view);
    ...
    -    $build = $this->renderDisplayTop($view);
    

    If we're going to stop using these methods, we should deprecate/inline them.

Wim Leers’s picture

changes the elements after a rebuild, and the attributes like 'data-drupal-selector', 'formnovalidate' and even the CSS ids of

😵

How is that possible?!

andypost’s picture

I bet because states are bound to exact html-ids

lauriii’s picture

+++ b/core/modules/views_ui/src/ViewEditForm.php
@@ -677,14 +693,16 @@ public function rebuildCurrentTab(ViewUI $view, AjaxResponse $response, $display
+    $views_ui_controller = new ViewsUIController($this->viewsData);

This approach introduces a circular dependency between Drupal\views_ui\ViewEditForm and Drupal\views_ui\Controller\ViewsUIController. We might want to avoid that if possible.

Wim Leers’s picture

#11: but that still doesn't explain why the patch in #5 would break this?

lauriii’s picture

#13: The root cause for these problems is that the Views UI re-render completely by-passes form system. For example the data-drupal-selector attribute is added by Drupal\Core\Form\FormBuilder::doBuildForm. Some of the side affacts has been documented in the code as well:

AJAX context, ViewUI::rebuildCurrentTab() returns this outside of form context, so hook_form_views_ui_edit_form_alter() is insufficient.

I don't think this behavior is changed by #5. This is already like this in core.

Views UI provides some custom alter hooks which we might be able to use except that they are only triggered in modules. We should research if triggering hook_views_ui_display_tab_alter in themes would unblock Claro.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett
Issue summary: View changes
Issue tags: -Needs followup, -Needs issue summary update

Discussed at length with @lauriii. #5 at first seems like a stop-gap, but is insufficient. I will write a test that proves that we need something closer to #2. I will also address the rest of feedback from #5.

Also, even after fixing this bug, the change from #5 would be a nice improvement, but that should be left to #3051605: Create new template for rendering Views UI tabs..

Finally, #14 is 100% correct in that #5 does not cause any new bugs, the fact that the markup is missing the additional attributes is a direct effect of the bug itself.

xjm’s picture

Priority: Normal » Major
Issue tags: +blocker

Promoting to major since this is blocking the inclusion of Claro in core.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
896 bytes
3.33 KB

I don't know how close a proper solution to this issue is. If it's close, then ignore this comment.

If we want to explore a hack in the meantime...

Views UI provides some custom alter hooks which we might be able to use except that they are only triggered in modules. We should research if triggering hook_views_ui_display_tab_alter in themes would unblock Claro.

Here's a Views UI patch that does that, along with the corresponding Claro patch. With very casual testing, it looks to me to solve #3067085: Views top dropbutton breaks when editing a view, but I did not perform a detailed inspection of the HTML source as #8 did.

Note that there are by design very few alter hooks that themes are allowed to participate in. I'm not at all keen on the idea of adding new ones that aren't well thought out, which is why this patch names them in a way to indicate their internal/temporary nature.

effulgentsia’s picture

+++ b/core/modules/views_ui/src/ViewEditForm.php
@@ -363,6 +363,7 @@ public function getDisplayTab($view) {
+    \Drupal::theme()->alter('_internal_do_not_use__views_ui_display_tab', $build, $view, $display_id);
...
+    \Drupal::theme()->alter('_internal_do_not_use__views_ui_display_top', $element, $view, $display_id);

Alternatively, we could check if Claro is the active theme or a base theme of the active theme, and if it is, call the Claro functions directly. That would be a way to more strongly prevent other themes from using these hooks, so that we don't have to preserve BC for them.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

tim.plunkett’s picture

Cross-posted, so my patch numbers are wrong.


As #5.1 points out, this issue is really about changes from form_alter being lost. It's just coincidental that the way this bug was exposed was because of a form_alter making #theme changes.

#12 points out the circular dependency introduced by this approach.

And in #14, the question of hook_views_ui_display_tab_alter() is brought up. Views UI acknowledges that this code will not run in a form context, and so provides a hook. This is not documented anywhere but the hook invocation.

Views UI's usage of AJAX and the Form API is very convoluted, and in some places even predates the APIs themselves. Untangling that is a much larger issue, and this fix maintains internal consistency.

The last submitted patch, 20: 3067386-views-17-FAIL.patch, failed testing. View results

tim.plunkett’s picture

effulgentsia’s picture

A bit more docs.

effulgentsia’s picture

--- a/core/modules/views/views.api.php
+++ b/core/modules/views/views.api.php

Can we add this to views_ui.api.php instead?

tim.plunkett’s picture

We sure can. Opened #3087465: Move hook_views_ui_display_top_links_alter() to views_ui.api.php to move the existing Views UI hook that is in the wrong place.

The last submitted patch, 22: 3067386-views-22-FAIL.patch, failed testing. View results

effulgentsia’s picture

Title: Template suggestions for elements in the Views UI form not preserved on AJAX rebuild » AJAX rebuild of Views UI form re-renders parts of the form without using Form API, so additional hooks are needed in lieu of hook_form_view_edit_form_alter()
Status: Needs review » Reviewed & tested by the community

#25 looks great to me, and as a Theme API maintainer, I even accept the two alter hooks being routed to themes, despite what I wrote in #17 about not being keen on that. I think the docs in #23 explain the rationale for doing so.

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Overall this looks good; just a few nitpicks...

  1. +++ b/core/modules/views_ui/src/ViewEditForm.php
    @@ -361,8 +361,12 @@ public function getDisplayTab($view) {
    +    // Because themes can implement hook_form_FORM_ID_alter() and because this
    +    // is a workaround for hook_form_view_edit_form_alter() being insufficient,
    +    // also invoke this on themes.
    
    @@ -776,6 +780,14 @@ public function renderDisplayTop(ViewUI $view) {
    +    // Because themes can implement hook_form_FORM_ID_alter() and because this
    +    // is a workaround for hook_form_view_edit_form_alter() being insufficient,
    +    // also invoke this on themes.
    

    The inline comments document that we're implementing this as a workaround, which would make me expect a link to the followup to fix it (#3087455: \Drupal\views_ui\ViewEditForm::rebuildCurrentTab() breaks hook_form_view_edit_form_alter() implementations I think). It's documented on the hook docs, but not here where the hooks are invoked. Probably we want the @todo in both places.

  2. +++ b/core/modules/views_ui/tests/themes/views_test_classy_subtheme/views_test_classy_subtheme.info.yml
    @@ -0,0 +1,6 @@
    +core: 8.x
    

    Oh yay, another place that's gonna need to be fixed in #3072702: Core extensions should not need to specify the new core_version_requirement in *.info.yml files so that 9.0.x can be installed. :P

  3. +++ b/core/modules/views_ui/tests/themes/views_test_classy_subtheme/views_test_classy_subtheme.theme
    @@ -0,0 +1,22 @@
    + * Add hooks for tests to use.
    

    I think perhaps it implements views UI alter hooks in a theme to provide test coverage for them. :)

  4. +++ b/core/modules/views_ui/views_ui.api.php
    @@ -0,0 +1,59 @@
    + * @param array $build
    + *   The edit page.
    ...
    + * @param array $build
    + *   The edit page.
    

    We usually like to be more specific than array; since it's a render array, is it an array[] at least?

lauriii’s picture

Status: Needs work » Needs review
FileSize
7.11 KB
3.33 KB

This should address feedback from #28. 🤞

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

While I worked on the most-recentely-RTBC patch, I have reviewed the changes since then and can sign off that they are correct and address #28. Thanks @lauriii!

larowlan’s picture

+++ b/core/modules/views_ui/tests/src/FunctionalJavascript/DisplayTest.php
@@ -122,4 +122,29 @@ protected function toggleContextualTriggerVisibility($selector) {
+  public function testAjaxRebuild() {
+    \Drupal::service('theme_installer')->install(['views_test_classy_subtheme']);
+
+    $this->config('system.theme')
+      ->set('default', 'views_test_classy_subtheme')
+      ->save();

now we have support for setting the default theme via a test-class property, should we split this into its own class and make use of that property?

larowlan’s picture

Also, we need an issue summary update here, as the proposed resolution doesn't resemble the patch.

Tentatively tagging for needs change record too, on the basis of the new hook and making two new hooks available to themes, although given the invocations both have a todo, I'm not sure if we're advertising them? Their presence in .api.php makes me think so, so I added the tag.

larowlan’s picture

Happy if the answer to #31 is » followup issue

lauriii’s picture

Updated issue summary and added change record: https://www.drupal.org/node/3088233.

Opened follow-up for #31: #3088234: Use $defaultTheme for using views_test_classy_subtheme.

larowlan’s picture

Issue summary: View changes
larowlan’s picture

  • larowlan committed 5ed0049 on 9.0.x
    Issue #3067386 by tim.plunkett, effulgentsia, bnjmnm, lauriii, Wim Leers...

  • larowlan committed 5cea2a5 on 8.9.x
    Issue #3067386 by tim.plunkett, effulgentsia, bnjmnm, lauriii, Wim Leers...
larowlan’s picture

Version: 8.9.x-dev » 8.8.x-dev

Committed 5ed0049 and pushed to 9.0.x. Thanks!

backported to 8.9.x

published the change record

moving to 8.8.x for decision about backport

  • effulgentsia committed 6aafa5d on 8.8.x authored by larowlan
    Issue #3067386 by tim.plunkett, effulgentsia, bnjmnm, lauriii, Wim Leers...
effulgentsia’s picture

Status: Reviewed & tested by the community » Fixed

Pushed to 8.8.x and updated the CR to that version.

Status: Fixed » Closed (fixed)

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