Problem/Motivation

The Drupal.Message.defaultWrapper() removes class="hidden" from the fallback div by using wrapper.removeAttribute('class'); which prevents adding classes needed for styling on the wrapper element.

Proposed resolution

Replace wrapper.removeAttribute('class') with wrapper.classList.remove('hidden').

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#8 Screen Shot 2019-12-11 at 16.53.01.png195.3 KBlauriii
#7 interdiff_5-7.txt692 byteszrpnr
#7 3086723-7.patch4.86 KBzrpnr
#5 3086723-5.patch4.64 KBzrpnr
#3 3086723-3.patch1.24 KBzrpnr
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lauriii created an issue. See original summary.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

zrpnr’s picture

zrpnr
he/him
Primary language English
Location UTC-7
CreditAttribution: zrpnr at Acquia commented
Status: Active » Needs review
FileSize
3086723-3.patch1.24 KB

Removing the entire attribute does seem like overkill when just the hidden class needs to be removed. If markup from StatusMessages was overridden to have additional classes those could be removed by this js.

It looks like only the div[data-drupal-messages-fallback] is affected by this, if Drupal is already printing a status message it would appear in the div[data-drupal-messages] which never has the hidden class and which would be selected instead as the defaultWrapper.

This patch is what @lauriii suggested in the IS, removing just the hidden class instead of the entire class attribute and any other classes that might already be in place.

I came across this issue from the @todo message in Claro messages.es6.js - if this lands we should open a follow-up to change that file.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Needs review » Needs work

It seems like it would be a good idea to solve the Claro issue as part of this given that Claro is now in core. This way we can ensure that the class can bee indeed added and that the @todo comment in Claro can be removed.

The change itself looks good so I'm moving this to NW to address the Claro @todo.

zrpnr’s picture

zrpnr
he/him
Primary language English
Location UTC-7
CreditAttribution: zrpnr at Acquia commented
Status: Needs work » Needs review
FileSize
3086723-5.patch4.64 KB

This fallback markup is added in StatusMessages::generatePlaceholder. This patch now adds a method to ClaroPreRender which replaces the markup to add .messages-list to that placeholder div.

Without the change in message.es6.js this new class would be removed along with hidden, and now there is no need to override Drupal.Message.defaultWrapper in Claro.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Needs review » Needs work 
+++ b/core/themes/claro/src/ClaroPreRender.php
@@ -176,6 +176,13 @@ public static function textFormat($element) {
 
+  public static function messagePlaceholder(array $element) {

We should add a comment for this method. Besides that, this looks great!

zrpnr’s picture

zrpnr
he/him
Primary language English
Location UTC-7
CreditAttribution: zrpnr at Acquia commented
Status: Needs work » Needs review
FileSize
3086723-7.patch4.86 KB
interdiff_5-7.txt692 bytes

Thanks @lauriii ! Added a comment for messagePlaceholder

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Needs review » Reviewed & tested by the community
FileSize
Screen Shot 2019-12-11 at 16.53.01.png195.3 KB

Thank you @zrpnr! 🙏This looks good now! 🚀Tested manually with cd_tools to confirm that there are no regressions.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

Is there test coverage of this in Claro's tests? And is there test coverage anywhere of the hidden class removal?

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented

We don't have test coverage for JavaScript messages in Claro. We could test this but it might open a rabbit hole to testing all kinds of things given that the class is not needed for functionality but for styling. 🤷‍♂️

We don't have explicit test coverage for checking that the class is added or removed, but the JavaScript messages system is tested by Drupal\FunctionalJavascriptTests\Core\JsMessageTest which tests that the system is functional. Failure to remove the hidden class would certainly cause this test to fail.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Category: Feature request » Task
Status: Reviewed & tested by the community » Fixed

I think this is more of a task than a feature.

Committed and pushed 09d69a9f13 to 9.0.x and fe8a46eb80 to 8.9.x. Thanks!

  • alexpott committed 09d69a9 on 9.0.x 
    Issue #3086723 by zrpnr, lauriii: Allow adding classes to JavaScript...

  • alexpott committed fe8a46e on 8.9.x 
    Issue #3086723 by zrpnr, lauriii: Allow adding classes to JavaScript...

Status: Fixed » Closed (fixed)

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