Outside-in Accessibility: Allow escape from edit mode with ESC key

I'm at the Midwest developers summit. I'm going to try to do some JS to fix it.

This is my first time working on a core issue.

Don't spend too much time on this, the related issue fixes this problem. Might be better to give the other patch a review or continue the clean-up.

OK. I'll focus effort there instead.

#7 only seems to toggle toolbar's edit/editing toggle visually. tray does not close. needs work

I wonder whether @nod_ should update us on the status of this re: using the dialog system. If this issue is moot, we should close it.

Re #9
Once #2786459: "Offcanvas" tray should be using the existing dialog system is committed ESC will close the offcanvas dialog. It will not exit out of editing mode.

I am not sure what the best behavior is.

Maybe if dialog open close dialog but stay in "editing" mode(this would happen now)

If dialog is not open exit "editing" mode.

In that case #7 could just check if dialog is currently open.

if this is to be tackled from the current outside_in module...
   interactions of the tray and the toolbar are 2 separate behaviours.. an ESC would need to exist for both script sets.

Re #10:

Thanks for that information. That was really helpful.

I would love to work on this issue, but I guess at this point, I'm still confused about where work needs to happen. It sounds like #7 was wasted effort due to less-than-ideal issue hygiene, and it would be really discouraging if more effort was spent on this due to confusion about whether this issue still needs work or not, and ambiguity about what code to work against. It looks like #11 has the same confusion I have.

So again, to avoid wasted effort and ambiguity, I'm not sure what if any work needs to be done here, and what base to work off of. But form my perspective, it looks like this issue should be closed — and another one opened? Perhaps another one should be opened once the commit you're referring to occurs? And perhaps a better issue summary?

re nod_ in #5

Don't spend too much time on this, the related issue fixes this problem.

#2786459: "Offcanvas" tray should be using the existing dialog system fixes closing the offcanvas try but exiting Outside In edit module(which is disables links/forms etc)

@miteshmap's patch will exit edit mode but also will enter edit mode.

I am updating with small change to check if the site is currently in Edit mode.

Re @SKAUGHT in #11

if this is to be tackled from the current outside_in module...
interactions of the tray and the toolbar are 2 separate behaviours.. an ESC would need to exist for both script sets.

Yes that sounds right.
Once #2786459: "Offcanvas" tray should be using the existing dialog system lands ESC will close the offcanvas tray b/c the existing dialog system works this way.

This patch will make it so hitting ESC will exit Edit mode.

So with the patch and #2786459: "Offcanvas" tray should be using the existing dialog system 1 time pressing ESC will both close the tray and exit Edit mode.

re @miwayha

So again, to avoid wasted effort and ambiguity, I'm not sure what if any work needs to be done here

Could you review the patch and confirm that it exits Outside In edit mode? Currently the tray will remain open until #2786459: "Offcanvas" tray should be using the existing dialog system lands. thanks

#2786459: "Offcanvas" tray should be using the existing dialog system just got committed.

Patch in #13 still applies. It should now close offcanvas and exit edit mode at the same time.

This looks good to me.

I'd rather we use the API instead of ad-hoc code.

+++ b/core/modules/outside_in/js/outside_in.js
@@ -146,5 +146,11 @@
+  ¶

Removed the extra spacing.

@nod_ the patch in #16 exits "Edit" mode whenever you close the tray.

#13 exits the "Edit" mode only when you close the tray via ESC. Maybe that was bad idea but we do want to remain in "edit" mode when tray is closed.

So maybe for ESC we should go with

if dialog open close dialog but stay in "editing" mode(this would happen now)

If dialog is not open exit "editing" mode.

So if you had a tray open, first ESC closes tray, second ESC exits "Edit" mode

see #18.

i'm not clear on which patches to apply at this point. it seems they are all individual commits.

I talk with @nod_ on irc about patch in #16(#17 is the same but with spacing fix)

The original UX design in #2762505: Introduce "outside in" quick edit pattern to core should that you don't exit "Edit" mode every time you close the tray. That is what the patch in #16 does.

He also didn't like my idea:

So if you had a tray open, first ESC closes tray, second ESC exits "Edit" mode

But rather prefered

closing tray and exiting edit mode with 1 ESC is better than with 2 subsequent ESC.

(from IRC)
This not possible as far as I can tell with the approach in #16. This because

