Implement Panels In-place editor display builder in panels_ipe module

I have taken a first step toward a UX/Design pattern for IPE: Panels IPE D8 UX 1.0

some notes:
- I am trying to move away from a modal interaction pattern because in D7 there were many usability problems (such as modal inside a modal)
- Sidebar "workspace" would provide an easier staging environment for users drop content on the page after they have selected all the content
- I split apart the experience for "add content" and "place existing" so that the interaction patterns are normalized.

Let me know what your thoughts are.

Incremental patch update. Includes:

• Controller class to handle Backbone requests.
• Backbone Models and Views for Regions, Blocks and the App itself.
• Ability to re-render any block using our Controller (useful for a lot of reasons).

New incremental patch. Includes:

• Ability to select a new layout and and re-render blocks in the new layout's regions (currently defaults to putting everything in the first one.
• Removed reliance on Page Manager Page Entity.
• Lots of refactors to the Backbone Views, which can all be re-rendered at any time.

Immediate todos:

• Add a move button to Blocks that lets you move blocks between regions (drag and drop can be added way later, this is a mobile-first choice).
• Add logic to save the current state of the app to Drupal.
• Fill out the "Manage Content" tab with existing blocks.

New incremental patch. Includes:

• Ability to move blocks between regions and reorder blocks within regions.
• Ability to actually save changes made in the IPE to the Layout.
• Ability to change layout and move/reorder blocks as many times as you want in one IPE session.

Immediate todos:

• Fill out the "Manage Content" tab with existing blocks.
• Investigate problems that could occur as a result of multiple editors.

Last patch was created wrong, sorry all.

Removed the existing D7 code to make dev on D8 a bit cleaner/easier.

New incremental patch. Includes:

• First shot at filling out the "Manage Content" tab.
• Basic ability to browse existing block plugins and render a block plugin form.

Immediate todos:

• Add ability to actually add blocks after the form has been submitting.
• Make sure that rendering and saving new blocks works.
• Ensure that contexts can be passed to blocks that require contexts.
• Make sure that our form logic matches what's happening in the backend.
• Investigate problems that could occur as a result of multiple editors.

I haven't pushed the code yet - but new block additions seem to be working with very little code changes! By next week we should have block plugin addition/removal in the IPE.

New incremental patch. Includes:

• Ability to add blocks and configure blocks via the tray or block-headers.

Immediate todos:

• Fix bugs with some form fields, verify that current form logic makes sense.
• Ensure that contexts can be passed to blocks that require contexts.
• Investigate problems that could occur as a result of multiple editors.

Finally had some time to test this and look (a bit) at the code.

First, as far as testing:

• Moving blocks, adding blocks and saving all worked in most cases!
• It's a little weird that there is no button to remove a block, but I assume that's coming in a future iteration. :-)
• There were a couple blocks that failed to add correctly, one that I remember for sure was the "Entity view (User)" block from CTools.
• I was testing with the "Two column" layout from Panels, and while it was generating the right markup, it didn't show in two columns (maybe the CSS wasn't being loaded?)

Next, a little code review! This adds a ton of new code and I didn't really have a chance to look at it all. So, I focused on the backend parts because it seems like you have a handle on the frontend stuff.

1. +++ b/panels_ipe/panels_ipe.routing.yml
   @@ -0,0 +1,53 @@
   +    _permission: 'access panels in-place editing'
   

   We're going to need to come up with a way for these routes to also check that the user has permission to edit the underlying thing (page_manager variants in this case).

2. +++ b/panels_ipe/src/Controller/PanelsIPEPageController.php
   @@ -0,0 +1,405 @@
   +    $layout = $variant_plugin->getConfiguration()['layout'];
   

   Since we're always dealing with a PanelsDisplayVariant you should call $variant_plugin->getLayout()->getPluginId() for this. The fact that it's in 'layout' in it's configuration, isn't actually part of the API - that's an implementation detail.

