When we do have panelizer On!!!!

$editables = $('[data-drupal-outsidein="editable"]').once('outsidein');
      if ($editables.length) {
        // Use event capture to prevent clicks on links.
        <strong>document.querySelector('#main-canvas').addEventListener('click', preventClick, true);</strong>

Comments

RajabNatshah created an issue. See original summary.

RajabNatshah’s picture

When we do not have it too.

droplet’s picture

Component: ajax system » outside_in.module
Status: Needs work » Active
Issue tags: +JavaScript

Needs a check on #main-canvas.

It's a PHP side bug tho. In some cases #main-canvas won't apply to (custom) theme

@RajabNatshah, can you tell us more about your Drupal site configs

RajabNatshah’s picture

tedbow’s picture

@droplet @RajabNatshah custom themes should still have because of outside_in_element_info_alter. I tried a few contrib themes and they work.

I tried vartheme which is inside varbase but the install profile itself. The #main-canvas wrapping is there.

RajabNatshah’s picture

Thanks Ted,
Your feedback on this issue is rewarded :)

Varthem is a sub theme of bootstrap theme.
I will keep testing and troubleshooting this issue.

Rewarded time :)

tedbow’s picture

Status: Active » Postponed (maintainer needs more info)

@RajabNatshah thanks for looking into it more. I am going to postpone this issue just to keep track better.

Please re-open if you are able to figure out more.

The more you can isolate the problem the better. varbase as a distribution probably has a lot going on. So if we could figure out if is the theme or some module that is enabled during the install that would help.

tkoleary’s picture

@RajabNatshah

Settings tray is currently not working with Bootstap or any subtheme of bootstrap because bootstrap completely removes and replaces jquery dialog. It's likely that Bootstrap will not support settings tray until it is no longer experimental.

RajabNatshah’s picture

Than you Terrence,

We had this fixed for Bootstrap at this issue.

#2821008: [bootstrap] Add support for the Settings Tray (outside_in) experiemental module

Left to right with Bootstrap
outside in ltr

Right to left with Bootstrap
outside in rtl

The patch is undergoing under the issue #2821008: [bootstrap] Add support for the Settings Tray (outside_in) experiemental module

tedbow’s picture

Status: Postponed (maintainer needs more info) » Closed (duplicate)
Related issues: +#2826726: Settings tray will not trigger in Bootstrap
droplet’s picture

Status: Closed (duplicate) » Active

Needs a simple check on #main-canvas.
e.g.
if(!$('#main-canvas').length) { return; }

Or throw a compatibility warning to help themers

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tedbow’s picture

Status: Active » Needs review
FileSize
790 bytes

We are no longer using #main-canvas here.

We are now using an attribute data-offcanvas-main-canvas set up in outside-in-page-wrapper.html.twig.

I added a comment in outside-in-page-wrapper.html.twig warning that you cannot remove this attribute. So a themer would be warned when they copied the default template from the module to override.

Re @droplet's comment

Needs a simple check on #main-canvas.
e.g.
if(!$('#main-canvas').length) { return; }

Or throw a compatibility warning to help themers

Since we are not using a class which might be changed and warning about the attribute I think this is not needed anymore.

drpal’s picture

Version: 8.3.x-dev » 8.4.x-dev
Status: Needs review » Reviewed & tested by the community

I agree that the comment is probably the way to go here.

webchick’s picture

Assigned: Unassigned » droplet
Status: Reviewed & tested by the community » Needs review

Assigning to droplet for his 2 cents.

droplet’s picture

Assigned: droplet » Unassigned

In my opinion, it still worth to stop the error. (I think it will allow to opt-out to use Setting Tray for some pages for whatever reasons)

It has another problem. When the [data-offcanvas-main-canvas] is missing, the top toolbar "EDIT" button still there. It leads further errors.

So the whole point is: Do we allow [data-offcanvas-main-canvas] is missing when the module is enabled?

drpal’s picture

Alright, this throws an error the start of setEditModeState() if you've removed data-offcanvas-main-canvas from the template. One thing to keep in mind is that the edit-mode functionality is prohibited, but the dialog system effects will still run.

tedbow’s picture

Status: Needs review » Reviewed & tested by the community

re #16

In my opinion, it still worth to stop the error.

#17 covers this. I test it does through a Javascript console error.

(I think it will allow to opt-out to use Setting Tray for some pages for whatever reasons)

Edit mode will not work if the attribute is not there. I think per page disabling of Settings Tray functionality is beyond the scope of this issue about Javascript error.
That should be separate Feature Request issue if needed. It should also probably be solved on the server side by loading JS files all together.

It has another problem. When the [data-offcanvas-main-canvas] is missing, the top toolbar "EDIT" button still there. It leads further errors.

I think the warning in the twig file is sufficient. If someone is removing the attribute they should see the warning in the code comment. If they don't they will then see the error in the Javascript console.

If think if they go through all that and don't want errors they need to do library alter and provide their own JS files that work without the attribute.

So the whole point is: Do we allow [data-offcanvas-main-canvas] is missing when the module is enabled?

I would say no and but if that would be necessary if should be different issue.

Going to mark this as RTBC. Thanks all for work on this!

droplet’s picture

OK. So we have a warning from JS side now.

I think the warning in the twig file is sufficient.

It's never enough. I think reporter of this issue is the one who never touches the twig but gets errors. We have many developers never understand how to trace down to error source yet.

I would say no and but if that would be necessary if should be different issue.

I think it again. It sounds right. If you want to opt-out, then you should hide the EDIT button instead.

Thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 17: 2811717-17.patch, failed testing.

tedbow’s picture

Status: Needs work » Reviewed & tested by the community

