Fix js and jsdoc of outside-in module

Backbone part is duplicate of #2784935: Use Backbone for client-side state management

This issue was marked as outside_in.module component because it is new. I updated it

+++ b/core/modules/outside_in/js/outside_in.js
@@ -53,45 +29,67 @@
+      // Close/remove offcanvas.

This is trying to close the offcanvas try on edit mode exit right? We are settings the data-drupal-canvas=close attribute anywhere. I couldn't see how could do this on the php side.

Only where I saw do it would be
in offcanvas.js

$(window).on({
    'dialog:aftercreate': function (event, dialog, $element, settings) {
      if ($element.is('#drupal-offcanvas')) {
$('.ui-dialog-offcanvas .ui-dialog-titlebar-close').attr('data-drupal-canvas', 'close');

But even with that the namespaced event doesn't trigger anything

Talk with @nod_ about #12 and he said that was left over code.

Fixing though I don't see a way to use an attribute there.

This patches adds to \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest a check to make sure that when you exit edit mode the tray is closed also.

1. +++ b/core/modules/outside_in/js/outside_in.js
   @@ -53,45 +29,67 @@
   +    return $('#main-canvas, #toolbar-bar, [data-drupal-outsidein="editable"] a, [data-drupal-outsidein="editable"] button')
   

   Maybe move this selector string with the rest of them.

2. +++ b/core/modules/outside_in/js/outside_in.js
   @@ -53,45 +29,67 @@
   +  function setActiveMode(activeMode) {
   

   Maybe something like setEditModeState.

@drpal good points fixed both those.

Maybe something like setEditModeState.

Yes I changed this and other references from "Active" to "EditMode"

I also update OutsideInBlockFormTest to check that you can click the "Drupal" link in the "Powered By Drupal" block and it will open the block form and not leave the page.

@tedbow

This patch has just a minor formatting issue at the bottom, could you reroll? Thanks. Sorry, error on my end. I haven't had coffee.

@tedbow

Alright, this looks good. After applying this patch I didn't notice any regressions.

@droplet in #2782915-26: Standardize the behavior of links when Outside In editing mode is enabled suggested this change

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

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

Seems like a good idea to me but my only concern is that is search /core/misc and /core/modules I only find addEventListener once but see ".on(" tons. So it seems like the event bubbling is what is done more in core. Then again we are doing something different then most use cases, trying to suppress clicks for most elements.

I am new to JS in Drupal core so I am trying to go by what the current practice is. Maybe that is not a good idea or our use is very different(which it might be).

my only concern is that is search /core/misc and /core/modules I only find addEventListener once

Don't worry about it. It's a historic problem only.

the `domready` function in drupal.js based on addEventListener and it's used in Drupal widely. (and jQuery 2.x also)

Event Delegation (e.g. jQuery.on) still has its advantages but it's okay switch over to addEventListener if it's provided better workaround for particular cases :)

you may interesting in this post also, @see:
https://signalvnoise.com/posts/3137-using-event-capturing-to-improve-bas...

Ok setting to RTBC since @nod_ and @droplet the 2 core JS maintainers seem to be ok with current patch.

A core committer is unlikely to find anything here that those two can't, so committed and pushed to 8.2.x and 8.3.x. :) Thanks!

Hm, sorry, I lied. Committed and pushed to 8.3.x, but would not cherry-pick cleanly to 8.2.x for some reason. Probably needs a small re-roll.

Not sure why @webchick's cherry-pick did not work but I was able to cherry-pick it and pushed that to 8.2.x.

I'm pretty sure that this needs reverting. When using outside_in the save button in the sidebar no longer works for me. Using git bisect I've narrowed it to this commit. I've tested on the latest versions of firefox, chrome and safari.

Steps to reproduce:

1. Install the standard profile
2. Enable the outside_in module
3. Log in as user 1.
4. From the user/1 page go into editting mode
5. Click on "quick edit" contextual link for the site branding block.
6. Update the site name field
7. Try and press "Save block"

@catch has confirmed that he experiences that same problem and doing git revert -n fdc09d3 fixes it. Going to revert this for now.

Reverted the commit on 8.3.x - committed d32af63 and pushed to 8.3.x.
Reverted the commit on 8.2.x - committed 6201818 and pushed to 8.2.x.

Another thing this broke was "configure block" taking the user to the full configuration page.

@alexpott sorry for not catching this. Right now we don't have a JS test for submitting the form because of #2789381: Submitting an ajax dialog form in Javascript test throws a GastonJS error.

The problem was
document.addEventListener('click', preventClick, true);
This was preventing clicks on the whole document which obviously includes the offcanvas tray.
I have changed this to:
document.querySelector('#main-canvas').addEventListener('click', preventClick, true);

Another thing this broke was "configure block" taking the user to the full configuration page.

I have updated the preventClick to check first if the target is a contextual link so they will continue to work as normal.

One thing I have discovered in #2764931: Contextual links don't work with 'use-ajax' links is that we don't have JS tests for contextual links which makes it hard to test them here. For that issue I tried to write tests but kept running into #2773791: Clicking elements with children in Javascript tests throws a GastonJS exception or other GastonJS problems.

In #2782915: Standardize the behavior of links when Outside In editing mode is enabled I have added a bunch for JS Test coverage though I am not able to test form submitting or contextual links because of the aforementioned GastonJS bugs. But it does add JS coverage for about 6 other behaviors.

In that issue I did do a bit of a hacky work around to get around #2773791: Clicking elements with children in Javascript tests throws a GastonJS exception. We will see what others think of it. But it might allow testing of contextual links.

Ok I update the JS test to address a couple of the issue covered in patch.

I was able to find a temporary workaround #2789381: Submitting an ajax dialog form in Javascript test throws a GastonJS error.

In the test I am using JS in test to click the submit button. This not ideal but I think better than no coverage. I have a @todo to remove this when GastonJS issue is solved.

I also added testing that the default action of links and form elements in the editables is suppressed during edit mode.

+++ b/core/modules/outside_in/js/outside_in.js
@@ -53,45 +31,100 @@
+        document.querySelector('#main-canvas').addEventListener('click', preventClick, true);

While I'm in favor of using vanilla JS where possible, seems odd to start using it here to bind the click event handler. Looks like you already had this discussion previously.

+++ b/core/modules/outside_in/js/outside_in.js
@@ -53,45 +31,100 @@
+  function preventClick(event) {

Add docs here to explain what's happening.

@drpal thanks for review!

I added a doc to that function and another 1 that was missing.

#38:

we don’t have JS tests for contextual links which makes it hard to test them here

Yep. Contextual Links long predate JS testing (which we've had for about 6 months now): Almost 7 years ago, they were moved into a separate module, at #626286: Make contextual links a module. Kudos for working on JS tests for them! It will indeed be very very valuable to have test coverage for them.

I was able to find a temporary workaround #2789381: Submitting an ajax dialog form in Javascript test throws a GastonJS error

Hm… now this issue is introducing a work-around, even though that's exactly what #2773791: Clicking elements with children in Javascript tests throws a GastonJS exception is doing. Speaking of which, it looks like #2789381: Submitting an ajax dialog form in Javascript test throws a GastonJS error is simply a duplicate of #2773791: Clicking elements with children in Javascript tests throws a GastonJS exception.

So, sadly… I think this is effectively blocked on #2773791: Clicking elements with children in Javascript tests throws a GastonJS exception. Since this is just a normal issue, I don't think it makes sense to rush this in; we should instead fix it properly in #2773791: Clicking elements with children in Javascript tests throws a GastonJS exception.

Unlocked.

(I didn't review the patch anyway)

Something wrong, it passed in my local env (Phantomjs 2.1.1) .

#43 fixed one error anyway.
Trying to reduce the testing and see what happening.

@droplet thanks for patch.

I see that the only problem was dialog is not rendering the button slightly differently so the css selector for targeting the button needs to be updated.

This more boringly named patch does that.

EDIT: @droplet I didn't see your comment in #45 but I think my patch will fix the error in your patch in #43

I see that the only problem was dialog is not rendering the button slightly differently so the css selector for targeting the button needs to be updated.

Ahh. It looks like I'm running on an old code base. I just `git reset --hard && git pull` and see the errors.

Great! It's GREEN!!

Ok so this no longer postponed so changed the title.

I think this is ready for RTBC.

@Wim Leers' review in #42 was all about the testing. There is still a @todo about switching to pressButton() but click() is working for now without #2773791: Clicking elements with children in Javascript tests throws a GastonJS exception. That is still important but doesn't block this issue.

@drpal's review in #40 I addressed in #41.

By the way:

+++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php
@@ -26,72 +36,92 @@ protected function setUp() {
+    foreach ($blocks as $block) {
...
+        $this->click('.ui-dialog-offcanvas .js-form-submit');

Clicking actions catching the edge cases. But we need not test the Save action 3 times if that's time-consuming for testbots.

@Wim Leers' review in #42 was all about the testing. There is still a @todo about switching to pressButton() but click() is working for now without #2773791: Clicking elements with children in Javascript tests throws a GastonJS exception. That is still important but doesn't block this issue.

+++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php
@@ -26,72 +36,92 @@ protected function setUp() {
+        $page->pressButton($block['button_text']);

We can do it. Passing dynamic text into it. @todo is wrong notes I think.

I preferred click with selectors.

`pressButton()` is also calling `click()` in background but `pressButton()` helping you to locate the BUTTON from page without selectors.

Ok switching to RTBC. @droplet's patch in #54 removed last @todo regarding GastonJS bug.

This is only a reroll of #54 now that #2782803: Rename Outside-in module to "Settings Tray" in the UI and in comments was committed.

Also forgot to address this:

But we need not test the Save action 3 times if that's time-consuming for testbots.

The save action actually only gets called 2 times in search_form_block doesn't need to test saving the form. It is just used to test opening the block form by clicking a form element in the block. This shows that clicking on form element don't trigger the regular form action.
Each of other blocks tests something different when saving

1. system_powered_by_block test that regular block settings take effect when in save in the BlockEntityOffCanvasForm.
2. system_branding_block uses are new class SystemBrandingOffCanvasForm and test that it's configuration in buildConfigurationForm takes effect.

Bumping this to major because it a major rewrite of the Javascript.

@tedbow

This looks good. Just one minor nit about jsodoc types.

+++ b/core/modules/outside_in/js/outside_in.js
@@ -53,45 +31,111 @@
+   * @return {bool}

This should be boolean.

@drpal ok great. fixed the jsdoc.

Looks like the tests asked for by @alexpott were accounted for in #39.

I'll confess I have no idea what 90% of this is. ;) But it's largely by our JS maintainers, and it's against an experimental module, soooo...

Committed and pushed to 8.2.x and 8.3.x! (Take two.) Thanks!

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