3. +++ b/panels_ipe/src/Controller/PanelsIPEPageController.php
   @@ -0,0 +1,405 @@
   +    // Remove all blocks from the build.
   +    $regions = $variant_plugin->getRegionNames();
   +    $region_data = [];
   +    foreach ($regions as $id => $label) {
   +      // Get all block/random keys.
   +      $children = Element::getVisibleChildren($build[$id]);
   +      // Unset those keys, retaining the theme variables for the region.
   +      $build[$id] = array_diff_key($build[$id], array_flip($children));
   +
   +      // Format region metadata.
   +      $region_data[] = [
   +        'name' => $id,
   +        'label' => $label
   +      ];
   +    }
   

   Hm. Rendering the variant and then removing the blocks works, but has you doing a lot of extra work!

   You could do $variant_plugin->getLayout()->build($regions) and put the markup you want in the regions into $regions.

4. +++ b/panels_ipe/src/Controller/PanelsIPEPageController.php
   @@ -0,0 +1,405 @@
   +    $configuration['layout'] = $layout['id'];
   

   We don't have an API function for setting the layout, but we should! Since this the key on the configuration array is internal information. Something like ->setLayout() which takes either a string or LayoutInterface object. We also need a way to set the layout configuration.

5. +++ b/panels_ipe/src/Controller/PanelsIPEPageController.php
   @@ -0,0 +1,405 @@
   +    /** @var \Drupal\layout_plugin\Plugin\Layout\LayoutBase $layout */
   

   This should be LayoutInterface - there's no guaranty that this descends from LayoutBase.

6. +++ b/panels_ipe/src/Controller/PanelsIPEPageController.php
   @@ -0,0 +1,405 @@
   +    $layout = $this->layoutPluginManager->createInstance($layout_id, []);
   

   Calling $plugin->getLayout() would be sufficient! And it would include the layout configuration, which you're just passing [] for here.

7. +++ b/panels_ipe/src/Form/PanelsIPEBlockPluginForm.php
   @@ -0,0 +1,243 @@
   +    $elements = [
   +      '#theme' => 'block',
   +      '#attributes' => [
   +        'data-block-id' => $uuid
   +      ],
   +      '#configuration' => $configuration,
   +      '#plugin_id' => $block_instance->getPluginId(),
   +      '#base_plugin_id' => $block_instance->getBaseId(),
   +      '#derivative_plugin_id' => $block_instance->getDerivativeId(),
   +      'content' => $block_instance->build()
   +    ];
   

   I think we need to apply the context mapping before doing this...

8. +++ b/panels_ipe/src/Form/PanelsIPEBlockPluginForm.php
   @@ -0,0 +1,243 @@
   +    $form = [
   +      '#type' => 'container',
   +      '#attributes' => [
   +        'id' => 'panels-ipe-block-plugin-form-json'
   +      ],
   +      ['#markup' => Json::encode($settings)]
   +    ];
   

   This seems a little odd. Can we do something with AJAX commands to pass this to the drupalSettings without having to put JSON in markup?

9. +++ b/panels_ipe/src/Plugin/DisplayBuilder/InPlaceEditorDisplayBuilder.php
   @@ -0,0 +1,124 @@
   +        $build[$region]['#prefix'] = '<div class="' . $region_name . '" data-region-name="' . $region . '">';
   +        $build[$region]['#suffix'] = '</div>';
   

   The D7 IPE did something similar too. But I think it'd be really cool NOT to add any markup to the regions when first rendering the page.

   Instead, we could make an AJAX call to rerender the panel again (with any markup changes) once the user has activated the IPE. The advantage of doing this is that original version of the page (which is what will be cached) has cleaner markup.

   Anyway, not important to do now, especially since D7 did it the same way as is in the patch. Just something to think about for the future.

