Display "You are now in edit mode" prompt when user enters edit mode

Ok here is patch that adds the message. I think will probably want to change the actual message to describe what it is.

I put a todo about refactoring once #77245: Provide a common API for displaying JavaScript messages is committed.

Right now if you click the message it disappears. This is inline JS which I imagine should be changed.

I wasn't sure how to insert it with unknow page markup so I used before main-canvas.

This also includes a test to make sure the message appears and is removed when leaving edit mode.

+++ b/core/modules/outside_in/js/outside_in.js
@@ -148,8 +166,9 @@
-      if (editMode) {
-        setEditModeState(true);
+      // Enter Edit mode if set in local storage and not currently in edit mode.
+      if (editMode && !isInEditMode()) {
+        setEditModeState(true, false);

I added this extra check because displaying the message pointed out the on page load if already in Edit mode setEditMode would be called over and over again. Presumably this is every time a contextual link is added.

I'm not thrilled about having simple string represent complex functionality. Perhaps a better way to construct this and attach the event listener.

@drpal I want you to thrilled about this patch ;)
Moved to an event listener and not adding the element as a big string now.

Ok add anounce() and debounce()

I am not sure how we handle adding the correct css.
Also if you are scrolled down on the page you won't see the message.

I think we should postpone this on #77245: Provide a common API for displaying JavaScript messages
It doesn't make sense to do this twice.

Ok #77245: Provide a common API for displaying JavaScript messages may or may not be committed by the time 8.4.0 comes out.

We can add this here and then refactor if needed.

This mostly a re-roll of #16 plus

1. Necessary es6 changes
2. Removed message when exiting edit mode(can still be clicked on to remove at any time.
3. Updates test coverage.

1. Adding destructing on Drupal announce, ajax, debounce, toolbar, behaviors, attachBehaviors(), t() are used.
2. add more comments
3. Yes eslint picked this up. fixed
4. This selector is only used once. It is called multiple times if the user goes in and out of edit mode. but the message element will be different because is created each time.

Also I had to explicitly add toolbar/toolbar as a dependency to the outside_in library not sure if this has to do with destructing.
UPDATE: figured out why destructing JS change probably cause an error unless a dependency was added for toolbar/toolbar for the oustide_in library.

Since without destructing we didn't pass in Drupal.toolbar explicitly we just passed in Drupal and referenced Drupal.toolbar later, at the time the file loaded the toolbar property didn't have to be on the Drupal object. It only had to be there later when edit mode was invoked. So if toolbar.js was loaded later that would still work.

Using destructing Drupal.toolbar needs to be defined before the file is loaded so toolbar.js must be loaded first.

really toolbar/toolbar should have always been a dependency of oustide_in library but we just never noticed before.
So the really we should dependencies on drupal.announce and drupal.debounce also.

1. Fixed
2. Fixed

Also had needed dependencies to drupal.outside_in. They should have already been there but didn't notice until similar problem found in #21

@droplet thanks for the information. I definitely would never have thought of that.

So this patch doesn't alias Drupal.t.

But this results in
})(jQuery, Drupal, Drupal);
Which looks strange, but it does allow use to do destructiing.

As a follow up I think any Javascript function that doesn't allow aliasing because of something like this should have that clearly documented in the JSDoc. I looked at the JSDoc for Drupal.t and there is no way you would ever figure this out without looking at PHP code.

