Follow-up to #2497455: Remove unnecessary markup from views templates, a.k.a. divitis

Problem/Motivation

This child issue will tackle views-view.html.twig template. The other Views templates are handled in #2497455: Remove unnecessary markup from views templates, a.k.a. divitis

After the removal of CSS classes from the core templates, many unnecessary HTML tags were left behind. They were not removed in the process of removing classes and other work, because removing them requires paying particular attention to any affect on the output.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task
Issue priority Normal because nothing is broken.
Unfrozen changes Unfrozen because it only changes templates and possibly CSS which are both unfrozen.
Prioritized changes The main goal of this issue is to improve themer experience.
Disruption There should be minimal disruption.

Proposed resolution

Go through each core template and remove any unnecessary markup. This will require looking at the output to see if things shift around in unwanted ways. Various CSS files may also need checking. For example, if there is CSS like .some_class > div > .some_other_class the CSS should be adjusted, or deleted.

Evaluation criteria:

  • divs and spans with no additional attributes should be removed mercilessly unless they justify their existence.
  • Semantic tags (footer, article, etc.) should be retained even if they have no attributes.

User interface changes

API changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

th3m0d’s picture

Completed this. Testing in all browser and then will upload patch.

th3m0d’s picture

All checks out. Not surprised as it was a pretty easy removal of useless divs. Here is my patch.

Chernous_dn’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: issue2510794removing_unnecessary_div_elements.patch, failed testing.

star-szr’s picture

Issue tags: +dreammarkup