10. +++ b/panels_ipe/src/Plugin/DisplayBuilder/InPlaceEditorDisplayBuilder.php
    @@ -0,0 +1,124 @@
    +      $build['#attached'] = [
    +        'library' => ['panels_ipe/panels_ipe'],
    +        'drupalSettings' => $this->getDrupalSettings($regions, $layout)
    +      ];
    

    This is why the layout isn't rendering correctly. This would overwrite the 'library' provided by the layout.

Here's a quick patch that fixes #17.10. Once we're not overwriting the 'library' it just works!

And a question that just occurred to me: where would layout settings fit into the IPE? In Drupal 7, I think we ignored them. :-) We only allowed configuring them in the normal Panels admin. But it would be great to allow this through the IPE somehow!

Gah! I hate working with binary data in patchs. This one should apply and make testbot happy. :-)

Thanks for the notes, I'll follow up with a new patch tomorrow. #9 is most interesting to me because it would remove a lot of the Display Builder and panels_ipe.js init code. Totally agree with the rest of the suggested changes.

Here's another incremental patch that addresses #1, #2, and #3. #5/6 are valid, but I can't use $plugin->getLayout() as the layout currently active in the IPE isn't necessarily the one currently saved to the PanelsDisplayVariant plugin. Maybe I should set the new layout temporarily and call getLayout()/getRegionNames() after that?

The code in #7 is called after calling PanelsIPEBlockPluginForm::getBlockInstance(), which attempts to add context to the Block.

#8/#9 are in my todo list, but require a bit of refactoring so I don't have that code yet.

#10 was already fixed by dsnopek, but his change is reflected again in my patch as there was some issues creating patches with binary data before.

One problem I'm working on now is keeping my just-JSON AJAX endpoints and still loading the attached CSS/JS from new Blocks and Layouts. I think I'm going to have to do an initial request for the Backbone Model (metadata/configuration), then a secondary request for the HTML, which would include the contents of #attachments. If there's another way to do this, let me know. :-)

(P.S. I removed the contents of /images as those SVGs are reflected in our Icon Font. Please ignore the top of the interdiff.)

Thanks! This is looking really great. :-)

#5/6 are valid, but I can't use $plugin->getLayout() as the layout currently active in the IPE isn't necessarily the one currently saved to the PanelsDisplayVariant plugin. Maybe I should set the new layout temporarily and call getLayout()/getRegionNames() after that?

Yeah, I think that makes the most sense. We should add a method to set the layout, like:

  /**
   * @param string|LayoutInterface $layout
   * @param array $settings
   */
  public function setLayout($layout, $settings) { ... }

