Standardize the behavior of links when Outside In editing mode is enabled

This is partially a result of the fact that even if links are disabled, their hover states are not. This leads to the mistaken impression that what you are clicking to open the off-canvas tray is the "Add content" link when in fact you are clicking the entire block.

We need to find a way to disable all hover states. Something like:

#main-canvas link:hover {
  pointer-events: none;
}

This patch contains #2786459: "Offcanvas" tray should be using the existing dialog system because it hasn't been committed yet.
Also uploading a do-not-test.patch with only changes for this issue.

This patch

1. Disables all form elements on the page(inside #main-canvas) when going into Editing mode
2. Re-enables all form elements on the page when leaving edit mode.
3. Avoids disabling already disabled elements to avoid accidently enabling them.
4. Makes sure all links inside #main-canvas are prevented from leaving the page.

For item 4 we weren't apply the outside_in logic to the main content block because it doesn't have contextual links to edit the block. This is still the case but now links in the main content area will not leave the page in edit mode.

This adds functionality that @xjm suggested here #2783509-10: Outside In's "Edit" toggle lets you do just about everything except edit your content.

I still think the biggest quick win would be providing the Outside In interactive JS on the main content block (not the region), but making the onclick behavior trigger Quick Edit (same as its contextual link) instead of a sidebar. Could we try a rough implementation of that and see if it is viable?

Adding it to this issue because seems to relate more.

Basically it does this by:

1. first checking if the link should be covered by the outside_in pattern.
2. If not looking at it's parents to see if it is another contextual region.
3. If it is then click the link provided by the quickedit module if available.

I also added the functionality to close the Quickedit module editor when you open the offcanvas tray.

This patch contains #2786459: "Offcanvas" tray should be using the existing dialog system because it hasn't been committed yet.
Also uploading a do-not-test.patch with only changes for this issue.

Important: I realize now that will need to separate better now in the .js files what is outside_in and what is offcanvas re: #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it
Nope I was wrong this functionality is in outside_in.js where it should be.

@tedbow

Looks good when I test it.

The only thing i notice is that the hover states are still there (see #3).

@tkoleary thanks for taking a look.

Now that #2786459: "Offcanvas" tray should be using the existing dialog system has landed here is patch without those changes.

It is the same as #8 except with the no pointers css suggested in #3

This what I ended up adding

#main-canvas.js-outside-in-edit-mode a,
#main-canvas.js-outside-in-edit-mode input{
  pointer-events: none;
}
#main-canvas.js-outside-in-edit-mode .contextual-links a {
  pointer-events: inherit;
}

It still has works with contextual links.

This patch also contains the fix for #2787135: Editing mode gets "stuck" after switching between configuration of different blocks, was the on purpose?

