"Offcanvas" tray should be using the existing dialog system

Attached a very rough patch that uses core dialogs. Bonus is the ability to resize the sidebar.

Using dialogs is good because it'll help with a11y and once we figure out what todo with them on mobile, this will benefit.

fix some stuff in dialog.ajax.js

Make use of the (renamed) OpenOutsideinDialogCommand ajax command to clean up dialog settings.

Reroll and removing of #2784463: Convert outside_in_page_(top|bottom)() to a #theme_wrappers altogether since we don't need any extra wrapping.

I like this issue ... less code, by using an existing pattern is a good thing.

Two comments :-

1)
From #6

we don't need to extra wrapping

Good I hope we can remove it ....

(A) There are still references to #main-canvas in the css.

In one case I can use (A) to explain a visual regression - namely

(B) In edit mode all blocks should have "dotted-line box styling". that toggles with the edit button

There are lots of other outside_in issues waiting in the wings to style the tray and add throbbers etc

#2784513: Off-canvas tray requires feedback on load
#2784437: Clean up CSS in outside_in module

So I want to be limited in the restoring all the visual function of the tray --- but I think we should fix (B)

Let me describe what is going wrong with an example

#block-bartik-branding - the div that says by default "Site-Install"

it is a "contextual region" that has the dotted-line border because this css selector is matched.

"#main-canvas.js-outside-in-edit-mode .ourside-in-editable"

#main-canvas has gone away so we need to refactor the js.

2)

I like the injection of Drupal.debounce and Drupal.displace. that is good news for flexibility.

BUT there is still a call in outside_in.js bodyPadding() to Drupal.displace() which needs to be converted into a displace() call for consistency.

@tedbow

I agree with you --- I see lots of the problems you describe

I was taking the line the patch was a good start but there are lots problems

I was focusing on the issues I could isolate and just building up information.

Thanks for the tip about caching ... I am well aware .. I was working around those.

I will have more time tomorrow to contribute and will try and work in away that takes into account your perspective and prefered order of doing things.

coordinating things remotely is tricky...

Are we talking about modal or non-modal dialogs here?

This is starting to block various other issues, so escalating to major.

@nod_

Do you think this negates the need to transition the state management of the off canvas tray to Backbone? #2784935

Great, that now functions much better.

it is now appropriate to fold in the stuff from #7

1) I have made a start on #7.1 -- fixing references in the css to #main-canvas etc.

When edit mode is toggled the dashed borders now return.

2) #7.2 fixed

3) like ajax.js, block.js I want "window" to be injected into the js library.

@tedbow

Looks good, but of course with the refactor there are a bunch of UX regressions. Here's what I can see at first look.

Visual and interaction regressions

• Tray does not load with animation (slide in) or unload (slide out)
• Black background is visible behind tray on slide out
• No hover affordance on editables
• Tray using jquery close icon not core icon
• Table is again overflowing details pane and tray
• All styling from outside in CSS in the tray is gone (I expect we just need to re-name the parent class)

And new issues introduced

• There is a "flash" of black (the background) when clicking from one block to another as the new form loads
• Contents of expanded details elements overflow buttonpane. .ui-dialog-buttonpane needs to be z-indexed above the form
• Inability to toggle out of editing mode without a force refresh is still happening

When used with Place Block

• Triggering place block forces tray closed
• Toggling off place block forces tray closed

When used with Quickedit

• When quickediting a node it is still possible to click a block opening the tray and the node editing toolbar and outlines do not disappear.
• When the tray is open and quickedit is triggered on a node the tray remains open.

Sorry, just being the tag fairy.

Few adjustments:

* Drupal.attachBehaviors expects a DOM object as first argument, not a jQuery object.
* we don't use $.each for iterating over an array, it's not safe. Refactored to use array functions.
* we don't inject window in the closure. If someone tries to run this in a non browser env, they'll have other problems. and window is not used in that closure. We shouldn't be using it that much anyway, so not much value into adding that to a closure.

