Set toolbar warning message to only appear on admin/edit pages

Since this is a beta blocker, escalating to major.

I'll work on this. I'll change the colour to #78520D (with #fdf8ed in the background passes AA with 6.5:1).

And here's the small patch.

Ok Mark! Then this visual changes need to be defined. Updating the issue and unassigning in case anyone wants to work on this before I get time.

Review of patch #8

1. Colour contrast.
The colour of the message is #78520d on #fdf8ed, which has a contrast ratio of 6.6:1, which satisifies WCAG at level AA.

However we aren't using this colour combination anywhere else in Drupal core. The existing style for warning messages in Seven and Classy is slightly stronger: #734c00 on #fdf8ed. This has a contrast ratio of 7.2:1 which satisfies WCAG at level AAA. Can we re-use this combination please?

2. Identifying an admin page
+ if (strpos(\Drupal::service('path.current')->getPath(), 'admin')) {

By testing the path like this, it doesn't match pages like the node edit form, where this message would be very useful.

Here's another way to test for admin pages. It matches the node add/edit pages, and others like devel/php too. It's done this way in the patch at #2934374: Add permanent warning message for experimental profiles to avoid its usage on production sites.

$admin_context = \Drupal::service('router.admin_context');
$route_match = \Drupal::routeMatch();
if ($admin_context->isAdminRoute($route_match->getRouteObject())) {
  drupal_set_message('route is admin route');
}

3. Something is up with caching. It might be something to do with the toolbar module itself, or maybe we can set cache metadata in individual toolbar items? The odd behaviour I found was that the message sometimes appeared on the node edit form, sometimes not. Clearing cache using Drush, then reloading the node form resulted in no message being displayed on the node edit page.

Sorry, but I'd say to remove the color change from this issue and do it on the Design issue. I'll move Andrew's comment there.

-    color: #000;
+    color: #734c00;

Re: #14-15

I think @ckrina is right. Better to take the colour change out of scope here, rather than commiting it in haste.

Updating tasks in issue summary.

At least, I thought I updated the tasks in the summary.... done now.

Oops, I failed HTML 101 exam in my last edit.

Patch in #20 looks good to me. Manually tested umami-themed content pages, node forms, and generic admin pages. Message also devel/* routes so the is-admin test is correct.

+++ b/core/profiles/demo_umami/demo_umami.profile
@@ -33,19 +33,24 @@ function demo_umami_toolbar() {
+  $admin_context = \Drupal::service('router.admin_context');
+  $route_match = \Drupal::routeMatch();
+  if ($admin_context->isAdminRoute($route_match->getRouteObject())) {

I don't think we need the inline variables here, and admin context will use route match if no route is passed

so we should only need

if (\Drupal::service('router.admin_context')->isAdminRoute() {

Also, we need a test for this.

We already have an existing test that visits a node-edit form, so we can assert the message is present there.
see \Drupal\Tests\demo_umami\Functional\DemoUmamiProfileTest::testEditNodesByAdmin

Sounds like a needs work. :)

Thanks, d.o has been eating my updates all morning

Edited the issue summary to mark *all* visual issues as deferred to #2938800: Finalize wording for Toolbar warning message, to make the remaining element easier to review.

I've tested the patch by navneet0693 in #25.

The patch works as designed but we need better tests.

We should move the test for checking the "demonstation" text to a new test and test the following:

• the message does not appear on the home page
• the message does not appear on a recipe page
• the message does appear on a recipe edit page
• the message does appear on a /admin/... page

Added a new patch to implement tests suggested by John Cook in 27, and moved to their own test rather than reusing the edit test.

Also removed static reference to toolbar in DemoUmamiProfileTest::modules static variable; afaik that should be mandated by the umami_demo profile anyway.

The list of assertions (pages to visit) look reasonable.

Nitpick for comment style:

/* @var $recipe_node \Drupal\node\Entity\Node */

The preferred form seems to be /* @var \Drupal\node\Entity\Node $recipe_node */. It's proposed at #2305593: [policy] Set a standard for @var inline variable type declarations, #2305641: Use the "standard" format for @var inline variable type declarations, and PSR-5, but these are all still at the draft stage. The majority of instances in D8 core already follow the proposal (type first, then name).

I've got the tests passing - at least locally.

The problem was that the toolbar block was being cached when the tests were running but not when viewing the page. I've put in a cache context for the route.

I've also fixed Andrew's nitpick.

I think this needs work:

There is an empty tab at the end of the toolbar items (noticeable due to the bug causing green hover over effects on toolbar items):

I think this is because we always have:

$items['experimental-profile-warning'] = [
     '#type' => 'toolbar_item',
-    'tab' => [
+    '#weight' => 999,
+    '#cache' => [
+      'contexts' => [ 'route' ],
+    ],
+  ];

But the tab inside it is conditional:

+  // Show warning only on administration pages.
+  $admin_context = \Drupal::service('router.admin_context');
+  if ($admin_context->isAdminRoute()) {
+    $items['experimental-profile-warning']['tab'] = [
       '#type' => 'inline_template',
       '#template' => '<a class="toolbar-warning" href="{{ more_info_link }}">This installation is for demonstration purposes only.</a>',
       '#context' => [
@@ -44,8 +54,7 @@ function demo_umami_toolbar() {
       '#attached' => [
         'library' => ['demo_umami/toolbar-warning'],
       ],
-    ],
-    '#weight' => 999,
-  ];
+    ];
+  }
   return $items;

I think the whole item, not just the tab, needs to be in the check for admin route. Otherwise the toolbar item gets rendered, without the child tab content.

I haven't tested that theory though - I will if I get a chance.

I was concentrating on getting the tests to pass.

If the ['#cache']['context'] isn't returned then the toolbar block gets cached and the tests fail.

I've moved the type into the conditional as well. This prevents the issue that @smaz found in #32.

An empty toolbar item (role) is a no-no for accessibility

Found a tiny coding standards issue.

Installing with #35 - have successfully checked the tests pass locally, and that no html element is inserted for the empty tab on pages that do not display the warning.

I did experiment with moving the entire definition of $items['experimental-profile-warning'] but that causes a test to fail, presumably because it breaks the caching.

So this looks good - I'd set it RTBC but I've probably done too much on it myself.

@andrewmacpherson the patch in #33 removes the empty toolbar item.

The following show the toolbar mark-up with the "admin" item selected.

/admin/content

/

So the (empty) toolbar item isn't rendered using that patch.

Another nitpick:

+  $items = [];

We do not need to declare $items in advance - no other core implementations of this hook do so (example from the toolbar module).

   public function testEditNodesByAdmin() {
-    $account = $this->drupalCreateUser(['administer nodes', 'edit any recipe content']);
+    $account = $this->drupalCreateUser(['administer nodes', 'edit any recipe content', 'access toolbar']);

I don't think we need to add the 'access toolbar' permission here for just testing editing nodes - I think that should only be in testDemonstrationWarningMessage(). Unless that was needed for some reason @John Cook?

I tested the empty tab issue & can confirm this is now resolved.

@smaz,

@eli-t and I adapted the test from @navneet0693 in #25. We moved the assertion but didn't notice the added permission, so it's not required in that test.

Adding credits for those who reviewed and impacted the final patch

Committed as 41baf34 and pushed to 8.6.x

Is this via module ? i am looking for same functionalities please suggest me Module for that