And then you can call that to set the layout currently selected in the IPE, and then call getLayout() (just don't save it yet). That way you're not depending on anything internal to the PanelsDisplayVariant class AND have the layout initialized correctly.

The code in #7 is called after calling PanelsIPEBlockPluginForm::getBlockInstance(), which attempts to add context to the Block.

Ah, ok, I didn't see that! However, I'm not sure the code in ::getBlockInstance() is quite right with regard to contexts. It's got:

    // Add context to the block.
    if ($block_instance instanceof ContextAwarePluginInterface) {
      /** @var \Drupal\page_manager\PageVariantInterface $variant */
      $variant = PageVariant::load($variant_id);
      $this->contextHandler->applyContextMapping($block_instance, $variant->getContexts());
    }

But $variant->getContexts() delegates all the way up to PageExecutable::getContexts() which fires the page_manager event to gather contexts, which will be for the form page route and not the IPE page. I think you'll need to do $variant->setContexts() or manually derive the contexts (at least with regard to routes) somehow to pass to the blocks... I may look into this more later...

Here's another incremental patch. Includes moving to using drupalSettings for new/updated Blocks, and setting a new layout instead of creating a new Layout Plugin instance. Everything in comment #17 is addressed except not wrapping regions on load (we should make a new issue for that) and setting context correctly (which I leave to the resident context experts).

I'm doing a formalish UX review tomorrow so expect another patch soon with lots of CSS/JS tweaks.

Here's (what I think will be) the last incremental patch before this issue is closed. Includes:

• Various small UX tweaks.
• The ability to preview a Block before adding it to a region.

Any un-addressed bugs should be followed up on in new issues, some of which I'll be creating tomorrow and appending to some sort of tracking/roadmap issue. I'll be iterating and refactoring this code up until some stable Panels release is out.

Awesome! Since we've decided to merge and then make further improvements and bug fixes in follow-up issues, I've done a quick review for (1) things that would be a pain to update later and (2) security issues. Those are really the only things we need to get right on this initial merge!

Here's what I found:

1. +++ b/panels_ipe/js/views/BlockPicker.js
   @@ -0,0 +1,314 @@
   +    template_plugin: _.template(
   +      '<div class="ipe-block-plugin">' +
   +      '  <div class="ipe-block-plugin-info">' +
   +      '    <h5><%= label %></h5>' +
   +      '    <p>Provider: <strong><%= provider %></strong></p>' +
   +      '  </div>' +
   +      '  <a data-plugin-id="<%= plugin_id %>">Add</a>' +
   +      '</div>'
   

   'label' could come from user entered input in the case of content blocks. This doesn't appear to be sanitized anywhere. I tried creating a custom block with a "Block description" of Custom block <script>alert('bad')</script> and it got executed when trying to place it in the IPE!

   I'm not sure if this template is the one that rendered the script or if it was one of the one's below...

2. +++ b/panels_ipe/js/views/BlockPicker.js
   @@ -0,0 +1,314 @@
   +    template_existing: _.template(
   +      '<div class="ipe-block-plugin">' +
   +      '  <div class="ipe-block-plugin-info">' +
   +      '    <h5><%= block.label %></h5>' +
   +      '    <p>Provider: <strong><%= block.provider %></strong></p>' +
   +      '  </div>' +
   +      '  <a data-existing-region-name="<%= region.name %>" data-existing-block-id="<%= block.uuid %>">Configure</a>' +
   +      '</div>'
   

   'block.label' here too!

3. +++ b/panels_ipe/js/views/BlockPicker.js
   @@ -0,0 +1,314 @@
   +    template_plugin_form: _.template(
   +      '<h4>Configure <strong><%= label %></strong> block</h4>' +
   +      '<div class="ipe-block-plugin-form"><div class="ipe-icon ipe-icon-loading"></div></div>'
   

   And 'label' again.

4. +++ b/panels_ipe/src/Plugin/DisplayBuilder/InPlaceEditorDisplayBuilder.php
   @@ -0,0 +1,122 @@
   + *   id = "in_place_editor",
   

   Could we change the 'id' to 'ipe'? That would match D7 and make one less thing we have to convert in the migration. :-)

I just did some more manual testing, and all the recent changes look great! The preview in particular is freaking awesome! :-) I also tried to run through the basics (moving, adding, deleting, changing layout) to try and see if there were any regressions - I couldn't find any problems.

I also added a short list of things that I'm aware of that need to get handled in follow-up issues (just so I don't forget about them). Anyway, once the issues in #27 are fixed, I think this should be ready to merge!

Ah, this is as simple as changing all instances of %= to %- in the Underscore templates, sorry for missing that. I'm testing the change now and will post a patch soon.

Here we go. I verified that replicating your steps of adding a malicious script to the Block Description doesn't get executed. First time Backbone/Underscore user here, obviously. :-)

Re-rolling...

Last patch recognized the panels_ipe directory as a sub-project and ignored all the files. This one should be good.

Updated issue summary with a few more todos.

Sweet! The interdiff on #32 looks good to me, and I re-tested the security issue with it and the XSS vulnerability was gone. :-) Personally, I think this is ready!

1. +++ b/panels_ipe/panels_ipe.permissions.yml
   @@ -0,0 +1,2 @@
   +  title: 'Access panels in-place editing'
   \ No newline at end of file
   
   +++ b/panels_ipe/panels_ipe.routing.yml
   @@ -0,0 +1,53 @@
   +    _method: 'POST'
   \ No newline at end of file
   

   Missing newline

2. +++ b/panels_ipe/src/Controller/PanelsIPEPageController.php
   @@ -0,0 +1,428 @@
   +use Zend\Diactoros\Response\JsonResponse;
   

   Any particular reason to use the Zend class and not \Symfony\Component\HttpFoundation\JsonResponse ?

3. +++ b/panels_ipe/src/Controller/PanelsIPEPageController.php
   @@ -0,0 +1,428 @@
   +  public function __construct(BlockManagerInterface $block_manager, RendererInterface $renderer, LayoutPluginManagerInterface $layout_plugin_manager, ContextHandlerInterface $context_handler) {
   ...
   +      $container->get('context.handler')
   

   This passes in the context handler, but doesn't use it

4. +++ b/panels_ipe/src/Controller/PanelsIPEPageController.php
   @@ -0,0 +1,428 @@
   +    if (!$variant->access('read')) {
   +      throw new AccessDeniedHttpException();
   +    }
   ...
   +    if (!$variant->access('read')) {
   +      throw new AccessDeniedHttpException();
   +    }
   ...
   +    if (!$variant->access('update')) {
   +      throw new AccessDeniedHttpException();
   +    }
   ...
   +    if (!$variant->access('update')) {
   +      throw new AccessDeniedHttpException();
   +    }
   ...
   +    if (!$variant->access('read')) {
   +      throw new AccessDeniedHttpException();
   +    }
   ...
   +    if (!$variant->access('read')) {
   +      throw new AccessDeniedHttpException();
   +    }
   

   Seems like these should be part of an access check at the routing level.

5. +++ b/panels_ipe/src/Controller/PanelsIPEPageController.php
   @@ -0,0 +1,428 @@
   +    $form = \Drupal::formBuilder()->getForm('Drupal\panels_ipe\Form\PanelsIPEBlockPluginForm', $plugin_id, $variant_id, $regions, $block_uuid, $new);
   

   Might as well inject the form builder

6. +++ b/panels_ipe/src/Form/PanelsIPEBlockPluginForm.php
   @@ -0,0 +1,320 @@
   +    $input = $form_state->getUserInput();
   ...
   +      $block_instance->setConfiguration($input['block']);
   

   This seems scary to me. Do we really want to set the raw and unvalidated user input?

7. +++ b/panels_ipe/src/Form/PanelsIPEBlockPluginForm.php
   @@ -0,0 +1,320 @@
   +        'class' => 'flipper'
   +      ]
   ...
   +        'class' => 'front'
   +      ]
   ...
   +        'class' => 'back'
   +      ]
   

   Missing commas, here and in a bunch of other places

