Allow off-canvas dialog to be rendered at the top of the page

This functionality should be doable because of #2844261: Allow dialog links to specify a renderer in addition to a dialog type

The way envision it working is

$mylink['attributes'] = [
      'class' => ['use-ajax'],
      'data-dialog-type' => 'dialog',
    // Each version would need it's own renderer.
     'data-dialog-renderer' => 'off-canvas-top',
];

I think we should tackle this after #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it because then modules would not need the Settings Tray module installed to use the dialogs.

So after that issue we would need to

1. Update \Drupal\Core\Render\MainContent\OffCanvasRenderer::__construct to accept another argument $position defaulting to "side".
2. Create another service main_content_renderer.off_canvas_top that would duplicate main_content_renderer.off_canvas accept it would send another parameter "top".
3. Update \Drupal\Core\Render\MainContent\OffCanvasRenderer::renderResponse to pass a 'position' option to \Drupal\Core\Ajax\OpenOffCanvasDialogCommand::__construct
4. In \Drupal\Core\Ajax\OpenOffCanvasDialogCommand::__construct add class dialog-off-canvas-top class to the dialog
5. Update css in off-canvas.*.css files to position based on dialog-off-canvas-top
6. Update off-canvas.es6.js not also ways displace from the side.

#2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it was committed 🎉! So removing from settings_tray.module component.

@timmillwood was there a UX discussion/issue about whether to use existing off-canvas as it is vs from the top. Why I ask is that one of the reasons we wanted to get the off-canvas dialog out of the Settings Tray and offer it as a stand alone dialog was so that other parts of core and contrib could use. The idea being that it is more unified experience if this kind of configuration always appears in the same place for the user.

Another thing I realized is that the CSS reset made in https://www.drupal.org/node/2826722 we specifically choose an ID selector because it is the most specific outside of inline css. If the off-canvas-top want to use this same reset then we would have to make 1 version of the off-canvase closed the other so that you couldn't have both the top and regular off-canvas open at the same(unless we found another way). Actually that seems like totally reasonable UX rule because both version would need resize the page.

1. +++ b/core/core.services.yml
   @@ -1052,6 +1052,11 @@ services:
   +  main_content_renderer.off_canvas_top:
   +    class: Drupal\Core\Render\MainContent\OffCanvasRenderer
   +    arguments: ['@title_resolver', '@renderer', 'top']
   +    tags:
   +      - { name: render.main_content_renderer, format: drupal_dialog.off_canvas_top }
   

   I like the reuse of the same class here!

