Allow customizing wrapper element in node.html.twig

Created on 14 March 2024, 8 months ago
Updated 17 September 2024, 2 months ago

Problem/Motivation

An article element should be identified with a heading. The one generated by node.html.twig may not be like this.

Proposed resolution

Use div when there will be no heading. Also allow the wrapper element to be customized.

Remaining tasks

Agree on what to do. Implement.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

🐛 Bug report
Status

Needs review

Version

11.0 🔥

Component
Theme 

Last updated about 5 hours ago

Created by

🇨🇦Canada Liam Morland Ontario, CA 🇨🇦

Live updates comments and jobs are added and updated live.
  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

  • Needs frontend framework manager review

    Used to alert the fron-tend framework manager core committer(s) that a front-end focused issue significantly impacts (or has the potential to impact) multiple subsystems or represents a significant change or addition in architecture or public APIs, and their signoff is needed (see the governance policy for more information). If an issue significantly impacts only one subsystem, use Needs subsystem maintainer review instead, and make sure the issue component is set to the correct subsystem.

  • Needs change record

    A change record needs to be drafted before an issue is committed. Note: Change records used to be called change notifications.

Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @Liam Morland
  • Status changed to Needs review 8 months ago
  • 🇨🇦Canada Liam Morland Ontario, CA 🇨🇦
  • Pipeline finished with Failed
    8 months ago
    Total: 500s
    #119439
  • Status changed to Needs work 8 months ago
  • 🇺🇸United States smustgrave

    This will need test coverage

    Updates to other templates

    A change record to announce it.

    But also will probably need frontend manager sign off for such a change.

  • 🇨🇦Canada Liam Morland Ontario, CA 🇨🇦

    Yes, I would like to get approval for this change before going to all the work of implementing it.

  • 🇺🇸United States bnjmnm Ann Arbor, MI

    FEFM here:
    While I'm not against making these kind of optimizations, something like this could result in regressions on existing sites that have styles/tests/etc that expect <article>. Sites this dependent on specific markup such as this should probably be extending a Stable theme, but I'd still like to be careful that these changes to fundamental templates provide more benefit than risk.

    Based on the context in MDN page linked in the IS + reviewing some W3 specs, it seems pretty reasonable to interpret the "should" at face value as opposed to it being a "must". I also see that the default scenario is one where a header appears, and that that default scenario is likely to be true by a very high percentage. Based on the MDN recommendation alone, I'm not sure the regression risk is justified. However, I'm also open to changing my mind - my current reluctance is based only on the info currently available to me. If there's further evidence that this is objectively an accessibility bug, or that there are clear benefits that warrant the regression risk, I'm very much open to hearing that.

  • 🇨🇦Canada Liam Morland Ontario, CA 🇨🇦

    An alternative would be to make the template allow setting the wrapper_element in preprocess. If it is not set, use article. This means no change in behaviour, but people can easily make the change if they want without the hassle of overriding the template file.

  • Status changed to Needs review 2 months ago
  • 🇨🇦Canada Liam Morland Ontario, CA 🇨🇦

    I have implemented #7 in the merge request. There is now no change in behaviour except that it it possible to change the wrapper element.

  • 🇨🇦Canada Liam Morland Ontario, CA 🇨🇦
  • Pipeline finished with Success
    2 months ago
    Total: 402s
    #285380
  • 🇺🇸United States smustgrave

    @bnjmnm should we remove frontend manager tag?

    Am moving to NW for test coverage and CR.

Production build 0.71.5 2024