Ok removing passing in Drupal twice.
#27.2
I added this check

  function displayEditMessage() {
    if ($('#settings-tray-edit-message').length) {
      return;
    }

So the displayEditMessage() won't try to display the method if it is already shown.
Removed dependence on debounce.

RE #27.1 and #28
I look back at the original design issue for this module such as #2762505: Introduce "outside in" quick edit pattern to core and #2732443: Finalize the behavior for triggering view/edit/build modes from the toolbar (and fix the "disappearing toolbar")

The designs call for something that looks more like a dialog with a standard close button.
Here is one of the examples from those issues:

So have changed this to use Drupal.dialog.
So

I think we should make it Drupal.theme

and use prefix with `.js-` so that will not removed by themer..etc

Because we using Drupal.dialog this not longer applies

I think this takes of #28 because we much closer to the original designs that were user tested this way.

Ahh. You using a secret version. haha.

Sorry updated the issue summary.

1. Removed dependency on core/drupal.debounce

2. The tests in #29 failed because now that we are using a dialog it may be in front of other elements that the test is trying to interact with.

I added \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest::closeMessageDialog(). This closes the dialog whenever edit mode is enabled explicitly or when a block for is opened which also may enable edit mode if it is not already enabled.

I added waitForElementVisible() for this to be sure the element was actually displayed before clicking
I also added a waitForNoElement because it could cause random failures on drupalCI if this still in process of being removed before the test proceeds.

3. add CSS to make the dialog appear more like the designs by remove the border and background color for the title bar.

4. We to figure if we need the second part of the message or if we can do that in a follow up.

Ok I added longer message from the original designs. I would rather not debate the text of the this message right now because that is more of UX issue and this issue still needs to figure out the technical aspect of displaying the message first.
I do think we will need a longer message that describes what edit mode is. We can figure out the exact text after we figure out the technical aspects.

But having the longer message allows use to get rid of the special styles for this dialog because we have a title and a message.

Here is screenshot:

Create #2893350: Determine exact wording of "You are in edit mode." message when edit mode is enabled. to picking wording

@drpal I feel we should add a comment instead to every function we cannot alias: #2893358: Add documentation Drupal.t() and other JS functions that cannot be aliased because they are scanned for in PHP

Ok adding comment asked for #35 which includes a @todo point to #2893358: Add documentation Drupal.t() and other JS functions that cannot be aliased because they are scanned for in PHP

I don't think we need to wait on that issue though.

Made a type in that comment. Also a @see to the function that scans for Drupal.t

@yoroy the current patch uses the standard Drupal dialog system(as does the tray itself) so it is not a custom solution in that sense. It is just positioned differently

Could we use that empty space in the toolbar?

The problem I see with that is that we are not sure what might be placed in the toolbar in edit mode in the future. Currently it the Place Button remains in the toolbar. There maybe other other things in the future that would go in the toolbar that only show up in edit mode.

Alternatively, what about stripping the current application down to its bare essentials:

I like this last approach.

The patch in #26 and above was taking similar approach before we started to use the dialog system.

Ok here is a patch that used the dialog system but does not have a special title section

I started from patch #32.

Then added:

1. CSS to remove special title section
2. Comment about destructing from #39
3. Fixed the test so it just clicks on the dialog itself to close it(this is supported by our dialog system)

Ok @drpal reminded me that once #77245: Provide a common API for displaying JavaScript messages lands we will just be to do something like
Drupal.message('#top-message-aread').add('You are in edit mode');

Lets look into standardizing the look and feel of these instead of introducing yet another version.

So once we have a common way to output display message via JS then this look like any other message on the site.

We the could a dismissable behaviour to all our messages whether they come the server or JS. So we could add something like
Drupal.message('#top-message-aread').add('You are in edit mode', {dismissable: true);

The point is that the solution here likely will improved and standardized using #77245.

+++ b/core/modules/outside_in/js/outside_in.es6.js
@@ -115,12 +116,47 @@
+   * @todo Refactor to use Javascript Message API https://www.drupal.org/node/77245

We already have an existing @todo point to this issue in the patch!

Either way, the single line of text in a dismissable dialog that's tucked closely under the toolbar is a good step in the right direction.

So I think our current patch is good then?

@droplet
- Yes, I think the message will improve UX but probably a different message like "Click a page element to edit it." that is why I created #2893350: Determine exact wording of "You are in edit mode." message when edit mode is enabled.. I think if we try to do both the JS implementation and wording in the same issue will never come to an agreement 😜
- I think "Exit Edit Mode" might be a good idea but I think that would in addition to wording that explain what the user should to in edit mode via ^^. So I think this should be a separate issue if we want to change the "Editing" text in the toolbar
- I think it is common pattern on the web to inform the user of something with a dialog and allow them to dismiss the message. I don't think the user expect dismissing the message would reverse what the message informed them of.

@yoroy

we should not introduce yet another styling, even if only temporary.

While agree we don't want to introduce a new styling without reason I don't know that copying the Exit link on the Block demonstration page is the best solution. Is this page actually used much?
The UX study #2497361: [meta] Fix issues found during UMN Usability Testing 2015 found

The "demonstrate block regions" link was completely missed. If that had been used, the user might have better understood block regions.

(#2514150: Advertise the block region demonstration in a more prominent way was opened to solve this but I was surprised to find was closed because Block Place "solves" the demo regions problem)

The fact is the styling provided by the block module for this probably only looks good in bartik. No core theme actually override the styles for the class .block-demo-backlink
Doing a quick google search tells me not many contrib theme actually took the time to style this.
Even if contrib or custom themes did style this because the block module uses a very specific class, .block-demo-backlink, our use would not get the same styling.
So we would either have to:

1. Copy the styling from the block module. If we did this any theme that already overrode .block-demo-backlink to make it look in their theme would immediately have a different styling between "You are in edit mode" and "Exit block demo"(because we can't use .block-demo-backlink). Any theme that didn't override .block-demo-backlink would get the same look but my guess this is because the Block demo page is not a page their users see much so they didn't bother to make it look good in the first place. So they still have to override our styles.
2. Create neutral styling over our own that we think will look good in many themes. Then have defeated the purpose of not introducing a new styling in core.

The fact is right now because #77245: Provide a common API for displaying JavaScript messages we have no common styling for messsage that appear after the user is already on the page because there is common way to do this. The only place I can think of is when you have unsaved changes in form you get a message style warning.

Here are a couple ideas to not introduced a new styling.

Idea #1: Dialog approach

Make this message look more like a standard dialog. Then we are using that a styling that already exists. I have attached patch that does this.

Here is what it looks like:

This is just using our standard dialog but with only the title bar showing, only the positioning has changed. One advantage of this method is that front-end themes will already styled modals/dialogs so we should just pick up the styles that they apply to dialogs and will get a more uniform look.

Here is the same dialog but with the page in narrower width.

You will notice that dialog actually because wider. But again this is because we are using the standard dialog and whatever styles are applied to dialogs will be picked up.

In this way we are not introducing a new styling.

Idea #2: Messages approach

Revert back to the idea in patch #26

This uses existing messages styles but does put them in new place.

If we went this way we would probably want to add a new "X" for closing the message.

Forgot to update the test coverage.

Changing to new settings_tray.module component. @drpal thanks for script help!

Needs reroll at least for changes in #2803375: Rename Outside-in module to "Settings Tray" for real

@Jo Fitzgerald thanks for the re-roll!

re @Wim Leers' idea about

If there were a visual (color) connection between the "Editing" tab in the top left and the blocks you can actually click to edit, this problem would go away.

I do think this an interesting idea but also don't want to just abandon the what was test in UX mockup before module code was ever written. I am not sure but I think we should go back to UX folks/meeting if we want to change something like this. Interested in what @yoroy thinks about this.

So as the patch stands now we are using the dialog system as described in #50. We just need sign off that this is correct way to go to mark this RTBC. In #47 @drpal marked this as RTBC. So I think we are good Javascript wise. In #49 @droplet had some concerns but no problems noted with the code.

In #48 @yoroy raised

we should not introduce yet another styling, even if only temporary.

I now don't think we are introducing new styling as detailed in #50

there were a couple UX concerned raised by @droplet in #49 and @Wim Leers in #50.

Both these are diverging from the UX that was actually tested in mockup with users before this module was written. I don't think we should be doing this in individual issues in the SEttings Tray modules. I think we should implement the UX that was tested.

I think if we are going to change the UX we should do it only after implementing the original UX and seeing that there is something wrong with it from actual users.

Couple remaining things on the patch. Uploading a another patch with these.

+++ b/core/modules/settings_tray/js/settings_tray.js
@@ -54,7 +54,7 @@
-      dialogClass: 'outside-edit-message'
+      dialogClass: 'settings-tray-edit-message'

This change should be have been made to settings_tray.es6.js and then the file should have been built as described here: https://www.drupal.org/node/2815083

1. +++ b/core/modules/settings_tray/js/settings_tray.es6.js
   @@ -60,12 +64,46 @@
   +    $(msgDiv).on('click.outsidein', e => $(e.target).remove());
   

   Need to update click event.

2. +++ b/core/modules/settings_tray/js/settings_tray.es6.js
   @@ -123,7 +165,8 @@
   -      disableQuickEdit();
   +      disableQuickEdit();// Remove edit mode message.
   +      $('#settings-tray-edit-message').remove();
   

   The comment just got moved to after the last line.

I updated the edit message to "Click a page element to edit it." because that is what was determined in #2893350: Determine exact wording of "You are in edit mode." message when edit mode is enabled.

Just re-rolled this patch

1. fixed
2. It returns early if the message div is already on the page. Added a comment before the return statement
3. fixed
4. using destructing now for Drupal.dialog

1. Because jQuery UI Dialogs add inline CSS it is hard to override. Basically we have dialog that only has a title. So we want not to display the content area. I have changed this to use Javascript to set the CSS.

2. Glad to remove these extra changes!
3. change to "Displays" also wrapped at 80. The standard is under discussion at #2924755: Set max line length for JavaScript code comments to 80 (rather than 100)
4. Fixed

Found a problem. I am not a keyboard navigation expert but...
The dialog we use for this is not tabbable. So you cannot close it via the keyboard.

This is because the Contextual module restrains tabbing to the contextual links and the toolbar via:
tabbingContext = Drupal.tabbingManager.constrain($('.contextual-toolbar-tab, .contextual'));

So we could add 'contextual' class to our dialog but that is a hack. Also the contextual module announces a message

Tabbing is constrained to a set of 8 contextual links and the edit mode toggle.
Press the esc key to exit.

So even if were to add our dialog into the tabbing context this message would be wrong.

#69 identifies problem with displaying the dialog message. To fix this we would need non-trivial to contextual module and probably to Drupal.tabbingManager

The problem with doing this is likely that if #77245: Provide a common API for displaying JavaScript messages was finished we would not be using a dialog at all for this message. So we would making changes to contextual for a temporary problem.

I would propose that we postpone this issue on #77245 for these reasons:

1. Not to make changes to contextual and Drupal.tabbingManager
2. We are currently giving the user visual clues they can "Click a page element to edit it.". Namely the dotted lines around the clickable elements and the hover changes for the elements that are clickable

Postponing based on #71

#77245: Provide a common API for displaying JavaScript messages just RTBC'ed

@drpal thanks for the updated patch!
@lauriii yep!
Here is an update to the test to assert the message appears and is removed when exiting edit mode.