@@ -120,7 +158,7 @@
    */
   Drupal.behaviors.toggleActiveMode = {
     attach: function () {
-      $('.contextual-toolbar-tab.toolbar-tab button').on('click', function () {
+      $('.contextual-toolbar-tab.toolbar-tab button').once('outside_in').on('click', function () {
         setToggleActiveMode();
       });

This patch also contains the fix for #2787135: Editing mode gets "stuck" after switching between configuration of different blocks, was the on purpose?

@cashwilliams thanks no that was a mistake. Fixed

I also realize I lost some changes from #8 related to " Quickedit module editor ". So doing interdiff from there.

1. +++ b/core/modules/outside_in/js/outside_in.js
   @@ -75,6 +91,40 @@
   +        $el.attr('data-outside-in-disabled', true);
   

   In some instances we are setting a class js-* to signal that we've modified things with JavaScript. Here we are setting a data attribute.

2. +++ b/core/modules/outside_in/js/outside_in.js
   @@ -75,6 +91,40 @@
   +    $('*').removeAttr('data-outside-in-disabled');
   

   Instead of selecting all the elements, refine the selector to just get the elements with the correct data attribute.

@drpal thanks for review.

In some instances we are setting a class js-* to signal that we've modified things with JavaScript. Here we are setting a data attribute.

All those a class js-* also are used so that css can reflect the change. In this case we strictly just need to remember which elements we have disabled so we know which ones to re-enable. We don't want to enable ones that were disabled when the page loaded or by some other Javascript.

Instead of selecting all the elements, refine the selector to just get the elements with the correct data attribute.

@drpal good point. Figured I could just chain onto previous line so now it is.
$('*[data-outside-in-disabled="true"]').prop('disabled', false).removeAttr('data-outside-in-disabled');

Talked to @drpal and figured out we could change
$('*[data-outside-in-disabled="true"]').prop('disabled', false).removeAttr('data-outside-in-disabled');
to
$('[data-outside-in-disabled]').prop('disabled', false).removeAttr('data-outside-in-disabled');
to be more efficient.

Just one small point that is not critical. This looks good otherwise.

+++ b/core/modules/outside_in/js/outside_in.js
@@ -75,6 +91,39 @@
+    $('[data-outside-in-disabled]').prop('disabled', false).removeAttr('data-outside-in-disabled');

Setting the attribute to false might be redundant if we are just going to remove the entire data attribute.

Setting the attribute to false might be redundant if we are just going to remove the entire data attribute.

@drpal looking at the jquery docs for attr and prop jquery treats properties like disabled differently from attributes like data-*
From http://api.jquery.com/attr/

To retrieve and change DOM properties such as the checked, selected, or disabled state of form elements, use the .prop() method.

You can actually see in the the DOM that disabled property is actually removed when you call prop('disabled', false).

We used disabled to actually disable the form elements. data-outside-in-disabled is just used to remember which ones were disabled by use(as oppose to already being disabled or by other js).

If we just did .removeAttr('data-outside-in-disabled'); the elements would still be disabled.

@tedbow

Just took a quick look at this an it looks like an unintended side effect of this patch is that the "place block" links are disabled when both place block and outside-in are installed and the user is in edit mode with place block active.

I *think* this might be because place block is not using the contextual class, which it should.

Perhaps this should be a bug on place block? Seems odd that we should need to make an exception for place block links.

@tkoleary I created a follow up issue #2796135: Allow modules to designate certain links should not be disabled during Edit mode

I *think* this might be because place block is not using the contextual class, which it should.

I don't think they should be using the contextual class.

The follow up issue would let module designate certain links should not be disabled in edit mode.

@tedbow

The follow up issue would let module designate certain links should not be disabled in edit mode.

Perfect.

@Jigar.tanna there is no need to tag issues with "drupal core", I have seen you add this tag to a few issue today. Drupal.org issue are in projects. Since this one is in "Drupal core (3060)" that is what determines it deals with "drupal core".

You can read about tagging issues here: https://www.drupal.org/node/1023102

By that way would be interested in your help on Outside In issues if you have the time and desire.

how about this?

  document.getElementById('main-canvas').addEventListener('click', function (e) {
    e.preventDefault();
    e.stopPropagation();
    console.log('action!');
  }, true);

Less side effects than changing the elements' attributes. (Imagine that if a module's script monitoring the attributes.)

If the patch is the only workaround, considering dispatch an event to inform other modules we are going to do crazy actions. Please stay away!

By the way, patch doesn't meet ESLint

@droplet thanks for the review

If the patch is the only workaround, considering dispatch an event to inform other modules we are going to do crazy actions. Please stay away!

This issue is not workaround. It is just fixing some problems in the initial("experimental") commit of the module. The plan was always to disable ways to leave the page in "Editing" mode

But yes we should dispatch events. I created a follow up #2797873: Dispatch Javascript events on Editing clicks and other events

Less side effects than changing the elements' attributes. (Imagine that if a module's script monitoring the attributes.)

Good call! For some reason the disableForms and enableForms functions where previously need or they would take the user to another. But I checked and it seems other changes in this issue has fixed the problem. I removed both disableForms and enableForms now.

The code for preventing the clicks is little move more complicated than your example code in #26 but basic idea is the same.
The things that make it more complicated are:

1. Any click inside an element that has a outside_in.module contextual link should invoke that link
2. Contextual links and buttons should still work as normal
3. If you click in an element that is not inside an outside in editable but is inside an element that has quickedit.module contextual links then the quickedit.module link should be invoked

By the way, patch doesn't meet ESLint

Sorry about that, turns out I did not have ESLint setup correctly for the project. I have it setup correctly now. I saw the errors but I think the ESLint problems where in code that I removed now.

The code for preventing the clicks is little move more complicated than your example code in #26 but basic idea is the same.

Yeah, I assumed you will copy the complicated code over there. There's a difference:

My example code is event capturing rather than event bubbling (in jQuery). We dropped old IEs in D8, it's safe to use it :)

We captured it at topmost layer (outside_in adding hover styling there `.outside-in-editable`) and don't let it go down to links & form's inputs...etc

that's fine if #27 is working also . :)

My example code is event capturing rather than event bubbling (in jQuery).

@droplet thanks for clarification. I have mostly been doing php for the last 4 years so if I had known that difference here I forgot. I read a tutorial and this makes sense now.

@droplet, since this issue is to "Standardize the behavior of links " and this patch does that can we move this idea to #2785589-22: Fix js and jsdoc of outside-in module which is broader issue about JS fixing? I have already quoted your idea there.

Changing this back to RTBC.

Follow ups have been created for 2 new comment since previous RTBC.

@tkoleary #19 #2796135: Allow modules to designate certain links should not be disabled during Edit mode

@droplet #26 moved this idea to #2785589-22: Fix js and jsdoc of outside-in module

This patch updates the changes now that #2785589: Fix js and jsdoc of outside-in module is committed. Functionally it should be the same. I also update OutsideInBlockFormTest to test clicking on links inside the blocks in Edit mode to make sure it opens the block form in the offcanvas tray.

I am getting this error again

1) Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest::testBlock
Zumba\GastonJS\Exception\BrowserError: There was an error inside the PhantomJS portion of GastonJS.
This is probably a bug, so please report it:
click

Which might be particular to my local setup so I am going to upload to retest.

I think we should update the tests in this issue or a follow up. The JS testing seems pretty buggy to me so I am not sure we will be able to get all this but if we could in Edit mode we should test:

1. Clicking anywhere in on the main node view opens the Quick Edit toolbar and closes an open offcanvas tray
2. Clicking on form elements in blocks open the block form in off
3. Opening the offcanvas try closes the Quick Edit toolbar if open
4. Exiting edit mode closes the offcanvas tray if open
5. Exiting edit mode closes Quick Edit toolbar if open
6. Reloading the page in Edit mode stays in Edit mode

Ok this patch fixes the GastonJS bug by using Javascript to trigger the click. I don't like this but don't see another to get around this GastonJS bug.

Ok this patch adds all of the test coverage I suggested in #32.

I think getting this test coverage in is import because even though this an experimental module we can be more confident going forward that aren't breaking existing functionality.

Postponing b/c #2785589: Fix js and jsdoc of outside-in module got reverted.
That issue should go in first

ok so #2785589: Fix js and jsdoc of outside-in module landed. Yay! thanks for everyone who worked on that!

2782915-37.patch is a reroll or #35 there were some big changes so couldn't do an interdiff.

I am getting some real strange local test errors so I am uploading 2 versions.

2782915-37.patch - is a re-roll with everything that was needed. It fails for me locally with

1) Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest::testBlocks
Zumba\GastonJS\Exception\BrowserError: There was an error inside the PhantomJS portion of GastonJS.
This is probably a bug, so please report it:
click
html.touchevents.details.js body.layout-one-sidebar.layout-sidebar-first.user-logged-in.path-user.toolbar-fixed.toolbar-horizontal div#main-canvas-wrapper div#main-canvas.js-outside-in-edit-mode div#page-wrapper div#page div#main-wrapper.layout-main-wrapper.layout-container.clearfix div#main.layout-main.clearfix div#sidebar-first.column.sidebar aside.section div.region.region-sidebar-first div#block-powered.contextual-region.outside-in-editable.block.block-system.block-system-powered-by-block div.content