getEdge doesn't need to be a function, if the rtl attribute changes on <html> it means the language changed and the page was reloaded.

var dir = (document.documentElement.dir === 'rtl') ? 'left' : 'right';

Is enough. document.documentElement exists wherever the JS is, header or footer, no need to put that in behavior or delay the evaluation.

Personally, I disagree this idea.

From concept side,
`Offcanvas` has its own pattern, and different to Modal & Dialog. #2672344: [UX] Open as Modal should not able to interact with other elements. For example, we called DROPDOWN as DROPDOWN, not Dialog. (It's also opening dialog-like element but different styling)

From code side,
We can see the patch doesn't reuse much of the code from Drupal.dialog but make it bloated.

+++ b/core/modules/outside_in/js/offcanvas.js
@@ -3,129 +3,103 @@
+    'dialog:beforeclose': function (event, dialog, $element) {
+      $(document).off('.outsidein');
+      $(window).off('.outsidein');
+      $('body').css('padding-' + edge, 0);

All these events should not be global. Because when we make it to standalone lib, we able to open multiple dialogs inside and do whatever crazy actions.

We use everything from Drupal.dialog, except dialog.position.js. The code in 8.2 is trying to do many things already done by Drupal.dialog such as a title bar, a close button and in the queue there is an issue to close the "offcanvas" with ESC key or another one to improve a11y.

The dialog is not opened as a modal, we can open several on top of each others so #2672344: [UX] Open as Modal should not able to interact with other elements doesn't apply.

We can open many modals but not many offcanvas modals (at least that is what I understood). We need to bind stuff to window since resize, dialog:* events are triggered on this. And document is needed for drupalViewportOffsetChange.

And look at the resulting code, it's much simpler after the patch. The only big piece of code is the positioning and it can't reuse what core does because it's just not the same.

@tedbow

Removes CSS for the close icon. Since we are using the standard dialog isn't there icon good for us.

I don't think so. I believe the standard jquery dialog close icon is a png. For retina we want the svg from core/misc/icons.

@tedbow

I think you can safely remove all the media queries from the offcanvas css files eg. @media (max-width: 700px) etc. since that's all handled by breakpoint module and drupal displace.

I'm lost on these outside_in issues. I could catch a bug within 10 sec after enabling the module. But we kicked it in. :s It must be some reason for that, I don't try to block its unusual workflow anyway :)

Same as outside_in, it's easy to find a wired bug:

I agree with @nod_ the intention of the offcanvas tray is there should only ever be 1 open at a time.

But you able to open a dialog inside, the dialog close will trigger above listeners.

We have to think about: "Offcanvas" vs "Offcanvas for outside_in"
As standalone Offcanvas, you won't put title bar & bottom bar there. Almost all sites I've seen, they don't. And now we're adopting the CSS from dialog, you will write a lot of custom CSS to revert these inherited rules. (Remembering that its loading jQuery UI also)

Also, we can see current refactor doesn't provide same rendering behavior ( the anime ). And this complicated logic from JS side will be another headache for themers.

standalone `Offcanvas` should only take the main API to render the content inside the canvas. And let outside_in define its pattern: title & bottom bar.

@tedbow

Just a few minor things.

1. +++ b/core/modules/outside_in/js/offcanvas.js
   @@ -1,131 +1,131 @@
   +  var edge = (document.documentElement.dir === 'rtl') ? 'left' : 'right';
   

   The initial condition doesn't need to be wrapped in parenthesis.