2. +++ b/core/lib/Drupal/Core/Ajax/OpenOffCanvasDialogCommand.php
   @@ -34,8 +34,10 @@ class OpenOffCanvasDialogCommand extends OpenDialogCommand {
   -  public function __construct($title, $content, array $dialog_options = [], $settings = NULL) {
   +  public function __construct($title, $content, array $dialog_options = [], $settings = NULL, $position = 'right') {
   

   We would want the default value for $position to be "side" since the current off-canvas is only on the right side for LTR languages.

1. +++ b/core/misc/dialog/off-canvas.es6.js
   @@ -62,7 +62,7 @@
   +    beforeCreate({settings, $element}) {
   

   Should have spaces around the properties.

2. +++ b/core/misc/dialog/off-canvas.es6.js
   @@ -86,12 +86,13 @@
   +    beforeClose({$element}) {
   

   Should have spaces around the properties.

3. +++ b/core/misc/dialog/off-canvas.es6.js
   @@ -86,12 +86,13 @@
   +      const position = Drupal.offCanvas.getPosition() === 'side' ? Drupal.offCanvas.getEdge() : Drupal.offCanvas.getPosition();
   

   Instead of calling `Drupal.offCanvas.getPostion()` twice, can we store the response and use it in the ternary?

4. +++ b/core/misc/dialog/off-canvas.es6.js
   @@ -104,14 +105,15 @@
   +    afterCreate({$element, settings}) {
   +      const eventData = {settings, $element, offCanvasDialog: this};
   

   Incorrect spacing around properties.

5. +++ b/core/misc/dialog/off-canvas.es6.js
   @@ -104,14 +105,15 @@
   +      const position = Drupal.offCanvas.getPosition() === 'side' ? Drupal.offCanvas.getEdge() : Drupal.offCanvas.getPosition();
   

   Same feedback as above.

6. +++ b/core/misc/dialog/off-canvas.es6.js
   @@ -127,7 +129,7 @@
   +    render({settings}) {
   

   Incorrect spacing around properties.

7. +++ b/core/misc/dialog/off-canvas.es6.js
   @@ -147,7 +149,7 @@
   +      $element.css({height: 'auto'});
   

   Incorrect spacing around properties.

8. +++ b/core/misc/dialog/off-canvas.es6.js
   @@ -182,9 +184,10 @@
   +      const height = Drupal.offCanvas.getPosition() === 'side' ? `${$(window).height() - (offsets.top + offsets.bottom)}px` : '300px';
          container.css({
            position: 'fixed',
   -        height: `${$(window).height() - (offsets.top + offsets.bottom)}px`,
   +        height: height,
          });
   

   If we're going to assign `height` to a variable, use object-shorthand.

9. +++ b/core/misc/dialog/off-canvas.es6.js
   @@ -208,11 +211,12 @@
   +      const position = Drupal.offCanvas.getPosition() === 'side' ? Drupal.offCanvas.getEdge() : Drupal.offCanvas.getPosition();
   

   Same feedback as above.

10. +++ b/core/misc/dialog/off-canvas.es6.js
    @@ -238,7 +242,22 @@
    +    getPosition() {
    +      const classes = $('.ui-dialog-off-canvas').attr('class').split(' ')
    +      for (i = 0; i < classes.length; i++) {
    +        if (classes[i].includes('ui-dialog-position')) {
    +          return classes[i].split('-').pop();
    +        }
    +      }
    +    },
    

    This can be written a bit more simply, also no jQuery :)

    getPosition() {
      return document.getElementsByClassName('ui-dialog-off-canvas')[0].className
        .split(' ')
        .filter(className => className.includes('ui-dialog-position'))[0]
        .split('-')
        .pop();
    },
    

+++ b/core/misc/dialog/off-canvas.es6.js
@@ -238,7 +242,22 @@
+
+    /**
+     * The position on the screen the tray will be shown.
+     *
+     * @return {string}
+     *   The position the tray will be shown.
+     */
+    getPosition() {
+      const classes = $('.ui-dialog-off-canvas').attr('class').split(' ')
+      for (i = 0; i < classes.length; i++) {
+        if (classes[i].includes('ui-dialog-position')) {
+          return classes[i].split('-').pop();
+        }
+      }
+    },
+  }

I don't think this should have to be done in JS at all. Instead of relying on the class. It would be better to in \Drupal\Core\Ajax\OpenOffCanvasDialogCommand::__construct() just set
$this->dialogOptions['drupalOffCanvasPostion'] = $position;

For dialogOptions if we want to pass an option that is not a jQuery UI dialog option then we just prefix it with drupal. That way we can pass options that will be ignore by jquery dialog but available in JS for us.

+++ b/core/misc/dialog/off-canvas.es6.js
@@ -208,11 +211,12 @@
       if (width !== mainCanvasPadding) {
-        $mainCanvasWrapper.css(`padding-${Drupal.offCanvas.getEdge()}`, `${width}px`);
-        $container.attr(`data-offset-${Drupal.offCanvas.getEdge()}`, width);
+        $mainCanvasWrapper.css(`padding-${position}`, `${width}px`);
+        $container.attr(`data-offset-${position}`, width);
         displace();
       }
     },

I think you would only want to worry about width !== mainCanvasPadding if $position == 'side'

Also the existing code

afterCreate({$element, settings}) {
      const eventData = {settings, $element, offCanvasDialog: this};

      $element
        .on('dialogContentResize.off-canvas', eventData, Drupal.offCanvas.handleDialogResize)
        .on('dialogContentResize.off-canvas', eventData, Drupal.offCanvas.bodyPadding);

      const position = Drupal.offCanvas.getPosition() === 'side' ? Drupal.offCanvas.getEdge() : Drupal.offCanvas.getPosition();
      Drupal.offCanvas.getContainer($element).attr(`data-offset-${position}`, '');

      $(window)
        .on('resize.off-canvas', eventData, debounce(Drupal.offCanvas.resetSize, 100))
        .trigger('resize.off-canvas');
    },

We attach handlers which up to now have used to handle resizing based on width, when the dialog itself is resized and when the body is resized.

It seems like the JS logic might be simpler if we attach the existing handlers only when $postion == 'side'. If $position == 'top' then we could attach different handlers that deal with special case what needs to happen on top.

Also in \Drupal\Core\Ajax\OpenOffCanvasDialogCommand::__construct()

// If no width option is provided then use the default width to avoid the
    // dialog staying at the width of the previous instance when opened
    // more than once, with different widths, on a single page.
    if (!isset($this->dialogOptions['width'])) {
      $this->dialogOptions['width'] = static::DEFAULT_DIALOG_WIDTH;
    }
  }

This should probably happen only for side. I think for `top` we would want width to be 100%. Not sure that Jquery dialog supports that but we could use .css() to set this. then resizing the body would be easier I think

Alright, some information about what's going on.

When transitioning between a top rendered tray and a right side rendered tray, without closing the tray in between, the information returned by Drupal.displace() still contains the offsets as if the top rendered tray where still open.

When resetSize() is called, the offsets are incorrect here because the previous dialog element is not actually closed, just it's position changed. Here, const topPosition = position === 'side' && offsets.top !== 0 ? `+${offsets.top}` : '';, this will append the incorrect offset to the top.

There are a couple of options which I think might be possible to fix this,

- Ensure that the dialog is closed in between transitions. Settings tray does this by manually clicking on the close button.
- If the dialog is already open and an orientation change is happening, ignore (or adjust) the values coming from Drupal.displace()

Ensure that the dialog is closed in between transitions. Settings tray does this by manually clicking on the close button.

This method works.

Tested it. Looks much better with the new animation.

@timmillwood can you post the related issue(s) here as where this is actually needed?

1. +++ b/core/modules/system/tests/src/Functional/Ajax/OffCanvasDialogTest.php
   @@ -45,8 +45,9 @@ public function testDialog() {
   +          'drupalOffCanvasPosition' => 'side',
              'buttons' => [],
   -          'dialogClass' => 'ui-dialog-off-canvas',
   +          'dialogClass' => 'ui-dialog-off-canvas ui-dialog-position-side',
   

   We could use a dataprovider here too to test both positions.

2. +++ b/core/tests/Drupal/Tests/Core/Ajax/OpenOffCanvasDialogCommandTest.php
   @@ -31,8 +31,9 @@ public function testRender() {
   -        'dialogClass' => 'ui-dialog-off-canvas',
   +        'dialogClass' => 'ui-dialog-off-canvas ui-dialog-position-side',
            'width' => 300,
   +        'drupalOffCanvasPosition' => 'side',
   

   Should make a data provider for this test to test top and side?

There was something going on with \Drupal\Tests\system\FunctionalJavascript\OffCanvasTest::testOffCanvasLinks() where it iterator over all the themes it failed after the first theme.

This may be because some state that was set or javascript interaction in the previous iteration.

Anyways this loop just made the test much more complicated.

So I create a dataProvider function that runs the test once for every theme. I also reversed #28 because I think it is no longer needed.

We ran into this same problem with \Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTrayBlockFormTest::testBlocks() and remove the loop over the themes.

see: #2902191: Determine cause and fix random fail in \Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTrayBlockFormTest::testBlocks

In that case it was test logic error that only showed on second iteration but having that loop needlessly complicated the test as I think it does here(I probably added it originally, sorry).

We might to do a follow to remove all remaining instances of

foreach ($this->getTestThemes() as $theme) {

and use this dataProvider

+++ b/core/modules/system/tests/src/FunctionalJavascript/OffCanvasTest.php
@@ -107,4 +104,34 @@ public function testNarrowWidth() {
+    $this->waitForOffCanvasToOpen();
+
+    // Check that the canvas is not on the page.
+    $web_assert->elementExists('css', '#drupal-off-canvas');
+    // Check that the canvas is positioned on the side.
+    $web_assert->elementExists('css', '.ui-dialog-position-' . $position);

From the error I think this was only passing because the top panel had been opened previously.

Since we don't close dialog it passes before the old one closed.

Fixed by keeping track of last dialog class.

1. +++ b/core/modules/system/tests/modules/off_canvas_test/src/Controller/TestController.php
   @@ -45,17 +45,22 @@ public function thing2() {
   -        '#title' => 'Click Me 1!',
   +        '#title' => 'Open side panel 1',
   

   Changed this so all the links follow the same pattern.

2. +++ b/core/modules/system/tests/modules/off_canvas_test/src/Controller/TestController.php
   @@ -45,17 +45,22 @@ public function thing2() {
   -        '#title' => 'Click Me 1!',
   +        '#title' => 'Open side panel 1',
   

   Changed the link text so all links follow the same pattern.

3. +++ b/core/modules/system/tests/modules/off_canvas_test/src/Controller/TestController.php
   @@ -64,6 +69,9 @@ public function linksDisplay() {
   +            'classes' => [
   +              "ui-dialog" => "ui-corner-all side-2",
   +            ],
   

   Assigning a unique class to each link so that when you click a dialog link and a dialog is already open the test can distinguish between the old dialog and the new dialog.

4. +++ b/core/modules/system/tests/src/FunctionalJavascript/OffCanvasTest.php
   @@ -113,16 +122,16 @@ public function testNarrowWidth() {
   -    $link_text = $position === 'side' ? "Click Me $link_index!" : "Open top panel $link_index";
   +    $link_text = "Open $position panel $link_index";
   

   Now that link text all follows the same pattern this can be simplified.

5. +++ b/core/modules/system/tests/src/FunctionalJavascript/OffCanvasTest.php
   @@ -113,16 +122,16 @@ public function testNarrowWidth() {
   +    if ($this->lastDialogClass) {
   +      $this->waitForNoElement('.' . $this->lastDialogClass);
   +    }
   +    $this->waitForOffCanvasToOpen($position);
   +    $this->lastDialogClass = "$position-$link_index";
   

   First wait for the old dialog to close, then wait for the new one to open.

   Otherwise waitForCanvasToOpen will actually pass because the old dialog has the same class on the 2nd iteration.

6. +++ b/core/modules/system/tests/src/FunctionalJavascript/OffCanvasTest.php
   @@ -113,16 +122,16 @@ public function testNarrowWidth() {
   -    // Check that the canvas is not on the page.
   -    $web_assert->elementExists('css', '#drupal-off-canvas');
   -    // Check that the canvas is positioned on the side.
   -    $web_assert->elementExists('css', '.ui-dialog-position-' . $position);
   

   Both these checks are now in waitForCanvasToOpen()

Reviewed the code and manually tested with the off_canvas_test module, and everything is looking great :)

I think there are a couple of things that could be improved:

- the styling of the top dialog needs to remove the left border
- it would be nice if the animation triggered when the top dialog is opened would also include the toolbar. Right now, the "main window" transitions slowly to accommodate for the new thing at the top, but the toolbar moves "instantly", without any animation

But both of those things could be done in a followup, so let's get this in!

+++ b/core/core.services.yml
@@ -1070,6 +1070,16 @@ services:
+  main_content_renderer.off_canvas_side:
+    class: Drupal\Core\Render\MainContent\OffCanvasRenderer
+    arguments: ['@title_resolver', '@renderer', 'side']
+    tags:
+      - { name: render.main_content_renderer, format: drupal_dialog.off_canvas_side }

I don't really think it make sense to add this off_canvas_side renderer now.

What is the advantage of doing this? I think it will just confuse developers.

Which one should they choose? What is the difference?

I know there is no difference but for a new developer this would be confusing. I would problem assume there would some difference.

I think we should just not introduce off_canvas_side service until we are ready to deprecate off_canvas service.

I think deprecating off_canvas in 8.6 just seems like a pain to developer who just started using the off-canvas dialog in after 8.5.

@tedbow I removed off_canvas_side and off_canvas_top services, please correct me if it was just off_canvas_side

@timmillwood ok. we can figure it out in follow up.

One reason I think it is ok not have the default be side without explictly putting _side in the service is the term "off-canvas" seems to usually mean on the side.

https://www.w3schools.com/howto/howto_js_off-canvas.asp

https://foundation.zurb.com/sites/docs/v/5.5.3/components/offcanvas.html

https://www.webpagefx.com/blog/web-design/off-canvas-menu/

https://tympanus.net/codrops/2014/09/16/off-canvas-menu-effects/

@tedbow pinged us in the accessibility channel on Slack, in case this needed an accessibility review.

tl;dr - we're OK I think.

Apart from using different viewport edges, does the patch here change any other behaviour? If not, then I don't expect this to this introduce any new accessibility issues that we don't already know about.

Note we already have #2940677: Support prefers-reduced-motion in off-canvas dialog as a follow-up, and that will need to take this new feature into account. I think we should ensure that UI designs don't rely on animation, just as we don't want them to rely on colour vision. The settings tray is probably a high-risk animation for users affected by vestibular disorders, etc. - though admittedly I'm not exactly clear how to assess animation risks. I'm going by the fact that it involves a large portion of the viewport, especially on the top-edge demo. It isn't a blocker for this issue, but I think it should be a stabilty-blocker for experimental modules which employ off-canvas from now on. Especially if they're going to use trays with bigger dimensions. The good news is it looks easy enough to implement.

The most recent screenshot GIF is in #20 - is that still broadly up to date? One thing in that GIF strikes me as odd. It shows a tray on the right-hand side being dismissed when the user clicks a link on the main page, which makes a different tray slide in from the top. If the tray has an accessible role of "dialog", then this means it's behaving as a non-modal dialog. At one stage, the WAI-ARIA Authoring Practices suggested those ought to have a standard keyboard control for window-switching behaviour (like old HTML framesets, F6 I think). I went looking for the details but it isn't in the latest version of that W3C note. (I asked the editor about it on Twitter, in case it's still a thing.) It's not a blocker here, but it's on my back-burner thinking list now. Surprised I didn't wonder about it before.

@andrewmacpherson thanks for taking a look.

If the tray has an accessible role of "dialog", then this means it's behaving as a non-modal dialog.

Not sure if I follow all this but so far "off-canvas" in only being using a non-modal way, that it does not stop interaction with the rest of the page. In core using data-dialog-type=modal attribute would actually stop you from interacting with the page when the modal is open.

Off-canvas uses data-dialog-type=dialog which does not stop you from interacting with the rest of the page. So even before this patch you could open the off-canvas and then click another link that opens in the off-canvas which would close you previous off-canvas dialog.

So this follows the same pattern. If you had 1 off-canvas dialog open, whether side or top and click another link that opened an off-canvas dialog either side or top your previous dialog would be closed.

Thanks for the extra info @tedbow. The part about data-dialog-type=modal|dialog is handy to know. It's something to consider in #2893640: Modernize ARIA usage, in line with ARIA 1.1 and the ARIA Authoring Practices guide., in relation to the new <dialog> element from HTML 5.2 and aria-modal="true" from ARIA 1.1 which we might want to employ one day.

So even before this patch you could open the off-canvas and then click another link that opens in the off-canvas which would close you previous off-canvas dialog.

I see, I think I saw that behaviour, but it don't think it was obvious that the off-canvas closed and re-opened (would it be animated, if they were both on the same side, and same width...?)

In general it's a rather weak argument to me that "some modules may have a need". With these kinds of additional options we also introduce more ways to make things inconsistent for people using these interfaces.

1. Does this only "make possible" or is using another position implemented already with this patch? I assume this is mostly in preparation for workspaces to use top positioning eventually.
2. General question: is it at all possible to show multiple trays at the same time? I hope not! :)

Thanks!

Thanks!

How is the off-canvas rendered in different responsive modes when it is on the top mode?

I didn't review the code yet, but will get to that next.

Nitpicks ahead, not setting back to Needs work because the changes are so small and they might not even be needed.

1. +++ b/core/modules/system/tests/src/Functional/Ajax/OffCanvasDialogTest.php
   @@ -45,8 +47,9 @@ public function testDialog() {
   +          'drupalOffCanvasPosition' => $position ?: 'side',
   ...
   +          'dialogClass' => 'ui-dialog-off-canvas ui-dialog-position-' . ($position ?: 'side'),
   
   @@ -54,9 +57,20 @@ public function testDialog() {
   +    $wrapper_format = $position ? 'drupal_dialog.off_canvas_' . $position : 'drupal_dialog.off_canvas';
   ...
   +      [NULL],
   

   Shouldn't we be more explicit here? And pass in side, and top as possible positions? The fact that NULL falls back to side should be a unit or kernel-level test instead.

2. +++ b/core/tests/Drupal/Tests/Core/Ajax/OpenOffCanvasDialogCommandTest.php
   @@ -40,4 +43,14 @@ public function testRender() {
   +  /**
   +   * @return array
   +   */
   +  public static function dialogPosition() {
   

   I think this docblock needs a summary as well. It also doesn't need to be static.

1. +++ b/core/misc/dialog/off-canvas.es6.js
   @@ -66,6 +66,9 @@
   +      $('.ui-dialog-off-canvas .ui-dialog-titlebar-close').trigger('click');
   

   Adding this slightly changes the way off canvas is rendered. This causes the off-canvas to "reboot" when updating content within same off-canvas which makes the interactions less smooth.

2. +++ b/core/misc/dialog/off-canvas.es6.js
   @@ -75,10 +78,14 @@
   +       * Applies initial height and with to dialog based depending on position.
   

   s/with/width

3. +++ b/core/misc/dialog/off-canvas.es6.js
   @@ -204,17 +214,26 @@
   +        $('nav#toolbar-bar').css('margin-top', `${height}px`);
   
   @@ -238,6 +257,16 @@
   +      $('nav#toolbar-bar').css('margin-top', '0');
   

   I don't think we can do this since this would create dependency on toolbar module. Let's explore if there would be a way to detach this from the core toolbar implementation.

Just was doing some manually testing found that this breaks some functionality we weren't actually testing for:

1. Open an off-canvas dialog link.
2. Open another off-canvas dialog link, without closing the first dialog
3. If the 2nd dialog dialog contains a link that will also open in the off-canvas dialog it will not work. It opens in new page instead of the dialog

If you don't do step 1 the link in step 3 will work.

Adding the test to the existing patch.

Also adding a patch with *only* this test change to show that somehow this patch breaks it.

Not sure why this is yet but a top panel dialog does not have to be involved for it not to work.

Fix patch with just new test

I looked at the toolbar problem. It would be better if we make the dependency from Toolbar to off-canvas. It should quite easy with the existing architecture by listening to the events that off-canvas provides.

Also few problems I spotted:

Currently if you open off-canvas top on mobile/tablet, it is opened behind the toolbar:

We also don't support screen resizing properly because this is what happens if you open the off-canvas top on desktop and then reduce the screen size:

This moves the toolbar related interactions to toolbar module.

+++ b/core/misc/dialog/off-canvas.es6.js
@@ -66,6 +66,9 @@
+      // Ensure the previous dialog, if any, is closed.
+      $('.ui-dialog-off-canvas .ui-dialog-titlebar-close').trigger('click');

This addition added in #17 is actually the change that causes the current test failure that I described in #54.

I debugged this with @drpal and he figured out that we could fix this with adding Drupal.ajax.bindAjaxLinks($element); in openDialog of core/misc/dialog/dialog.es6.js. But this now I figured out the problem is that we are closing dialog after the use-ajax events have been bound this doesn't seems that best fix.

I originally I added this line that is causing the failure from @drpal's suggestion in #16 but he also suggested another way in the same comment.

There are a couple of options which I think might be possible to fix this,

1. Ensure that the dialog is closed in between transitions. Settings tray does this by manually clicking on the close button.
2. If the dialog is already open and an orientation change is happening, ignore (or adjust) the values coming from Drupal.displace()

Maybe should try the 2nd approach he mentioned here.

Closing the dialog also the problem mention by @lauriii in #53

Adding this slightly changes the way off canvas is rendered. This causes the off-canvas to "reboot" when updating content within same off-canvas which makes the interactions less smooth.

to be rendered elsewhere on the page

I've been taking a look to some patterns and using the off-canvas top makes sense, specially for content that needs to have an horizontal distribution (which will eventually need a general solution on small screens). The left also makes sense and has a much simpler and known responsive solution. Thanks :)

But I'm afraid using this pattern for left&bottom situations could create some issues with OS elements like the Dock&similar in desktops or collide with Drupal elements on the left (like the Toolbar). Would it be possible to limit it somehow to only use the top&right positions?

@ckrina thanks for the feedback.
Update the title because this issue just adds the top rendering.

We need to update the summary to match.

Updated IS as requested on #69.

+++ b/core/misc/dialog/off-canvas.es6.js
@@ -66,6 +66,9 @@
+      // Ensure the previous dialog, if any, is closed.
+      $('.ui-dialog-off-canvas .ui-dialog-titlebar-close').trigger('click');

Removing the code I added in #17 that caused the current bug. This will reintroduce the problem described in #16

@drpal helped me figure this out.

Needed to remove the data-offset-top after displace() is called so when a new dialog link is clicked without closing the previous one it will not pick up the data-offset-top attribute from the previous dialog.

1. +++ b/core/misc/dialog/off-canvas.es6.js
   @@ -119,6 +119,8 @@
   +      Drupal.offCanvas.getContainer($element).removeAttr(`data-offset-${Drupal.offCanvas.getEdge()}`);
   +++ b/core/misc/dialog/off-canvas.es6.js
   @@ -201,20 +211,30 @@
   +        displace();
   +        $container.removeAttr('data-offset-top');
   

   What if displace is initiated elsewhere? Wouldn't that lead into wrong offsets?

2. There's at least one more bug that is easy to reproduce on the test page. The off-canvas height calculations are not correct when trying to open the top panel 1 when off-canvas is already active. I attached a video of this.

@lauriii

#73.1 - Yes. I talked with Ted about this exact problem yesterday. What should happen is that the previous orientation offset data attributes are removed. However currently we don't have a way to know which orientation the tray is transitioning from. We could make some manual attempt to guess previous orientation, or just close the tray in between transitions.

Re #74

We could make some manual attempt to guess previous orientation, or just close the tray in between transitions.

If we close the dialog let's not do it in the same place as before, the dialog:beforecreate event. This has at least 1 known side effect as we saw in this issue but also would probably mess up other module trying to react to that event because it would probably act weird if there listeners came before ours.

The only way I can think to do it right now would be to a click event listener to any off-canvas dialog link that would fire before the regular ajax listeners and close the off-canvas there.

@tedbow Could we store the current open orientation on window.Drupal and just reference that?

- fixes an issue with resize events
- sets a minimum height on the tray element if opened by the top.

I still get visibly different heights when open up the "top panel 1" link.

1. If I open it as the first link on the page it is bigger. The inline style attribute of #drupal-off-canvas has "min-height: 73.4091px"
2. If I open it as the 2nd link it is smaller. The inline style attribute of #drupal-off-canvas has "min-height: 0px"

Found the problem.

+++ b/core/misc/dialog/off-canvas.js
@@ -88,11 +90,16 @@
+      if (position === top) {
+        $element.css('min-height', Drupal.offCanvas.minimumHeight + 'px');
+      }

top here should surrounded in quotes 'top'.
It was never matching.

- fixes incorrect match of top

@drpal 🎉, thanks for the fix.

I tested it manually and everything looks great!

- do not clobber the state object, just set a postition prop instead.

Thank you @drpal! I think we are getting really close on getting this RTBC, nitpicks only at this point.

1. +++ b/core/misc/dialog/off-canvas.es6.js
   @@ -13,6 +13,15 @@
   +     * Storage for position information about the tray.
   ...
   +     * The minimum height of the tray when opened at the top of the page.
   

   Nit pick: should we also document the type?

2. +++ b/core/misc/dialog/off-canvas.es6.js
   @@ -13,6 +13,15 @@
   +    position: false,
   

   We should probably use null instead of boolean to keep the type more consistent.

3. +++ b/core/misc/dialog/off-canvas.es6.js
   @@ -168,11 +180,29 @@
   +      /**
   +       * Only remove the `data-offset-*` attribute if the value previously
   +       * exists and the orientation is changing.
   +       */
   

   Thanks for adding the clarifying documentation! Nit pick: could we change this to use the single line comment syntax instead?

4. +++ b/core/misc/dialog/off-canvas.es6.js
   @@ -238,6 +280,15 @@
   +     * Reset main canvas wrapper and toolbar padding / margin.
   

   Nit pick: s/Reset/Resets

Thank you! Finally I think we are ready! 🚀

Update credits. Removed mentions of bugs from issue summary because as per recent comments those got fixed.

Looked good to me too other than a missing semicolon in toolbar.es6.js that I fixed on commit:

+              $element.on('dialogContentResize.off-canvas', () => {
+                const newHeight = Drupal.offCanvas.getContainer($element).outerHeight();
+                $toolbar.css('margin-top', `${newHeight}px`);
+              })

Thanks all! I'll publish the change record now.

I discovered a small (unrelated) bug while working on implementing the workspace UI using the top position for the off-canvas dialog: #2976287: The off-canvas dialog should have a 1px transparent border

Posted another followup to this issue: #2976385: Provide the ability to wrap the entire page with a border when opening off-canvas in the top position

It would be great to hear the opinion of the product managers and off-canvas maintainers in there :)