/var/www/d8_2_ux/vendor/jcalderonzumba/gastonjs/src/Browser/BrowserBase.php:122
/var/www/d8_2_ux/vendor/jcalderonzumba/gastonjs/src/Browser/BrowserBase.php:99
/var/www/d8_2_ux/vendor/jcalderonzumba/gastonjs/src/Browser/BrowserMouseEventTrait.php:17
/var/www/d8_2_ux/vendor/jcalderonzumba/mink-phantomjs-driver/src/MouseTrait.php:31
/var/www/d8_2_ux/core/tests/Drupal/Tests/BrowserTestBase.php:1527
/var/www/d8_2_ux/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php:222
/var/www/d8_2_ux/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php:116

2782915-37b-no-pointer-css.patch is the same re-roll except with the changes to outside_in.module.css that were just to pointer-events:.
This patch passes locally for me.

I have no idea why but maybe it is a bug in GastonJS like error suggests. Or maybe they will both pass or neither ;)

I also uploaded an interdiff between the 2 patches.

Umm. PointerEvents changed the click target that PhantomJS didn't expect I think. And no idea how well the PointerEvents working inside PhantomJS

My little thoughts:
A: It's known issues on testbots. We muted the PointerEvents.
B: Before use .triggerClick in above patches, I'd try jquery-simulate first. It sends a MouseEvents which same to gastonjs's API (non-behat-mink standard API):

  public function clickCoordinates($coordX, $coordY) {
    return $this->command('click_coordinates', $coordX, $coordY);
  }

But we can't get the X & Y from our test cases.

@droplet this I think this is simpler solution I created a new test module, outside_in_test_css, which just adds this CSS to the all pages:

#main-canvas.js-outside-in-edit-mode a,
#main-canvas.js-outside-in-edit-mode input{
    pointer-events: inherit !important;
}

It just overrides the CSS pointer-events from the module. We aren't testing the pointer-events CSS which is simply visual effect. This fix lets us just us the standard click method for testing.

I saw @tedbow's tweet and felt compelled to give this a crack. I tried running the tests in two different environments and keep getting the following:

There was 1 failure:

1) Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest::testQuickEditLinks
Javascript condition met:
(jQuery('#quickedit-entity-toolbar').length > 0)
Failed asserting that false is true.

This is failing on the line 166 of OutsideInBlockFormTest.php:

      $this->waitForElement($quick_edit_selector);

This is happening right after we are clicking the body selector. It feels like this may have to do with what you guys are referring to above but I am not yet super familiar with these types of tests to offer a solution.

Other then that the functionality seems to be working.

default timeout time is too short, that's a cause may be.

I think you may be right.

Although, I have been playing with different timeout values. I went up to 10 seconds and still not getting reliable passes. Its very sporadic.

This is what I am doing:

$this->waitForElement($quick_edit_selector, 10000);

Is this passing no problem for you guys?

@nickgs thanks for testing. It is working for me. I am retesting #40 just to be sure it works on DrupalCI consistently.

I don't think we should raise the default timeout in unless we find it is consistently not working. Because it gets called so many times this would really increase test time.

@nickgs also 1 thing that use to give me random result was the way I was invoking phantomjs. Are you running from command line? How are you invoking phantomjs?

Mine is
phantomjs --ssl-protocol=any --ignore-ssl-errors=true vendor/jcalderonzumba/gastonjs/src/Client/main.js 8510 1024 768

If you are getting errors with 10 second delay I feel it is something else.

changing to "Needs Review" until we make sure it is not a local testing issue.

@nickgs,

Capture the screenshot before & after that click

And try to remove the foreach loop

It's a true headache. #48 shown another random error but I didn't touch that line

Thanks guys,

@tedbow That is exactly how I am invoking PhantomJS.

@droplet Good call on taking some screen shots and removing the foreach. Playing with this a bit I can confirm that the second node load is causing the failure. Being that we are not in edit mode on second load that seems to be why the assertion is not passing. Using the screen shots I can clearly see this.

I very well may be missing something but shouldn't we test that the quick edit links do NOT show when we click the $body_selector and are NOT in edit mode? If so the conditional would look something like this:

      if($page_load_times == 1) {
        // In Edit mode clicking selector should open QuickEdit toolbar.
        $this->waitForElement($quick_edit_selector);
      }
      else {
        // When not in Edit mode QuickEdit should not be open.
        $web_assert->elementNotExists('css', $quick_edit_selector);
      }

I am going to give a patch a shot here. Attached is a patch implementing this.

Being that we are not in edit mode on second load

I could confirm it too.

In bad case, a simple sleep(2) would save us to write huge amount of test code.

another dirty patch for the bots.