8. +++ b/panels_ipe/src/Plugin/DisplayBuilder/InPlaceEditorDisplayBuilder.php
   @@ -0,0 +1,122 @@
   + * The In-place editor display builder for viewing and editing a ¶
   

   Trailing whitespace

Thanks, @tim.plunkett!

Re #35.4: that'd probably involve putting {page_variant} in the route, which just couples this more to page_manager so I was cool with that for now, since we still have the follow-up task to decouple it :-)

Re #35.6: Great catch! We should definitely apply some kind of validation.

So for #6, that default configuration is only used to provide initial defaults to the configuration form - from there on in form values are validated/submitted normally. That said the potentially "raw" values are again used in PanelsIPEPageController::updateVariant() when finally adding/update the blocks.

So I'm thinking instead of working with raw block configuration values in JS, we should only store user form input. Then we could map/validate/submit that in the backend without directly setting configuration. This would be nice too as the BlockModel would only contains keys we're aware of, and would never be mapped directly to the backend (we could remove those nasty unset() calls I'm doing).

After working on #6 for awhile, it has become apparent that storing FormState or user input looks just as bad and is possibly as insecure as what we have now. Instead, we should probably just verify the configuration before finally saving it to the PageVariant. A long term alternative would be to move to using TempStore across the IPE, but that will take awhile and can be added in another issue.

This patch should address every issue raised in comment #35 except #35.6, which I'm still working out.

My hopeful plan for the next patch is:

1. Remove all block configuration from the frontend and instead use private TempStore. The frontend should only be aware of metadata.
2. Modify PanelsIPEPageController::updateVariant() to only save the Page Variant and remove the TempStore.
3. Add a cancel button to the tray, which clears TempStore.

It's a lot of work, but in the long run I think many of us would like it to work like this anyway.

Okay so I'm going to hold off committing until those next 3 points in #39 are in.

So here's what I have so far on the TempStore front, which addresses #35.6 as no Block configuration is stored in the App.

Things I would like to get in at some point:

1. When you move or remove a block it isn't immediately saved to TempStore (only "committed" when you click save). I like this from a performance perspective, but this may be confusing to users. Maybe we should do a sync call every few seconds that temporarily saves the block position/weight, and removing a block should do a new AJAX call (DELETE?).
2. We can kind of support multiple editors now, how crazy do we want to get with that support? Should the App poll for TempStore changes as you use it? This is way far in the future, if we ever want this.

These may have to be new issues if we want this closed soon.

Fixed a one-line JS problem -- the urlRoot for AJAX callbacks had a hard-coded leading slash, which meant that AJAX calls would fail on sites installed in a subdirectory.

That will break any page that is in a sub-path ex: /foo/my-page will try to reach out to URLs like /foo/admin/panels_ipe/variant/panels/block_plugins .

Changed to use settings.path.baseUrl, as recommended by @samuel.mortenson.

Regarding the tempstore changes in #42: I think this is sufficient to solve the problem originally identified in #35.6! However, some of the new code is quite messy.. I'll give a review on the interdiff below - but, personally, I'd be fine with deferring it to a follow-up in the interest of getting this merged!

2. We can kind of support multiple editors now, how crazy do we want to get with that support? Should the App poll for TempStore changes as you use it? This is way far in the future, if we ever want this.

In D7, the person currently editing the tempstore gets a "lock" on it, and if someone else comes to edit it they get a popup letting them know and asking if the want to break the lock in order to start editing it. Something like that should be sufficient - I'm not sure we really want to support multiple people editing the same Panel at the same time..

And here's a little review:

1. +++ b/panels_ipe/src/Plugin/DisplayBuilder/InPlaceEditorDisplayBuilder.php
   @@ -93,8 +136,41 @@
   +    /** @var \Drupal\page_manager\PageVariantInterface $page_variant */
   +    $page_variant = \Drupal::request()->attributes->get('page_manager_page_variant');
   

   This couples us more to page_manager, and even beyond that, to stuff from the current request (ie. global state).

   I think we could avoid this by making the $temp_store_key be the $plugin->uuid() rather than using the PageVariant id.

   However, since we'll need a follow-up to decouple from page_manager this is probably fine for now. But it's extra icky!

2. +++ b/panels_ipe/src/Plugin/DisplayBuilder/InPlaceEditorDisplayBuilder.php
   @@ -93,8 +136,41 @@
   +      // Build again as regions and layout may have changed.
   +      $build = parent::build($regions, $contexts, $layout);
   

   Hrm. Running parent::build() more than once kinda sucks. Could we just not run it the first time?

Beyond that, some of the code that manipulates the tempstore seems a little verbose, like it might be doing more than it needs to. However, without actually attempt to reduce it and see what happens, I'm not 100% sure what to recommend. But, yeah, I'd be fine with committing what we have now, and making a follow-up to clean it up.

@dsnopek I think it's fair to work on this a bit more before merging it in, we're not in a huge rush.

I'll take some time tomorrow to try and review the TempStore set/get calls and look into that parent::build() problem you mentioned. From what I remember the \Drupal::request()->attributes->get('page_manager_page_variant') call was necessary, but I'll debug and see if I can get the plugin from the current request instead. Changing the $temp_store_key seems fine as well.

Maybe we should set a cut-off date on tweaks for end of day tomorrow, or end of this week?

Well, there's a couple follow-ups I really wanna start working on, so that I can use the IPE with Panelizer. ;-) I'm going to take a stab at fixing some of the things from my review in #47...

Bah! So, this interdiff-y patch shows what I was thinking for fixing the issues in #47. It requires changing DisplayBuilderInterface, but this more closely matches discussions that EclipseGC and I have been having about the Panels API in #2626944: Create Panels value object which can be serialized/unserialized with a schema to validate it. The problem is that it runs into the fact that we're coupled to the PageVariant in other places too (see the @todo that the patch adds) so it would take much more work to actually get this working. :-/ So, I think this really belongs in the "decouple from page_manager" follow-up.

Just tested this and I must say I'm really impressed with it, congratulations to all already!
Can't say much about the code since I'm not to familiar yet with the D8 code of page magers/panels etc, so I'll someone else RTBC it again, but as far as I'm concerned, this is great to start :)

