Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
We have a polyfill that supports collapsing the HTML5 details element in browsers that do not support it natively.
All modern browsers support the details element now so we can remove the polyfill.
Steps to reproduce
Proposed resolution
Remove the polyfill code.
Remove all CSS and JS around the collapse-processed and details-title classes as these are now obsolete.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
In Drupal 10 the details HTML tag is available in all supported browsers and the supporting code for Internet Explorer will be removed.
| Comment | File | Size | Author |
|---|---|---|---|
| #38 | Status_report___Drupal.png | 305.08 KB | mherchel |
| #23 | Screen Shot 2022-05-20 at 7.29.43 AM.png | 57.06 KB | bnjmnm |
Issue fork drupal-3269082
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3269082-remove-html5-details
changes, plain diff MR !1967
1 hidden branch
Comments
Comment #2
longwaveThere was a surprisingly large amount of code and CSS to support this feature.
Comment #4
longwaveComment #5
longwaveWhen committing please credit @kostyashupenko from the parent issue as parts of their patch were borrowed here.
Comment #7
mherchelUpdated the branch.
Looked through the MR, and all of the changes look good! This is a pretty large MR, so if it might be more manageable to create followups for each theme instead of tackling everything in this one commit.
That being said, I'll let the core committer make that decision, as everything that I tested here looks perfect.
Comment #8
bnjmnmThere are multiple @todo items pointing to #2981732: Add prefix to Modernizr configuration, aside from that issue being closed anyway, the changes this issue would have been the ones to make those todos irrelevant, so they should get removed in this issue's scope.
Setting to NW for visibility - I'm still reviewing but don't expect to find much, this looks pretty good.
Having all the theme changes here seems fine - referencing the changes in one theme to contextualize the other actually makes the review a little easier.
Comment #9
andregp CreditAttribution: andregp at CI&T commented@bnjmnm Does this address your comments? If so I can commit these changes to the MR.
Comment #10
bnjmnmDid you check to see if removing this and other .details style doesn't cause unwanted effects. There's a good chance the styling should still be there - it just can't rely on a class that won't be provided once Modernizr goes away.
Comment #11
andregp CreditAttribution: andregp at CI&T commented@bnjmnm I didn't... I though that once Modernizr is removed these styling would not be applied anywhere (since there wouldn't be any element with .details class anymore), so I just removed them.
I though about removing just the .details part from the selectors, but I wondered if this wouldn't cause the style to be applied on other undesired elements (like for e.g. being applied to all elements with .system-status-report__status-icon--error:before class, etc).
Should I search for a class to substitute .details?
Comment #12
bnjmnmRe #11 I'm guessing you can just remove
.details, but would need to manually test to be sure. I suspect it was there to add these styles only if<details>elements are supported. In D10 all supported browsers work with<details>, so I assume it simply isn't necessary.A class like
.system-status-report__status-icon--erroris (intentionally) a very specific selector that does what it says. It is named using the BEM methodology and it's all but guaranteed any class with that name wants to be styled the same.The best way to know if this works correctly is go to the system status page and compare screenshots before/after the patch - and make sure there isn't a
.detailsclass in<html>post-patch. It should look the same. If icons are suddenly missing this means something likely got removed that should have been modifiedComment #13
longwaveThe icons still show, because there is a fallback in system.admin.css, but they are not quite correctly aligned.
It is also tricky to test because Modernizr is still loaded on the system report page and so the
<html>tag still has thedetailsclass applied.We haven't removed the prefix, so should we be removing the todo?
Comment #14
longwavePushed a commit that removes the
.detailsclass. Also cleans up what seemed to be a minor bug in both Seven and Claro where the LTR version used the icon class, but the RTL version used the title class instead. As this is all icon related I changed both to use the icon class; both classes are applied to the element in question, and system.admin.css also uses icon in both cases, so this just seems better for consistency.I want to do some more testing on collapse.es6.js before removing the code that opens fragments automatically; I couldn't get this to work properly.
Comment #15
longwaveSeven's system-status-report.css still has this slightly cryptic bit of CSS:
I think this can be safely removed now, but not entirely sure?
Comment #16
longwaveRe: collapse.es6.js, I think this needs to stay. formFragmentLinkClickOrHashChange is a custom event fired by form.js when a link is clicked on the same page that contains a fragment. There is an argument for refactoring it into form.js itself I think, given that it is effectively one line. I guess some functional JS test coverage would be useful here :)
Comment #18
longwaveHiding #9, the MR is the thing to review again.
Comment #19
bnjmnmRe #13
A few things led me to conclude that we may as well remove the todo (also glad you're asking for clarification 🙂):
.detailsclass to<html>, and un-prefixing these would then move it to the<details>elements. If all the themes switch to a.detailsclass, the css can just target the<details>element.I think screenshots of the system settings page before/after and RTL would be helpful. I often remind users not to clog an issue with redundant/unnecessary screenshots... but right now I'm officially stating "this could use some screenshots", and I invite the screenshot-inclined to hook us up.
Comment #20
Wim LeersWhat's the next step here? 😊
Comment #21
longwaveSomeone to make some screenshots as requested in #19, and ideally someone to understand what the CSS in #15 does and whether it can be removed now.
Comment #22
longwaveRebased against 10.0.x.
Comment #23
bnjmnmRe #15
This CSS can be removed
But there will be a small regression unless this is added:
Without this, the summary element uses the default
displayvalue oflist-item, which results in making the hidden disclosure arrows visibleComment #24
nod_added release note
Comment #25
nod_took a stab at writing the change notice
Comment #26
longwaveChange notice looks fine to me. This isn't going into 9.x, so retagging for 10.0.0 release notes.
MR needs rebasing and the MR comments and #23 still need addressing.
Comment #28
catchThis is the last remaining blocker to deprecating modernizr, which we want to do in 10.1.x for removal in 11.x, so bumping to critical.
Comment #29
catchReplacing the general 'stop using modernizr' issue in #3118149: [meta] Requirements for tagging Drupal 10.0.0-beta1 with this one, so tagging as a blocker. I'm not sure if it'd be fine to just do this in 10.1.x instead, if so we could make it a 'should have'.
Comment #30
Gábor HojtsyComment #31
catchPretty sure we can just do this in a minor, but leaving as critical since it still blocks modernizr removal.
Comment #33
SpokjeRebased and force pushed MR
Comment #34
nod_soft postponed on #3110137: Remove Classy from core that will make this easier.
Comment #35
bnjmnmUpdated the MR, which is much smaller with Classy/Bartik/Seven removed.
Comment #36
bnjmnmComment #37
mherchelThis is looking really good. I have some questions/concerns about the remaining collapse.js file
in #16, @longwave states
However in this MR's version of the file, the event now references a function that was removed in the MR. This will result in a console error. Note that the
formFragmentLinkClickOrHashChangeevent also gets referenced in vertical-tabs.jsIn an MR comment (below #22) @nod_ suggested
This is not yet done, however I'm still unclear on the need for leaving collapse.js/details.js
I'm going to reach out to @nod_ in Slack to ask him to comment/clarify.
Other than the JS issue, this looks RTBC to me. I tested both Claro and Olivero's details elements, in addition to testing the status report styles within Claro (of which the styles were modified).
Comment #38
mherchelNote that you can see the console error in action on the status report page
Comment #39
bnjmnmRe #38: looks like more got ripped out than there should have been. MR updated
Comment #40
longwaveElementsVerticalTabsTest still refers to the now-removed collapse.js. The comment reads:
However this functionality was provided by the now-removed polyfill, so I think this test can be safely removed.
Comment #41
nod_thanks for the file rename. Everything looks in order.
Comment #44
lauriiiIt's nice to see how much simpler this issue has evolved over time 🤩
Committed 9ad9131 and pushed to 10.1.x. Also cherry-picked to 10.0.x. Thanks!
I'm not sure we actually need a release-note for this, but I did publish the CR. We will have a generic release note about the changed browser support which should cover this as well.