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
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
Comment | File | Size | Author |
---|---|---|---|
#48 | view-beforepatch.png | 237.9 KB | davidhernandez |
#48 | view-withempty.png | 281.11 KB | davidhernandez |
#48 | view-withoutempty.png | 285.82 KB | davidhernandez |
#41 | 2510794-41.patch | 4.9 KB | andypost |
#41 | interdiff.txt | 2.25 KB | andypost |
Comments
Comment #1
th3m0d CreditAttribution: th3m0d as a volunteer commentedCompleted this. Testing in all browser and then will upload patch.
Comment #2
th3m0d CreditAttribution: th3m0d as a volunteer commentedAll checks out. Not surprised as it was a pretty easy removal of useless divs. Here is my patch.
Comment #3
Chernous_dn CreditAttribution: Chernous_dn at FFW for FFW commentedComment #5
star-szrSee 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!
Comment #6
davidhernandezI'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.
after:
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?Comment #7
Chernous_dn CreditAttribution: Chernous_dn at FFW for FFW commentedMay be need to leave the
<div>
for primary element.Comment #8
Chernous_dn CreditAttribution: Chernous_dn at FFW for FFW commentedComment #12
Arnion CreditAttribution: Arnion at Skilld commentedTest failure because the div was not closed properly.
Comment #13
davidhernandezSetting to needs review for testing.
Comment #14
Arnion CreditAttribution: Arnion at Skilld commentedRemoved wrap <div> to rows and no result content.
I think wrappers header and footer should be <header> and <footer> respectively.
Comment #16
Chernous_dn CreditAttribution: Chernous_dn at FFW for FFW commentedI tested patch #12. Looks good.
Comment #17
ShaunDychko CreditAttribution: ShaunDychko as a volunteer commentedFixed 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.
Comment #19
ShaunDychko CreditAttribution: ShaunDychko as a volunteer commentedFixed more tests.
Comment #20
ShaunDychko CreditAttribution: ShaunDychko as a volunteer commentedComment #21
ShaunDychko CreditAttribution: ShaunDychko as a volunteer commentedComment #22
davidhernandezThe 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.
Comment #23
andypostThe 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.
that's interesting case...
"exposed" elements was rendered in header section now they go after it!
Comment #24
ShaunDychko CreditAttribution: ShaunDychko as a volunteer commentedAre 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?Comment #25
davidhernandezDo 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.
Comment #26
andypost@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?
Comment #27
davidhernandez@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.
Comment #28
ShaunDychko CreditAttribution: ShaunDychko as a volunteer commentedAdding 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.Comment #29
star-szrAre 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.
Comment #30
dawehnerIdeally 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.
Comment #31
andypost@dawehner I suppose this is in scope because remove DIV also means "replace" with proper tag.
this DIVs should be removed/replaced anyway in the patch
Comment #32
andypostpretty sure used in tests only, so better check header/fotter with actual render in integration tests
Comment #33
davidhernandez<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.
Comment #34
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commentedAwesome to see progress on this!
Attached patch swaps those divs for header/footer. So that at least we can all test this =)
Comment #38
davidhernandezJust check that the patch in #22 still applies. I think that is what #34 is going for?
Comment #39
ShaunDychko CreditAttribution: ShaunDychko as a volunteer commentedI think #22 is great as well.
Comment #40
ShaunDychko CreditAttribution: ShaunDychko as a volunteer commentedComment #41
andypostFixed tests that rely on div for header/footer
Comment #42
karolus CreditAttribution: karolus as a volunteer commentedDoing some manual testing with placeholder content. Before and after shots for markup and display attached.
Comment #43
andypostContextual links is not working so filed new issue
Comment #44
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commentedOn #38:
Ow I see, sorry!- I should have read the conversation here more carefully.
Comment #45
joelpittetThis 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.
Comment #46
alexpottThe logic of only printing rows or empty has been removed. Not sure that's right.
Comment #47
davidhernandezI 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.
Comment #48
davidhernandezI 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.
Comment #49
davidhernandezI created another issue. #2547811: Header and footer are appearing in markup when they should be empty
Comment #50
alexpottThanks for looking into that @davidhernandez. Committed c6bd55b and pushed to 8.0.x. Thanks!