See the discussion on the issue this was spun off from (see around #2497455-14: Remove unnecessary markup from views templates, a.k.a. divitis ), it's unfortunately not so easy. We need to decide how to make this template more semantic and less divvy. Therefore adding the dreammarkup tag.

Thanks for the patch @th3m0d!

davidhernandez’s picture

I'd like to see some screenshots of the markup before and after the divs are removed, if anyone has the time to do that on a decent sample View.

Regarding the AreaEntityTest fail, this is what I see before and after fixing the xpath.

Array
(
    [0] => SimpleXMLElement Object
        (
            [div] => SimpleXMLElement Object
                (
                    [@attributes] => Array
                        (
                            [class] => field field-entity-test--name field-name-name field-type-string field-label-hidden
                        )

                    [div] => SimpleXMLElement Object
                        (
                            [@attributes] => Array
                                (
                                    [class] => field-items
                                )

                            [div] => iyi6G8Kt
                        )

                )

        )

)

after:

Array
(
    [0] => SimpleXMLElement Object
        (
            [@attributes] => Array
                (
                    [class] => field field-entity-test--name field-name-name field-type-string field-label-hidden
                )

            [div] => SimpleXMLElement Object
                (
                    [@attributes] => Array
                        (
                            [class] => field-items
                        )

                    [div] => x5C9tsIy
                )

        )

)

We can see it changes slightly because the div is gone. When the SimpleXMLElement gets cast to string it outputs something like full | iyi6G8Kt. Afterwards, the 'full' is missing. I don't see a toString method anywhere to tell what it is doing. Anyone know how to track that down?

Chernous_dn’s picture

Status: Needs work » Needs review

May be need to leave the <div> for primary element.

Chernous_dn’s picture

Status: Needs review » Needs work

The last submitted patch, 8: remove_unnecessary-2510794-7.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 8: remove_unnecessary-2510794-7.patch, failed testing.

Arnion’s picture

Test failure because the div was not closed properly.

davidhernandez’s picture

Status: Needs work » Needs review

Setting to needs review for testing.

Arnion’s picture

Removed wrap <div> to rows and no result content.
I think wrappers header and footer should be <header> and <footer> respectively.

Status: Needs review » Needs work

The last submitted patch, 14: remove_unnecessary-2510794-14.patch, failed testing.

Chernous_dn’s picture

Status: Needs work » Needs review
FileSize
11.75 KB

I tested patch #12. Looks good.

<div class="view-header">
      test header
    </div>
<div class="view-content">
          <div class="views-row">
    <div class="views-field views-field-title"><span class="field-content"><a href="/node/1" hreflang="en">test</a></span></div>
  </div>
    </div>
<div class="view-footer">
      test footer
    </div>
ShaunDychko’s picture

Fixed tests and used <footer> and <header> tags as suggested in #14 and also suggested in the original issue here: https://www.drupal.org/node/2497455#comment-9991389.

Markup tested with the Stark theme.

AJAX pagination is broken, but that was also the case before applying patch #14, so I guess that topic belongs in another issue. Pagination without AJAX works fine.

Views markup

Status: Needs review » Needs work

The last submitted patch, 17: remove_unnecessary-2510794-17.patch, failed testing.

ShaunDychko’s picture

Fixed more tests.

ShaunDychko’s picture

Status: Needs work » Needs review
ShaunDychko’s picture

davidhernandez’s picture

The indentation needed fixing, but I think most of thise 'if's are no longer needed? Good job fixing the tests. It looks like the header and footer tags actually helped here.

andypost’s picture

The only question here about removal "exposed" from header section.
I think that nice, but we need to check what template used to render header because if there's no H* tags the header makes no sense.

+++ b/core/modules/views/templates/views-view.html.twig
@@ -39,55 +39,30 @@
+    <header>
       {{ header }}
-    </div>
-  {% endif %}
-  {% if exposed %}
-    <div>
-      {{ exposed }}
-    </div>
-  {% endif %}
-  {% if attachment_before %}
-    <div>
-      {{ attachment_before }}
-    </div>
+    </header>
   {% endif %}

that's interesting case...
"exposed" elements was rendered in header section now they go after it!

ShaunDychko’s picture

Are the "if's" needed? I found #1806538: Remove @todo about enabling strict_variables in Twig which says "scrict_variables" is "false", which means the "if's" are not needed. On the other hand #2445705: Core themes should not throw exceptions when strict_variables is TRUE suggests core themes should work if "strict_variables" is turned on, but there doesn't seem to be agreement. What should we do here? Is it worth checking all the preprocess functions to see that every variable is at least set to NULL?

As for the <header> and <footer> tags I linked to a couple of comments above, and a more authoritative description of the <header> element is https://developer.mozilla.org/en-US/docs/Web/HTML/Element/header. The element's are appropriate here. They don't need to contain specific types of tags. A header element doesn't need to contain an h2, or h3 tag, for example. The header tag contains any content meant to introduce what follows, which I think is it's typical usage in a View.

I think you bring up a good question as to whether the <header> should contain the "exposed" and "attachment_before" sections. In the current HEAD branch of Drupal, it doesn't. But should it?

davidhernandez’s picture

"exposed" elements was rendered in header section now they go after it!

Do you mean in the patch? Ordering wasn't changed. In general, we definitely need to do some good testing of the markup. Maybe someone can come up with some good use cases and screenshot the markup.

@ShaunDychko, the ifs are definitely not needed for the variables themselves. They were only there because of the wrapper elements (divs). You don't want to print the divs if the variable they contain is not there or is empty.

andypost’s picture

@davidhernandez you right, I just mixed ideas... but actually I thought about using header for "exposed" like #24 said

Not sure about "attachments" but probably it makes sense also!

Is there issue to update classy template accordingly?

davidhernandez’s picture

@andypost, there is no issue to update the Classy template as this was intended to only remove the unneeded elements. If we do end up changing the basic structure of the markup, adding header/footer elements, we should update it.

A follow up would be fine, since the Classy template would need some slightly different restructuring. It looks like we are talking about replacing <div class="view-header"> with <header>. I'm not sure if there is any CSS attached to it. (A quick search doesn't find anything.)

Although, it might be worth talking about here. I'm looking at both templates, and header, exposed, attachment, etc are all conditionally included and separate from each other. Putting them all into the header tag will make things a bit more complicated. We'll have to be careful about checking different scenarios.

We could also just remove the divs here and make a follow up to deal with reworking the templates. My concern with that would be making too many separate changes, and aggravating beta testers.

ShaunDychko’s picture

