AJAX MessageCommand markup and styling differs from Theme default

Created on 24 October 2023, over 1 year ago

Problem/Motivation

MessageCommand was introduced in #3086096: Add a generic Ajax Message command β†’ . It is helpful to show status messages in AJAX contexts. Sadly it uses a totally different "render pipeline" than the active theme does. In fact it uses none, but builds the message area in JavaScript, without using the Theme's twig templates. That's how it's implemented.

From a Themers and Theme Prespective and from "Component" thinking, this is wrong. AJAX messages should use the same templates, as regular messages do.
That's why I'd rate this as bug from outer perspective. Still I think it was a (discussable) design decision.

Of course, I know talk is cheap and this is hard to resolve, because the messages are added dynamically and you can't tell if the message area already exists. But these problems can be solved.

https://git.drupalcode.org/project/drupal/-/blob/11.x/core/misc/message....
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Ajax%21Me...

Steps to reproduce

Show a regular status message using PHP in a custom theme, where the message area was modified in twig
Compare the result to a status message added by JavaScript. They differ a lot, if the JS-built dom structure isn't the same as the one in Twig.

Proposed resolution

Return the theme rendered message wrapper and the messages in the AJAX result. Only add the wrapper on the page, if none is present.
After adding or if the message wrapper is already existing, add the rendered messages.

This way the theme handles the message output, like it would do on a regular call, but AJAX builds the message from the theme results without a difference.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

πŸ› Bug report
Status

Active

Version

11.0 πŸ”₯

Component
AjaxΒ  β†’

Last updated 2 days ago

