Split off from #3153234: [Meta] Olivero JavaScript should be selecting [data-drupal-selector] attributes where possible

We need to be using [data-drupal-selector] attribute selectors in messages.es6.js

Testing instructions:

  1. Try to login with invalid credentials
  2. Note the message that appears. Make sure it looks correct and the close (X) button works.

Remaining Tasks

We need tests for this. We should be able to use Drupal\FunctionalJavascriptTests\Core\JsMessageTest as a basis for the tests.

CommentFileSizeAuthor
#14 mixed-source-messages.png75.82 KBandy-blum
#14 all-js-messages.png35.62 KBandy-blum
#14 all-php-messages.png45.75 KBandy-blum
#11 interdiff-4-11.txt2.43 KBmherchel
#11 3212975-11.patch4.86 KBmherchel
#10 Cursor_and_Status_Messages___Olivero-2.png177.62 KBmherchel
#10 Cursor_and_Status_Messages___Olivero.png156.29 KBmherchel
#7 Screen Recording 2021-05-10 at 16.25.00.mov4.32 MBGauravvvv
#6 Screen Shot 2021-05-10 at 13.20.07.png126.05 KBlauriii
#4 interdiff-2_3.txt578 bytesGauravvvv
#4 3212975-4.patch2.86 KBGauravvvv
#3 Screenshot-3212975.png552.23 KBNeslee Canil Pinto
#2 3212975-2.patch2.87 KBmherchel

Issue fork drupal-3212975

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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mherchel created an issue. See original summary.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel at Lullabot commented
Issue summary: View changes
Status: Active » Needs review
FileSize
3212975-2.patch2.87 KB

Patch attached. Tugboat preview at https://3212975-messages-js-tbho9jrqqztqm3ordulbnwqxut4s7jsx.tugboat.qa/

Neslee Canil Pinto’s picture

Neslee Canil Pinto
he/him
Primary language English
CreditAttribution: Neslee Canil Pinto at Ramsalt Lab commented
FileSize
Screenshot-3212975.png552.23 KB

This seems to be working, used tugboat preview and followed the steps
1. The close button is working fine
2. [data-drupal-selector] attribute is been present too

Gauravvvv’s picture

Gauravvvv CreditAttribution: Gauravvvv at OpenSense Labs commented
FileSize
3212975-4.patch2.86 KB
interdiff-2_3.txt578 bytes

Re-rolled patch #2, attached interdiff for the same.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel at Lullabot commented
Status: Needs review » Reviewed & tested by the community

#4 looks perfect. Thanks!

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Reviewed & tested by the community » Needs review
FileSize
Screen Shot 2021-05-10 at 13.20.07.png126.05 KB

Where did you test this? I get following JS error when I try JS messages with the patch.

Gauravvvv’s picture

Gauravvvv CreditAttribution: Gauravvvv at OpenSense Labs commented
FileSize
Screen Recording 2021-05-10 at 16.25.00.mov4.32 MB

Hi Lauriii, I didn't get any console error message while tested with the given instructions.

Attached an after-patch screen recording.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel at Lullabot commented
Status: Needs review » Needs work

Where did you test this? I get following JS error when I try JS messages with the patch.

I tested this with the shortcuts module, and by attempting to login with invalid credentials. What steps did you take to do "JS messages"?

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Issue tags: +Needs tests

I used this custom module for generating JS messages: https://github.com/zolhorvath/cd_tools/tree/master/modules/message. Let's also make sure that there's test coverage for the JS messages since based on #4 passing, we are potentially missing test coverage.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel at Lullabot commented
FileSize
Cursor_and_Status_Messages___Olivero.png156.29 KB
Cursor_and_Status_Messages___Olivero-2.png177.62 KB

I still cannot reproduce this problem.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel at Lullabot commented
FileSize
3212975-11.patch4.86 KB
interdiff-4-11.txt2.43 KB

No tests yet, but here is the fix for the error in #6

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel at Lullabot commented

From @lauriii in Slack:

I think Drupal\FunctionalJavascriptTests\Core\JsMessageTest is the test you might want to use as a basis for your test

Gauravvvv’s picture

Gauravvvv CreditAttribution: Gauravvvv at OpenSense Labs commented
Status: Needs work » Needs review
andy-blum’s picture

andy-blum
he/him/his
Primary language English
Location Ohio, USA
CreditAttribution: andy-blum commented
FileSize
all-php-messages.png45.75 KB
all-js-messages.png35.62 KB
mixed-source-messages.png75.82 KB

I'm noticing something a bit strange regarding the messages.

Everything works fine when messages are added in from PHP or from JS via Drupal.Message, but JS messages added in when PHP messages are already present are not properly placed. Screenshots are attached. All messages close as expected, and "nested" messages close when their parent is closed.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel at Lullabot commented

Split off #14 into #3223264: Olivero: Messages can be malformed when JS creates messages and PHP messages already exist.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel at Lullabot commented
Issue summary: View changes

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