Adding the <header> and <footer> tags might have been out of the scope of this particular issue "Remove unnecessary..." and adds more onerous testing (ie. what else, such as JS or CSS, depends on a div tag rather than a header tag surrounding the 'header'?) Shall we go with regular div's, and punt the header and footer tag discussion to another issue? Patch attached for regular div's.

star-szr’s picture

Are the wrappers needed for any views functionality? Or just for tests?

I think it might make sense to stick with header and footer but not change what's inside them, like the patch in #22.

dawehner’s picture

Ideally I would like to replace the div for footer/header with a more semantic <footer> tag, but I guess this is out of scope of this issue?
On the other hand removing the DIV there should not be an issue.

andypost’s picture

@dawehner I suppose this is in scope because remove DIV also means "replace" with proper tag.

+++ b/core/modules/views/templates/views-view.html.twig
@@ -39,55 +39,30 @@
   {% if header %}
     <div>
       {{ header }}
     </div>
   {% endif %}
...
   {% if footer %}
     <div>
       {{ footer }}
     </div>
   {% endif %}

this DIVs should be removed/replaced anyway in the patch

andypost’s picture

Are the wrappers needed for any views functionality?

pretty sure used in tests only, so better check header/fotter with actual render in integration tests

davidhernandez’s picture

<header> and <footer> have semantic meaning, can be descendant elements, can contain any other element, and can appear multiple times on the page, so should be ok to use here. I definitely don't want to leave a <div> there, if we need some wrapper element.

What we need is some manual testing to make sure we don't have any visual regressions that need fixing.

Manuel Garcia’s picture

Awesome to see progress on this!

Attached patch swaps those divs for header/footer. So that at least we can all test this =)

Status: Needs review » Needs work

The last submitted patch, 34: remove_unnecessary-2510794-34.patch, failed testing.

The last submitted patch, 34: remove_unnecessary-2510794-34.patch, failed testing.

davidhernandez’s picture

Just check that the patch in #22 still applies. I think that is what #34 is going for?

ShaunDychko’s picture

I think #22 is great as well.

ShaunDychko’s picture

andypost’s picture

Status: Needs work » Needs review
FileSize
2.25 KB
4.9 KB

Fixed tests that rely on div for header/footer

karolus’s picture

Doing some manual testing with placeholder content. Before and after shots for markup and display attached.

andypost’s picture

Contextual links is not working so filed new issue

Manuel Garcia’s picture

On #38:

Just check that the patch in #22 still applies. I think that is what #34 is going for?

Ow I see, sorry!- I should have read the conversation here more carefully.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing, -Needs screenshots

This is pretty sweet! This was manually tested with screenshots in #42

The tests look a lot more useful now with specific targetting the header/footer elements, gotta love that.

Pure core markup changes, much cleaner template.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/views/templates/views-view.html.twig
@@ -39,55 +39,30 @@
-  {% if rows %}
-    <div>
-      {{ rows }}
-    </div>
-  {% elseif empty %}
-    <div>
-      {{ empty }}
-    </div>
-  {% endif %}

The logic of only printing rows or empty has been removed. Not sure that's right.

davidhernandez’s picture

I believe the empty variable won't be populated if there is a result. The if was there because the wrapping div had a CSS class specific to the empty content, "view-empty", which is still in the Classy template. It should be easy to test though, if someone has a moment.

davidhernandez’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
285.82 KB
281.11 KB
237.9 KB

I tested the empty text. The empty text correct does not render when there is no empty text. See screenshots.

I did notice that the header and footer tags still appear.

I checked head, and that seems to be an existing bug. No matter what there is blank space rendering. You have to remove the header and footer from the View itself to go away. Anyone know if there is an open issue about that?

In any case, it seems unrelated to this issue.

davidhernandez’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for looking into that @davidhernandez. Committed c6bd55b and pushed to 8.0.x. Thanks!

  • alexpott committed c6bd55b on 8.0.x
    Issue #2510794 by ShaunDychko, Arnion, davidhernandez, andypost,...

Status: Fixed » Closed (fixed)

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