Created by

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @Anybody
  • πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica
  • πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica
  • πŸ‡¬πŸ‡§United Kingdom catch
  • πŸ‡ΊπŸ‡ΈUnited States luke.leber Pennsylvania

    Injecting here that this would make styling file upload errors quite a bit easier!

    It'd pay to take a hard look at the file module to determine how feasible the approach in the IS actually is.

  • πŸ‡¬πŸ‡§United Kingdom catch

    I took a look at how this could work.

    The main thing we want, is to use #theme 'status_messages' to render the messages in PHP, before they're sent to JavaScript.

    I think something like this would probably do it:

    1. Add a new RenderedMessagesCommand - it would take an array of messages instead of a single one.

    2. messages.js would need to support a new 'rendered' option (in the options array) where it just appends the markup string it was given, instead of passing it through Drupal.theme.message

    3. RenderedMessagesCommand would take the messages array, pass it through '#theme' 'status_messages', add the 'rendered' option, and return otherwise the same structure as MessagesCommand.

    4. To solve πŸ› 10.3 upgrade now missing status-message theme sugestions Active , we'd then use RenderedMessagesCommand in big pipe instead of MessageCommand.

    It doesn't allow us to remove Drupal.theme.message though, because we provide a pure JavaScript API to add messages, and those can't be rendered in PHP, so the underlying issue that themes need to theme JavaScript messages separately is still there. That makes me wonder how useful it really is compared to opening issues against themes that don't implement message styling. e.g. I just found πŸ“Œ Add js message theme override to match Umami message markup Needs review .

  • πŸ‡ΊπŸ‡ΈUnited States luke.leber Pennsylvania

    We've started to use drupalSettings + js_settings_alter to provide server-side rendered templates to the frontend.

    Our Drupal.theme.messages implementation pulls the markup from drupalSettings. Might not be robust enough for core, but has worked rather well for us...and even covers the obscure file.module validation cruft too.

  • πŸ‡ΉπŸ‡·Turkey Umit Turkey

    These update problems of Drupal are so annoying that I'm at the point where I'm going to quit. There is no documentation on the subject. Why would they make such a change without consulting the community. I am not updating any site because of this issue.

  • πŸ‡¬πŸ‡§United Kingdom catch

    @luke.leber πŸ› 10.3 upgrade now missing status-message theme sugestions Active shows pretty conclusively that the current situation with separate js and twig theming isn't robust, so I think it's a good idea to try this - could you extract your logic into an MR? Seems OK for bc too given we'd be changing the default behaviour and js theme overrides should still work the same way.

  • Merge request !9329Let's see if this has any problems? β†’ (Open) created by luke.leber
  • Pipeline finished with Failed
    6 months ago
    Total: 241s
    #265094
  • Pipeline finished with Failed
    6 months ago
    #265108
  • Pipeline finished with Failed
    6 months ago
    #265113
  • Pipeline finished with Failed
    6 months ago
    Total: 385s
    #265111
  • Pipeline finished with Failed
    6 months ago
    #265178
  • Status changed to Needs work 6 months ago
  • πŸ‡¬πŸ‡§United Kingdom catch
  • πŸ‡ΊπŸ‡ΈUnited States daniel korte Brooklyn, NY

    daniel korte β†’ made their first commit to this issue’s fork.

  • Pipeline finished with Failed
    6 months ago
    Total: 470s
    #274808
  • πŸ‡©πŸ‡ͺGermany Tomefa Dresden

    I get the status message problem from https://www.drupal.org/project/drupal/issues/3396318 πŸ› AJAX MessageCommand markup and styling differs from Theme default Active and just try this MR.
    It doesn't fix the issue, it still using block--system-messages-block.html.twig

  • πŸ‡ΊπŸ‡ΈUnited States bkosborne New Jersey, USA

    #6 would solve this for rendering messages that are invoked from the backend, but not solve the problem where purely front-end rendered messages still use a different template/rendering solution. The MR solves this. It's a bit weird to use drupalSettings for storing the template. feels like there should be a better way to deliver the template down.

  • πŸ‡ΊπŸ‡ΈUnited States luke.leber Pennsylvania

    Agree with #14. MR !9329 is very close to what we use in our custom theme, but I'm not convinced it's core-worthy. It's pretty messy and would be hard to predict / guarantee b/c for folks that do strange things to their messages templates.

    There would have to be at minimum additional API set up to dynamically fetch the supported status headings in order to even offer full backwards compatibility support.

  • πŸ‡¬πŸ‡§United Kingdom catch

    Oh I didn't realise I hadn't commented properly here since #6. The MR is great and much, much better than #6 - I posted that comment before even knowing the MR solution was a possibility. Looks like a couple of real test failures though.

  • πŸ‡¬πŸ‡§United Kingdom catch
  • πŸ‡«πŸ‡·France nod_ Lille

    Comming from πŸ“Œ Replace js message with webcomponents Active and in that issue it does esentially the same thing through a <template> tag added to the DOM somewhere (if we ignore the whole webcomponent/ShadowDOM part).

    Instead of putting that in drupalsettings, I'd rather have a <template> dom node on the page to query when needed

  • πŸ‡ΊπŸ‡ΈUnited States bkosborne New Jersey, USA

    Ohhh yes, a template element seems most appropriate here.

  • Following the issue because I was actually implementing a custom template for Drupal messages. My use case is to use a single-directory component for message items. I naturally use the same logic as explained at the beginning and in the current MR: to pass the template to JS through the drupalSettings.

    Unless I'm wrong, this is how we do things in Drupal for years. Let JS knows about things from the back via drupalSettings 🀷

    Using a WebComponent (e.g. template) sounds like a good idea but It seems a bit too much to me to implement something different that what we're used to. If we go this way, doesn't it retire the whole Drupal JS API / drupalSettings in some ways? Does it means that we should use WebComponent for everything that can be rendered both in backend and frontend basically?

    I +1 to pass the template with a placeholder through drupalSettings.

  • πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

    Just added information to the issue summary that this also supposed to fix Devels current dsm() incompatibility with BigPipe.
    Currently kints output in messages can't be expanded and is useless because of this. https://gitlab.com/drupalspoons/devel/-/issues/525#note_2039843855

  • πŸ‡¬πŸ‡§United Kingdom catch

    template element sounds good here.

    If we go this way, doesn't it retire the whole Drupal JS API / drupalSettings in some ways?

    A lot of things in drupalSettings aren't rendered, but also drupalSettings has a lot of problems - it's a big array blog in the header of the page and the output isn't cacheable. If we can put some things into discreet elements instead that will be an improvement.

  • πŸ‡§πŸ‡ͺBelgium wouter.dierick@gmail.com

    I implemented this for a custom project:

    - add your messages html or render ui_patterns/sdc component on every page inside a tag
    - extend the core/drupal.message library with my own library
    - add new library and JS with custom Drupal.theme.message implementation (start from web/core/misc/message.js )
    - The JS will copy html from the tag, insert type, text inside the html and render it in the

    element.

    Works good, but by overriding the core js this is quite sensitive for core updates...

  • πŸ‡·πŸ‡ΈSerbia finnsky

    https://www.drupal.org/project/drupal/issues/3476471 πŸ“Œ Replace js message with webcomponents Active
    Merged now. So it can be used as testing area for `template` and Shadow Dom approach

Production build 0.71.5 2024