only one feedback for now - briefly mentioned this to dsnopek on IRC already. Sometimes the configure block form doesn't show up, sometimes it works smoothly, and I have no idea exactly what is going on. Vanilly drupal install, latest dev for all dependency modules.

While debugging, I see the request and there's a response coming back, but the loader doesn't get replaced and no form.

Added some debugging in Blockpicker.js

      // Remove our throbber on load.
      ajax.options.complete = function() {
        if (self.$('#panels-ipe-block-plugin-form-wrapper').length > 0) {
          self.$('.ipe-block-picker-top .ipe-icon-loading').remove();
          self.$('#panels-ipe-block-plugin-form-wrapper').hide().fadeIn();
        }
        else {
          console.log('oh dear, i am not there yet :(');
        }
      };

Sometimes I see the console log message, sometimes the block, but not sure exactly what goes wrong here and if it is even related to javascript/ajax. Note: usually it starts showing up again after a drush cr, and then usually the first 3/4 blocks are fine, but then suddenly, nothing again.

Also, this should hold up the commit either.

Thanks! Added a note to the issue summary to remember to make a follow-up issue for that.

RE: #50 - I took a look as well and came up with similar conclusions, I'll hold discussion until we make a new issue. This is the patch I came out with after trying to make more significant changes, ended up being mostly minor/nit-picky stuff.

Reviewed with dsnopek and sam. MVP looks good. Committed!

Please post follow-ups in their own issues.

NICE! :D

Created these follow-ups:

• #2636472: Load context for page that IPE is editing, rather than AJAX or form route
• #2636478: IPE shouldn't depend on Page Manager
• #2636484: Sometimes the block configuration form doesn't load in the IPE
• #2636490: IPE's cancel button is easy to mistake for a button to close the tray (rather than throw away all changes)
• #2636488: Hide IPE cancel button when there is nothing to cancel yet
• #2636486: In IPE, the available layouts have scrollbars on them

Created the rest of the follow-ups:

• #2636516: Allow creating new content blocks from the IPE
• #2636520: Decide how we scale with large amounts of Block categories, Layouts, and Blocks in the IPE.
• #2636522: Improve IPE mobile UX to make sure we have a nice experience on Tablets and Phones.
• #2636524: Filter out content blocks that aren't "reusable" per #2626942: Creating Custom Blocks that are not re-usable
• #2636530: "Items per block" setting in Views is never set.
• #2636532: Previewing a Block containing a Form in the IPE breaks add/update
• #2636538: In the IPE, new Blocks and Layouts do not have their #attachments loaded on render