thejimbirch opened merge request !934

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel at Lullabot commented
Status: Needs review » Needs work

Tests failed :(

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

marcoscano opened merge request !943

marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
CreditAttribution: marcoscano at Lullabot commented

Re-rolled the previous work for 9.3.x, no changes, on the 3212975-olivero-normalize-javascript branch. Tests will still fail.

marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
CreditAttribution: marcoscano at Lullabot commented

Ah! Didn't realize the class inherited this :: testAddRemoveMessages test :P
More troubleshooting to do there then.

marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
CreditAttribution: marcoscano at Lullabot commented

So the failure was

Testing Drupal\FunctionalJavascriptTests\Theme\OliveroMessagesTest
.F                                                                  2 / 2 (100%)

Time: 00:12.833, Memory: 4.00 MB

There was 1 failure:

1) Drupal\FunctionalJavascriptTests\Theme\OliveroMessagesTest::testAddRemoveMessages
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    0 => 'This is a message of the type, status. You be the judge of its importance.'
+    0 => 'Status message This is a message of the type, status. You be the judge of its importance. Close message'
 )

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:96
/var/www/html/core/tests/Drupal/FunctionalJavascriptTests/Core/JsMessageTest.php:121
/var/www/html/core/tests/Drupal/FunctionalJavascriptTests/Core/JsMessageTest.php:55
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:722

I'm unfamiliar with this fix, but I'm guessing the behavior is the expected, and just the test needs to be adjusted to pass if the text we are searching is _included_ in the element's text, instead of being an exact match? I updated the test in this direction, seems to be green for me locally.

marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
CreditAttribution: marcoscano at Lullabot commented
Status: Needs work » Needs review
andy-blum’s picture

andy-blum
he/him/his
Primary language English
Location Ohio, USA
CreditAttribution: andy-blum commented
Status: Needs review » Reviewed & tested by the community

With the exception of the issue spun off in #14, this all looks good to me. Moving to RTBC!

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

Hiding patches now there's an MR

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Needs review
Issue tags: -Needs tests

Left a couple of questions on the MR

marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
CreditAttribution: marcoscano at Lullabot commented

Addressed review feedback.
(I can't run tests right now so will rely on the testbot to check my changes. Apologies if it comes red for some reason.)

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

geekygnr’s picture

geekygnr CreditAttribution: geekygnr at Lullabot commented

The issue described in #3212975-24: Olivero: Normalize JavaScript selectors in messages.es6.js bothered me as well and I wanted to dig into it.

The extra text is being added from a visually-hidden h2 that is injected via JS.

You can find the code for this in core/themes/olivero/js/messages.js (https://git.drupalcode.org/issue/drupal-3212975/-/blob/3212975-olivero-n...)

Since this text is known and controlled programmatically I would argue that its existence should be a part of a test. This also creates a bit of a rabbit hole of whether or not visually-hidden aspect of the text should also be tested and if the line between what is visually-hidden and not visually-hidden should be tested. The simple answer would be to alter the expected text to include the message label, the more complex answer would be to have a check in the test to confirm what is visually hidden and what isn't.

I am out of time for now but I can try to take a look at it later in the week. I would also support this being broken off into another issue and marking this as RTBC.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel at Lullabot commented

Since this text is known and controlled programmatically I would argue that its existence should be a part of a test. This also creates a bit of a rabbit hole of whether or not visually-hidden aspect of the text should also be tested and if the line between what is visually-hidden and not visually-hidden should be tested. The simple answer would be to alter the expected text to include the message label, the more complex answer would be to have a check in the test to confirm what is visually hidden and what isn't.

Thanks for looking at this!

Feel free to open up a followup issue. TBH, this issue is just about changing the JS selectors and making sure it works, and IMO we've already had some significant scope creep.

mtift’s picture

mtift
he/him
Primary language English
Location Minnesota, USA
CreditAttribution: mtift as a volunteer and at Lullabot commented
Status: Needs review » Reviewed & tested by the community

This MR looks fairly straightforward. I think it's ready,so setting it back to RTBC.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Reviewed & tested by the community » Needs work

Posted one minor comment on the MR.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel at Lullabot commented
Status: Needs work » Needs review

Re-twiggerized the twig. Needs review once again on this commit https://git.drupalcode.org/project/drupal/-/merge_requests/943/diffs?com...

andy-blum’s picture

andy-blum
he/him/his
Primary language English
Location Ohio, USA
CreditAttribution: andy-blum commented
Status: Needs review » Reviewed & tested by the community
Related issues: +#2942404: Messages should have role=status instead of role=contentinfo

This looks good for the scope of this issue. Note that we may need to update that twig file for accessibility reasons per https://www.drupal.org/project/drupal/issues/2942404

  • lauriii committed 5698e6f on 9.3.x 
    Issue #3212975 by marcoscano, mherchel, thejimbirch, Gauravmahlawat,...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Reviewed & tested by the community » Fixed

Committed 5698e6f and pushed to 9.3.x. Thanks!

Status: Fixed » Closed (fixed)

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