+++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php
@@ -119,16 +124,94 @@ public function testBlocks() {
+      // Waiting for Toolbar animation.
+      $this->getSession()->wait(10000, '(typeof(jQuery)=="undefined" || (0 === jQuery.active && 0 === jQuery(\':animated\').length))');
...
+    // Waiting for QuickEdit icon animation.
+    $this->getSession()->wait(10000, '(typeof(jQuery)=="undefined" || (0 === jQuery.active && 0 === jQuery(\':animated\').length))');
...
+    // Waiting for Toolbar animation.
+    $this->getSession()->wait(10000, '(typeof(jQuery)=="undefined" || (0 === jQuery.active && 0 === jQuery(\':animated\').length))');

It seems working now. I will tidy up it later or anyone can help me? Thanks.

This is cleaning up @droplet's patch. Basically swapping in \Drupal\FunctionalJavascriptTests\JSWebAssert::assertWaitOnAjaxRequest for the code he highlighted in #54 which is actually the same.

Everything else looks good. @nickgs and @droplet thanks for figuring this out!

Thank @tedbow. All Looks good now. I leave it as Needs review for one more eyes

The tests and JS look right, but I really am not okay with signing off on the JS.

Here are some nits instead!

1. +++ b/core/modules/outside_in/css/outside_in.module.css
   @@ -47,3 +47,12 @@
   +#main-canvas.js-outside-in-edit-mode input{
   
   +++ b/core/modules/outside_in/tests/modules/outside_in_test_css/css/css_fix.theme.css
   @@ -0,0 +1,4 @@
   +#main-canvas.js-outside-in-edit-mode input{
   

   Needs space before {

2. +++ b/core/modules/outside_in/js/outside_in.js
   @@ -85,6 +86,20 @@
   +   * Disable the QuickEdit module editor if open.
   ...
   +   * Close/remove offcanvas.
   

   Disables, Closes/removes

3. +++ b/core/modules/outside_in/tests/modules/outside_in_test_css/css/css_fix.theme.css
   @@ -0,0 +1,4 @@
   +    pointer-events: inherit !important;
   

   Indented too far

4. +++ b/core/modules/outside_in/tests/modules/outside_in_test_css/outside_in_test_css.libraries.yml
   @@ -0,0 +1,7 @@
   +
   +
   

   Extra lines

5. +++ b/core/modules/outside_in/tests/modules/outside_in_test_css/outside_in_test_css.module
   @@ -0,0 +1,9 @@
   +<?php
   +
   +/**
   + * Implements hook_page_attachments().
   + */
   +function outside_in_test_css_page_attachments(array &$attachments) {
   

   These are still supposed to have @file

6. +++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php
   @@ -119,16 +124,93 @@ public function testBlocks() {
   +   * Test QuickEdit links behavior.
   

   Tests

7. +++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php
   @@ -119,16 +124,93 @@ public function testBlocks() {
   +    $node = $this->createNode(
   +      [
   ...
   +        'body' => [
   +          [
   

   That's a lot of blank lines, I'd tighten that up

8. +++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php
   @@ -119,16 +124,93 @@ public function testBlocks() {
   +      // TODO: Remove the hack after https://www.drupal.org/node/2542050.
   

   @todo Remove...

   (no colon or caps)

@tim.plunkett thanks for review.

This addresses all the nits in #57

On the JS side, this looks good to me.

Failed b/c of unrelated test.

Still need works:

+  function disableQuickEdit() {
+    $('.quickedit-toolbar button.action-cancel').click();
+  }
+
+  /**
+   * Closes/removes offcanvas.
+   */
+  function closeOffCanvas() {
+    $('.ui-dialog-offcanvas .ui-dialog-titlebar-close').trigger('click');
+  }

always use trigger() on jQuery objects, never the alias.

@nod_ thanks for review. Here is a fix. also TESTS_ONLY patch.

just re-roll

Changes look fine. Pre-RTBC assuming tests pass.

23:19:25 Slave went offline during the build
23:19:25 ERROR: Connection was broken: java.io.EOFException

Strange error saw the same on another issue.

Retesting

Looks like #69 was cause by a testing infrastructure failure. Setting back to RTBC tests have continue to pass after that.

Re-roll and changing to 8.3.x

Because this new test method testQuickEditLinks might have the same random fail potential as #2830485: \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest fails randomly

I will also in another comment attach a patch that simply runs this test 50x.

Here is patch to check random file by running this test 50 times.

Updating the summary to describe what this issue fixes.

Many of the original problems were fixed during #2785589: Fix js and jsdoc of outside-in module so they should not be included here.

Ok setting back to RTBC since this was just a re-roll.

Current patch is #75. #76 was just to check ran 50 times there would be random file. No fail.

Manual testing in chrome. Things are working as I would expect after reading the issue summary. See screencast http://recordit.co/ADBnxIflsh

@alexpott thanks for looking at this issue and providing the screencast.

Yes I can confirm this problem. This seems to happen when you use the contextual links instead of just clicking the sections of the site in Edit mode.
Right now it is difficult to test contextual links. There are no tests for contextual links in core. Trying to solve that here #2821724: Create Javascript Tests for Contextual Links. this issue also provides a trait to make it easier to test.

Ok this patch fixes the problems @alexpott documented in #80 and adds testing for it.
I added the functions from #2821724: Create Javascript Tests for Contextual Links with @todo's to remove them when the trait is added in that issue.

The problem was that when the "Quick Edit" links from Settings Tray and QuickEdit module were clicked the new behavior was not triggered. This fixes that.

Ok the test failure is just same 1 8.3.x branch is having randomly.

So setting to "Needs Review" to get eyes on it.

This looks to be behaving correctly.

Ok the random test failure is just same 1 8.3.x branch is having randomly.

All right! Tested this back a couple of weeks ago when the random testbot failures were screwing up the RTBC queue. Looks like alexpott also tested and his feedback was addressed, and nod_'s JS review was taken into account. No further bloickers.

Committed 5dbf5ab and pushed to 8.3.x. Thanks!

AHEM. It super helps when you remember to push. :P

Committed 07d2ae1 and pushed to 8.3.x, then cherry-picked to 8.2.x. Thanks!

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