2. +++ b/core/modules/outside_in/js/offcanvas.js
   @@ -1,131 +1,131 @@
   +      position: {
   

   Maybe some documentation around this confusing positioning format?

[duplicate comment]

@tedbow

The z-index and position of the button pane are working great now but when there is overflow, for instance when tools menu has both details panes open, there are three scrollbars.

Since #drupal-offcanvas has overflow scroll, I think we can safely set div.dialog-offcanvas to overflow: hidden, since the dialog javascript will always calculate the width of the three children. That way most of the time there will be only one scrollbar, and at most two, when there is overflow.

@tedbow

Also this is exposing some core technical debt in quickedit where z-indexes are all off with contextual links above entity toolbar and entity toolbar below offcanvas tray:

Making quickedit toolbar container z-index:502 (or higher) fixes it.

The good news is that now that it's using Drupal displace the edge detection is "pushing" the entity (quickedit) toolbar away from the offcanvas tray.

We have a follow up for #58, we should be good to go.

#2790285: Contextual link triggers displayed above the Quick Edit's toolbar

I agree this is RTBC, but I do want to reply to droplet and make sure we're on the same page before it gets in though so I'll let him RTBC.

On the CSS bloat from the jquery ui dialog that's a fair complaint and I kinda agree. We'll see how bad this in #2781577: Properly style outside-in off canvas tray if it's too bad we can always go back or find a different solution, it's experimental after all.

On the rendering behavior, the ajax framework has everything needed to let us replicate it. It's not great DX and not really documented but it's there. Might get some work to get working but it's doable.

My main concern is the creation of a new API while exploring the UX of the feature. we have tools that can do most of the job and I really want us to use what already exists and see where the limitations are. If a new API is needed we'll have a better idea of what it should do and how if we start by using dialogs.

I was surprised at the state this was committed too, but it does mean we can do pretty much whatever we want from here.

( edit ) for example, I was pretty sure we'll need to add identifiers to modals (or some sort of tags) to avoid too many $element.is('#whatever'); from what we do in this patch it's pretty clear we need a way to handle that properly.

The basic feature is fine now. Let's move forward. :)

But I still aginst the change to jQuery UI Dialog.
Note that: Mainly, I'm talking about `Offcanvas pattern` rather than `Offcanvas for outside_in`

The problem with this changes:
- (Although we don't have a plan, I think we have the intention to remove Drupal.dialog [jQuery UI] in the future versions and keep Drupal.dialog close to HTML5 Dialog as possible.)
- Bloated CSS, extra JS loading.
- Default style in dialog (`ui-dialog ui-widget ui-widget-content ui-corner-all ui-front`) shared across all Dialog instances.
- debounced positions from JS provided bad experiences on windows resizing.
- Themers got mad! It seems almost impossible to implement nice CSS anime (even from JS side). (It's pretty important for Offcanvas pattern, and it's why we called it Offcanvas. With current change, this is Side PopUp).

Just take a record, my Offcanvas idea ( Similiar to unpatched ):

<!-- toggling the offCanvas -->
<button class="button button--offCanvas" data-drupal-offcanvas="{target:'#module-A', position:'left'}">OPEN IT</button>

<!-- offCanvas layout: `Offcanvas.renderContainer()` -->
<div id="offCanvas" class="offCanvas">
    <div class="offCanvas__content">
    </div>
</div>

EXAMPLE with Module:
<!-- offCanvas layout -->
<div id="module-A" class="offCanvas offCanvas--ModuleA">
    <div class="offCanvas__content">
	
		// new component, do not related to parent component. `Offcanvas.render()`
		<div class="offCanvas-ModuleA">
			<div class="offCanvas-ModuleA__title"></div>
			<div class="offCanvas-ModuleA__content"></div>
			<div class="offCanvas-ModuleA__footer"></div>
		</div>
		
    </div>
</div>

// @flow

// === Defined OffCanvas elements.
Offcanvas.open()
Offcanvas.close()
Offcanvas.destroy()
Offcanvas.render(content: string, parentSelector) {
	$(parentSelector).find('.offCanvas__content').html(content)
}
Offcanvas.renderContainer() {}

Offcanvas.init(selector, content: string) {
	let parentSelector = selector
	
	if(!$(selector).length) {
		// setup container layout.
		Offcanvas.renderContainer();
	}
	
	Offcanvas.render(content)

	// init events..etc
	
	return {
		open,
		close,
		destroy,
		render
	}
}

// Optional
Offcanvas.positionHelper() {}

// Ajax system
Drupal.AjaxCommands.prototype.OffCanvas() {}

// Simple Decoupled  API
Drupal.CanDoWhatEverCrazyActions() {}



// === Outside_In
OffcanvasOutsideIn.container(layoutHTML: string): string {}

// Appended to `OffcanvasOutsideIn.container`
OffcanvasOutsideIn.header(html: string) {}
OffcanvasOutsideIn.content(html: string) {}
OffcanvasOutsideIn.footer(html: string) {}

Offcanvas.init('#OffcanvasOutsideIn', OffcanvasOutsideIn.container)



// === Module MENU
OffcanvasMenu.menu(html: string): string {}
let moduleMenu = Offcanvas.init('#module-menu', OffcanvasMenu.menu)

// Switching to Menu B
let menuHTML = fetch();
moduleMenu.render(menuHTML)
moduleMenu.open()



// === Module B
OffcanvasCustom.infoPanel(html: string): string {}
Offcanvas.init('#module-B', OffcanvasCustom.infoPanel)



// === Large screen.
// Rendering from server side, open by default. Narrow down window to Offcanvas pattern.
Offcanvas.init('#largeScreen')

- It able to borrow some layout hacks from Toolbar vertical mode.
- Allowed multiple instances (eg. main menu + Offcanvas for setting is a common pattern). Reducing DOM manipulation.

I see what you mean, it makes sense for the standalone offcanvas feature. For now I don't want to add another api to maintain so let's go with it for now.

Can you repost this in #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it? If it goes badly in the CSS issues and the others we can always go with your version.

Cool. In general, I can predict it won't fulfill most themers needs or my needs above. ServerSide Rendering & CSS Anime are headaches. We hit the same issue in the current patch right? it dropped the CSS anime.

One more note:

Scandalized the APIs, considering to adopt the Dialog pattern now:

Offcanvas.open() {
	dialog:beforecreate
}

Offcanvas.close() {
	dialog:beforeclose
}

Offcanvas.render(DialogCallbackFunction) {
	DialogCallbackFunction()
	Drupal.dialog()
	// ..etc
}

Offcanvas.init() {
	Offcanvas.render()
}

Drupal.AjaxCommands.prototype.OffCanvas() {
	// copy patched code here
}

(Sorry I can't provide a patch to demo it due to my personal schedule)

- Very similar concept to Drupal.dialog but I think hacking the Drupal.dialog would be harder. It's shared with 3 types.
- Less messed with Dialog event system and not limited by HTML5 Dialog SPEC.
- It allowed internal refactoring even after leaving experiment I think.
- No extra maintenance than Drupal.dialog but better API design.

I always see developers overridden Drupal.dialog.show & Drupal.dialog.close. They won't trigger events properly. Less or more, I also saw it in quickedit / edit modules. Our dialog is messed up before this patch. It made us very busy to test all 3 components with simple changes.

Makes sense. Agreed. Lets open a follow-up.

Don't need a patch in the other issue, just the comment and pseudo code is enough.

While my JavaScript level is approximately -1000, since this is a patch towards an experimental module in core, and it has sign-off (even if begrudging sign-off ;)) from both JavaScript maintainers, I think I'm comfortable committing this. This unblocks important other work going on in terms of accessibility and bug fixes, and its absence is making small improvement patches really hard to review.

I played around with the interaction and can't seem to break it. I know Kevin was very concerned that the animation is not as smooth as it used to be, but I wasn't able to immediately perceive that myself (as a non-designer). That also seems like something we can either try and get in before RC1 (though time is ticking) and/or a nice thing we can mention in the release notes of 8.2.1+ if we can't make it in time.

Committed and pushed to 8.2.x and 8.3.x. Thanks!

The idea of having a wrapping API is a good one, and I think the more things that get converted to this pattern, the more we'll be able to see the need for it. We still need a follow-up for that, so tagging.