Drupal CI error

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 17: 2811717-17.patch, failed testing.

himanshu-dixit’s picture

Status: Needs work » Reviewed & tested by the community

This looks good to me. Setting back to RTBC again

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I'm concerned that this is RTBC and #2830882: Abstract off-cavs.js (Settings Tray) JS to utilize Drupal.dialog instead of jQuery UI directly is postponed. #2830882: Abstract off-cavs.js (Settings Tray) JS to utilize Drupal.dialog instead of jQuery UI directly feels like a critical for outside to do before the module graduates from experimental. Before we commit this we need to clarify what is in scope for the outside module since committed this doesn't solve the original issue that cause the reporter to file it.

Marking as needs work to to get agreements as to the status of #2830882: Abstract off-cavs.js (Settings Tray) JS to utilize Drupal.dialog instead of jQuery UI directly.

dawehner’s picture

I think what we need is a planning issue describing the steps we have to do to get it stable. According to @drpal there was some dicussion and a plan which wasn't posted yet.

tedbow’s picture

@dawehner yes you are right. I will work on issue or point to existing about getting this module stable.

UPDATE: This issue has must haves for this to get stable #2762505: Introduce "outside in" quick edit pattern to core

tedbow’s picture

Issue summary: View changes
Status: Needs work » Reviewed & tested by the community

@alexpott I agree #2830882: Abstract off-cavs.js (Settings Tray) JS to utilize Drupal.dialog instead of jQuery UI directly should be resolved.

We have been working on that issue.

We have a patch https://www.drupal.org/node/2830882#comment-12068569

Getting initial feedback from @markcarver the bootstrap theme maintainer that it is generally in the right direction. It also appears that patch will not affect this problem.

Before we commit this we need to clarify what is in scope for the outside module since committed this doesn't solve the original issue that cause the reporter to file it.

I think the patch does solve the initially reported issue.
document.querySelector('#main-canvas').addEventListener('click', preventClick, true);
Was throwing an error. This was because document.querySelector('#main-canvas') returned no element.

The selector has now been changed to '[data-offcanvas-main-canvas]' because of the other commits. But the same problem remains. It would throw an error if the template was overridden and the querySelector return no elements.

We are doing 2 things to mitigate this:

  1. +++ b/core/modules/outside_in/js/outside_in.js
    @@ -124,6 +124,9 @@
    +    if (!document.querySelector('[data-offcanvas-main-canvas]')) {
    +      throw new Error('data-offcanvas-main-canvas is missing from outside-in-page-wrapper.html.twig');
    +    }
    

    Explicitly throwing an error to make these easier to debug.

  2. +++ b/core/modules/outside_in/templates/outside-in-page-wrapper.html.twig
    @@ -3,7 +3,9 @@
    + * "data-offcanvas-main-canvas" attribute is required by the off-canvas dialog.
    + * It cannot be removed without breaking the off-canvas dialog functionality.
    

    Adding a template comment to inform the themer not remove the attribute needed.

This solves the initial problem in the sense the themer would have to ignore the template comment, remove the attribute, and ignore the console error to still encounter this problem.

There is only so much we can do. Obviously if you can break this module or others if you are very careless in overriding templates.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 17: 2811717-17.patch, failed testing.

tedbow’s picture

Status: Needs work » Needs review
FileSize
1.38 KB
1.48 KB

Test failed because the actual attribute changed from data-offcanvas-main-canvas to data-off-canvas-main-canvas in #2862625: Rename offcanvas to two words in code and comments.

Fixing

drpal’s picture

Status: Needs review » Reviewed & tested by the community

Alright. I think the discussion happening in #2830882: Abstract off-cavs.js (Settings Tray) JS to utilize Drupal.dialog instead of jQuery UI directly shouldn't affect the status of this. With the fix for the test, I think this is good.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 29: 2811717-29.patch, failed testing.

tedbow’s picture

DrupalCI database server error so re-testing

drpal’s picture

Status: Needs work » Reviewed & tested by the community

  • lauriii committed e67f64e on 8.4.x
    Issue #2811717 by tedbow, drpal, RajabNatshah, droplet, alexpott: [...
lauriii’s picture

Status: Reviewed & tested by the community » Patch (to be ported)

Thanks @tedbow for the great summary on #27. I think this is the best way to address the original issue report. Big +1 from me for throwing useful errors helping developers to fix bugs instead of just silently failing.

Committed e67f64e and pushed to 8.4.x. Thanks!

I think this could be backported to 8.3.x since this is only affecting experimental modules and the error only appears in situations where another error would have happened anyway. This will likely need a new patch for 8.3.x because of #2862625: Rename offcanvas to two words in code and comments. .

lauriii’s picture

Version: 8.4.x-dev » 8.3.x-dev
tedbow’s picture

Status: Patch (to be ported) » Reviewed & tested by the community

@lauriii thanks for committing!!!!

This will likely need a new patch for 8.3.x because of #2862625: Rename offcanvas to two words in code and comments.

Just tested and the patch applies cleanly to 8.3.x because #2862625: Rename offcanvas to two words in code and comments. was also applied to 8.3.x.

For now 8.3.x is receiving all the backports because we are keeping all the changes in the experimental module folder. So hopefully all patches will apply to both 8.4.x and 8.3.x

These could change with #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it but that will be one of the last things before we get the module stable.

  • lauriii committed 657a612 on 8.3.x
    Issue #2811717 by tedbow, drpal, RajabNatshah, droplet, alexpott: [...
lauriii’s picture

Status: Reviewed & tested by the community » Fixed

I got confirmation from @xjm that this is eligible for backporting given that this only modifies experimental modules. Cherry picked 657a612 and pushed to 8.3.x.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.