+++ b/core/modules/outside_in/js/outside_in.js
@@ -146,5 +146,11 @@
+  $(window).on('dialog:afterclose', function (e, dialog, $element) {
+    if ($element.is('#drupal-offcanvas') && isActiveMode()) {

At this point you can't actually tell what key was pressed to close the dialog. I spent some time debugging the event here is the dialog close not the key or click event that closed it. Even if you could figure out the last key pressed and the key press event could have happened before the tray was open.

Re nod_'s comment #16

I'd rather we use the API instead of ad-hoc code.

It doesn't seem we can use the dialog system's API because of the reasons above.

So for

closing tray and exiting edit mode with 1 ESC is better than with 2 subsequent ESC.

This is actually done by my patch in #13 actually does this.

I will upload it again just to be clear, it is the same as #13.

As a screen-reader user, I often press escape to exit "forms mode" or interactive mode. I find that this can have undesired results of dismissing a dialog or other interactive mode of a web application.

The behaviour of not using escape in a particular context can be learned. Could it be worth adding a Drupal.announce to indicate that context has changed? Perhaps this is already happening, I have not tested.

"Exited edit mode" or something similar.

That sounds like a good approach Everett.

@Everett Zufelt my guess would this is already happening but I haven't tested.

Exiting the dialog is the same as closing any core dialog. Drupal.announce I think should already be called.

Exiting "edit" mode is actually controlled by the Contextual module. Settings Tray doesn't create edit mode but enhance it. I am not sure if the Contextual module is using Drupal.announce but it seems like it should be.

If someone could confirm these it would be good.

From my point of view, closing should be 1-on-1 with going out of a mode. Explaining closing + going out of edit mode, is a really though thing from a UI perspective.

• Works against 8.3.x
• Add Drupal.announce() for esc key action

@drpal thanks for patch.

Marking as RTBC.

For those reviewing

ESC exits edit mode.
It will also close the current form but that is from the dialog system.
There is also Drupal.announce called "Exited edit mode"

It'd be great to test this using a JavascriptTetsBase test. One of tests that extend \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInJavascriptTestBase might be a good place for this.

@alexpott good point.

Instead of creating a new test I added to \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest::testBlocks
This has to go in and out of Editing mode to test the functionality. I substituted 1 time with the ESC key and tested for announce also.

+++ b/core/modules/outside_in/js/outside_in.js
@@ -39,6 +39,15 @@
+  $(document).on('keyup', function (e) {

namespace:
keyup.outsideIn

@droplet - Fixed the namespace, and fixed another issue with namespace collision between outsidein and offcanvas.

Just needed a re-roll

I reviewed again. Since I just did a re-roll and didn't most of the work marking as RTBC.

+++ b/core/modules/outside_in/js/offcanvas.js
@@ -47,7 +47,7 @@
 
     $element
       .dialog('option', adjustedOptions)
-      .trigger('dialogContentResize.outsidein');
+      .trigger('dialogContentResize.offcanvas');
   }
 
   /**
@@ -100,15 +100,15 @@

@@ -100,15 +100,15 @@
         $('.ui-dialog-offcanvas, .ui-dialog-offcanvas .ui-dialog-titlebar').toggleClass('ui-dialog-empty-title', !settings.title);
 
         $element
-          .on('dialogresize.outsidein', eventData, debounce(bodyPadding, 100))
-          .on('dialogContentResize.outsidein', eventData, handleDialogResize)
-          .trigger('dialogresize.outsidein');
+          .on('dialogresize.offcanvas', eventData, debounce(bodyPadding, 100))
+          .on('dialogContentResize.offcanvas', eventData, handleDialogResize)
+          .trigger('dialogresize.offcanvas');
 
         $element.dialog('widget').attr('data-offset-' + edge, '');
 
         $(window)
-          .on('resize.outsidein scroll.outsidein', eventData, debounce(resetSize, 100))
-          .trigger('resize.outsidein');
+          .on('resize.offcanvas scroll.offcanvas', eventData, debounce(resetSize, 100))
+          .trigger('resize.offcanvas');
       }
     },
     'dialog:beforecreate': function (event, dialog, $element, settings) {
@@ -127,8 +127,8 @@

@@ -127,8 +127,8 @@
     },
     'dialog:beforeclose': function (event, dialog, $element) {
       if ($element.is('#drupal-offcanvas')) {
-        $(document).off('.outsidein');
-        $(window).off('.outsidein');
+        $(document).off('.offcanvas');
+        $(window).off('.offcanvas');
         $mainCanvasWrapper.css('padding-' + edge, 0);
       }
     }

Why are these changes necessary? I'm guessing this is because:

fixed another issue with namespace collision between outsidein and offcanvas.

But if that is caused by namespacing the keyup event then why not choose a different namespace.

@alexpott thanks for the taking a look at this issue.

UPDATE AND CORRECT EXPLANATION
Because in offcanvas.js we were already performing

$(document).off('.outsidein');
$(window).off('.outsidein');

To remove listeners in offcanvas.js when we added in this patch outside_in.js

 $(document).on('keyup.outsidein', function (e) {
  if (isInEditMode() && e.keyCode === 27) {
      Drupal.announce(
        Drupal.t('Exited edit mode.')
      );
      toggleEditMode();
    }
  });

offcanvas.js was all of sudden removing listeners in outside_in.js which was not intended.
So we switch all event namespaces in offcanvas.js to '.offcanvas'

Offcanvas should work without outside_in.js and not conflict so that eventually we can do #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it

Thanks @tedbow.

Committed 2161fb2 and pushed to 8.3.x. Thanks!

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