This issue has already been reported in this issue within the Zen theme project.
To summarize, the Settings Tray module is adding a HTML div element with the class "offcanvas-lining" within its template named "outside-in-page-wrapper.html.twig", located at outside_in/templates/outside-in-page-wrapper.html.twig. This div is then styled to cover the entire page with absolute positioning and a background color of #333. Similar to the issue above, I also encountered a problem with this when trying to implement my own custom theme by extending the core Classy theme. When the Settings Tray module is enabled while using such a base theme, the entire page is covered by the div mentioned above, such that the entire page is inappropriately covered by a gray box, even when there is neither a Settings Tray overlay opened nor the Editing mode engaged.
This div appears to possible be a vestigial part of the Settings Tray module that can safely be removed without affecting the module's functionality. I'm submitting a patch here for review to do just that.
| Comment | File | Size | Author |
|---|---|---|---|
| #17 | Screenshot 2016-11-09 19.21.10.png | 122.15 KB | tedbow |
| #11 | interdiff-2825154-8-11.txt | 619 bytes | michael_wojcik |
| #11 | removes-offcanvas-lining-div-2825154-11.patch | 1.5 KB | michael_wojcik |
| #8 | interdiff-2825154-4-8.txt | 1.71 KB | michael_wojcik |
| #8 | removes-offcanvas-lining-div-2825154-8.patch | 1.51 KB | michael_wojcik |
Comments
Comment #2
michael_wojcik commentedSubmitting a patch for removing the offcanvas-lining div in question. Initial local testing confirms that this removal does not affect the normal functioning of the Settings Tray module, while successfully getting rid of the offending gray box overlay.
Comment #3
tkoleary commented@michael_wojcik
Let me give you some background on this so you can understand the reasoning behind the off-canvas lining divand maybe we can get to a solution that solves the problem in a way that doesn't impact themes.
The experience that we are trying to effect is demonstrated in this attached video recorded with your patch applied. As you can see the tray slides in but not at it's full height and then 'grows' towards the bottom of the viewport, so there's a moment between the slide in and the 'grow' where you can see a blank gap at the bottom of the tray.
Some constraints around trying to fix this are that the jquery dialog is 'created' and loaded in a single function so it's not possible to add a lining element under the tray with CSS to conceal this effect because any css would only apply after the jquery animation had already run. Our solution was to add this div at z-index -1 and give it the same background color as the tray so that it's already there when the main-canvas wrapper 'shrinks' and reveals what is behind it. Unfortunately as you have seen in Zen, not all themes have a background color set on body as Bartik does and where that is the case you'll see the background color of the lining element.
To make a long story short, the implementation of the lining as it exists right now is clearly not optimal but a patch that removes it should make an attempt at replacing it with code that handles the problem it was created to solve. It may seem like a very minor visual artifact that passes quickly, but it looks sloppy and gives a poor impression of finish that hurts the overall experience of the feature.
Comment #4
michael_wojcik commented@tkoleary
Thanks very much for the background information and demo video! That was super helpful!
I've modified my original patch to add an additional fix to the issue you mentioned. So, the patch is still removing the old offcanvas-lining div and its CSS. But it is now also taking advantage of the jQuery UI API being used for the dialog boxes in question, such that a correct height is set as an option on each setting tray before it is created/rendered.
I've included the new patch and an interdiff for your reference.
Cheers,
Michael
Comment #5
tkoleary commented@michael_wojcik
Perfect. Nice work!
Comment #6
xjmI like having less code. I also confirmed that we use JS like this for the window height in
misc/dialog/dialog.position.jsand inoffcanvas.js.That made me wonder, though. Does this JS belong in offcanvas and not in Settings Tray's implementation?
Comment #7
tedbow@michael_wojcik thanks for the fix!
re @xjm
Yes this should be in offcanvas.js
Adding as child of #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it
All the child issue need to be take of first.
Comment #8
michael_wojcik commented@tedbow
@xjm
Thanks for the feedback! I've re-rolled the patch again, to move the JS to
offcanvas.js. I hope that's what you meant! If not, I might need a bit more background. Thanks again!Comment #9
michael_wojcik commentedComment #11
michael_wojcik commentedMinor fix to last patch, based on failed automated testing.
Comment #12
tedbow@michael_wojcik looks to me! Thanks!
Comment #15
xjmThanks @tedbow and @michael_wojcik!
Committed 6a1f068 and pushed to 8.3.x and 8.2.x (since this is an alpha experimental module, we can make these changes in 8.2.x as well). Thanks!
Also retroactively tagging as including JavaScript so the JS maintainers can see this issue. The fix makes sense to me and seemed straightforward enough that I did not block this issue on a JS mainainer review, but just in case.
Comment #16
michael_wojcik commented@xjm
That's great news, thanks!
BTW, I'm a new core contributor, so I'm not sure how things work, but will this commit show up on my profile as something I authored? I was really excited to get my first core commit :-)
Comment #17
tedbow@michael_wojcik won't show up as authored but will add to your credited count on your profile with a link to your issues.
It's already there:
Nice work!
Comment #18
tkoleary commented@michael_wojcik
Congratulations! Nice work.
Comment #20
tedbowChanging to new settings_tray.module component. @drpal thanks for script help! :)