Problem/Motivation

Found in #3406612: Exceptions in batch no longer are shown on the page: Javascript error where we had to use olivero as the test them because the message.js does not work with the stark theme.

This is because core/modules/system/templates/status-messages.html.twig does not print anything under the top level div if there are no messages.

in core/misc/message.js we have 

return wrapper.innerHTML === ''
        ? Drupal.Message.messageInternalWrapper(wrapper)
        : wrapper.firstElementChild;

But wrapper.innerHTML !== '' because there is return character. Any other character would also make it break. So in this case wrapper.firstElementChild will be NULL.

We could change the template but any contrib or custom template with this would also break.

Steps to reproduce

  • Using the stark theme (or many others)
  • On a fresh page load, run (new Drupal.Message()).clear(); in the browsers JS console
  • The code will go this.messageWrapper.querySelectorAll, but messageWrapper will be null

Proposed resolution

Check wrapper.childElementCount === 0 instead of wrapper.innerHTML === ''

Remaining tasks

Review

Issue fork drupal-3407067

Command icon 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:

About issue forks

Comments

tedbow created an issue. See original summary.

tedbow opened merge request !5741

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Active » Needs review
Issue tags: +Needs tests
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Title: message.js assumes status messages element has child element, incompatible default template » message.js doesn't status messages no child element but whitespace, incompatible default template
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Title: message.js doesn't status messages no child element but whitespace, incompatible default template » message.js doesn't work status messages element with no child element but whitespace, incompatible default template
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

No test failure is probably a good sign.

Could steps to reproduce be added please.

tim bozeman’s picture

tim bozeman commented
Issue summary: View changes
Status: Needs work » Needs review

Added testing instructions.

tim bozeman’s picture

tim bozeman commented
Issue summary: View changes
tim bozeman’s picture

tim bozeman commented
Issue summary: View changes
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Thanks for adding those! Was previously tagged for tests too in #3

tim bozeman’s picture

tim bozeman commented
Status: Needs work » Needs review

Thank you smustgrave

I added a test and it looks like they are all passing.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Possible to merge this into JsMessageTest to keep things well grouped.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

quietone made their first commit to this issue’s fork.

quietone’s picture

quietone commented
Title: message.js doesn't work status messages element with no child element but whitespace, incompatible default template » message.js should work with whitespace in empty message wrappers
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs tests +11.5.0 priority

Came across this while working on deprecating Olivero.

For #12, I simply renamed the test file, mostly because the existing test has setup function that this new test does not need.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Small comments on the MR.

quietone’s picture

quietone commented
Parent issue: » #3595082: [meta] Tasks to deprecate the Olivero theme
quietone’s picture

quietone commented
Issue tags: -11.5.0 priority +11.5.0 release priority
quietone’s picture

quietone commented
Status: Needs work » Needs review

Back to needs review for the recent changes.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs review » Reviewed & tested by the community

I manually code reviewed this MR. The changes look good and simple. @smustgrave's concerns have been resolved fine.

godotislate made their first commit to this issue’s fork.

  • godotislate committed 09c7a1d0 on 11.x 
    fix: #3407067 message.js should work with whitespace in empty message...

  • godotislate committed 970ac364 on main 
    fix: #3407067 message.js should work with whitespace in empty message...
godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Fixed

I kinda feel like the test should have been appended to the end of JsMessageTest::testAddRemoveMessages(), with the test method possibly renamed for purpose, so that we save a browser test bootstrap. But I don't think it's enough to block here, and we can refactor the test in the future if we really need to for CI run optimization.

Committed and pushed 970ac36 to main and 09c7a1d to 11.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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