It's a major maintenance pain, doesn't work on mobile (we had to make a killswitch for it to not open on mobile), relies on an old js library to handle url hash changes instead of the standard and cleaner pushState (not available for IE9 so core couldn't use it anyway, but contrib could). And as webchick very rightly points out: "once the Seven style guide is all done, there's a clear enough boundary between "front-end" and "back-end" that the extra chrome is no longer needed".

List of issues that can't be fixed properly because of overlay:
#1129578: Overlay doesn't respect internal anchor links
#2078951-19: Highlight the row of block that was just placed

Other made really complicated because of it:
#1440628: [Meta] the big javascript toolbar/tableheader/fragment mess
#655388: Many ways to lose data on form input in the overlay

And that's just the open ones.

Related: #1889150: Simplify overlay look, *only use for contextual operations*.

Files: 
CommentFileSizeAuthor
#56 2088121-56-remove-overlay.patch184.29 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 58,983 pass(es).
[ View ]
#2 core-overlay-remove-2088121-2.patch121.28 KBnod_
FAILED: [[SimpleTest]]: [MySQL] 58,819 pass(es), 5 fail(s), and 6 exception(s).
[ View ]

Comments

webchick’s picture

Issue tags:+Usability

I'm staying out of this one, but in fairness, I also pointed out that Overlay is quite handy from contextual links, and that most of the rage is because it opens all the time everywhere. So I'm +1 to the "use only for contextual operations." And I believe I said the chrome "could" be no longer needed, not "is." :)

nod_’s picture

Status:Active» Needs review
Issue tags:+JavaScript
StatusFileSize
new121.28 KB
FAILED: [[SimpleTest]]: [MySQL] 58,819 pass(es), 5 fail(s), and 6 exception(s).
[ View ]

And here is a patch. Sorry about the misquote webchick.

Just looking at how much code there is all over the place to cater for overlay kinda points to the issue as well.

jibran’s picture

+1 and I want to RTBC it.

Status:Needs review» Needs work

The last submitted patch, core-overlay-remove-2088121-2.patch, failed testing.

LewisNyman’s picture

I was really into the idea of using a stripped down overlay for just the contextual links. #1889150: Simplify overlay look, *only use for contextual operations*

tsphethean’s picture

I'd support removing overlay, disabling it is usually the first thing I do on a site.

nod_’s picture

To be fair, this other issue turns the overlay into something way, way better #1889150: Simplify overlay look, *only use for contextual operations*.

Gábor Hojtsy’s picture

Funny thing is the Spark team at Acquia worked really hard to effectively remove the overlay in Drupal 8 (especially @tkoleary and myself). The problem with removing the overlay wholesale is we still think contextual operations make sense (and they should let you quickly get back to your page). So if you hit the "Configure block" contextual link on a block, a *modal dialog* should open and let you do your stuff, submit and close. If the contextual link leads you to a whole new page, that is not very useful. So why not use the Drupal 8 modal for these thing?. We reiterated this *several times* in our meetings and came to these two key points:

- these modals need to show up on a frontend themed Drupal website; we don't have a 100% surefire way to ensure we can display a modal that is themed like a backend UI and does not interfere with the underlying page or vice versa; and Drupal can swap your backend looks independent of your frontend looks, so these are roles of two different themes; an iframe serves this purpose of sandboxing in HTML basically
- the CSS technologies available to do responsive pages does not let us work with "width of a part of the page", it only let us work with width of the whole page; so if the backend modal content needs to adapt to the available width of the modal, it can only do so without JS fiddling if the modal is an iframe

Then it would theoretically still be possible to put an iframe into a modal and ignore the overlay, but then the whole content of the modal would be an iframe, so the modal would have very tiny use in the whole process and most of its backend would need to be ignored and a whole iframe based thing implemented. Which is .... already implemented in overlay.

So therefore the plan in #1889150: Simplify overlay look, *only use for contextual operations* was to make the overlay totally look like a modal and only use it when a modal needs to happen on top of the frontend. It would not be the overlay as we know it in Drupal 7 (and currently in 8). @tkoleary posted this great piece at https://groups.drupal.org/node/294848 explaining all the things.

The funny thing is Spark has been really putting in lots of time and resources to make the overlay go away :) There is maybe not much demonstration of the community opposition to that idea in the groups post, but if you look at Ryan Weal's related issue to not include the overlay in the standard profile at #2004836: Disable overlay in standard profile you'll see how much community opposition there is.

In the end this was de-prioritized from the Spark queue to avoid fighting with community members who wanted to keep the overlay. Such irony.

falcon03’s picture

YAY, +1 for removing the overlay. I've always been against it since I started using Drupal! :-)

If it can't be removed, then its usage should be restricted to contextual operations. Maybe (but this is only a random thought that should be verified) restricting its usage would make it easier to use for screen reader users! :-)

And, if its usage can't be restricted to contextual operations, then it should be disabled in the standard profile. Not to mention that using Voiceover + OS X + Safari it can *easily* make the browser crash! Not sure if this depends on my local environment, though.

ParisLiakos’s picture

i wish that dream would ever come true:P
+10000
contextual links or not..just ?destination works for me there.

related #2004836: Disable overlay in standard profile

geerlingguy’s picture

Major +1 from me, too. If overlay wants to exist, it should exist in contrib. It's much too fragile to be part of core.

To add to the list of things overlay makes harder:

  • searching within the overlay using the browser's search is inconsistent, and occurrences of the search in the page behind the overlay are also found.
  • refreshing pages where an overlay is displayed causes two Drupal page loads—one for the page behind the overlay, one for the overlay. This can sometimes cause disastrous results, if the page behind does something strange.
  • scrolling can behave strange when inside the overlay.
brianV’s picture

I'll +1 the basic idea as well here.

I've found the the overlay mostly just interferes with the development process, can have inconsistent behavior at times, and consequently is usually the first thing I shut off when starting a new client site.

The only time I've ever intentionally enabled overlay was for a client who really liked the 'shiny stuff', and they were impressed by overlay.

I'm all for downgrading it to contrib so the people that want it can have it, while not adding a burden to the core development team.

LewisNyman’s picture

Every feature is a burden on the "core development team".

The main benefit of the overlay is for new users, new users don't even know what a contrib module is. Moving it to contrib just because it's annoying for experienced devs to turn it off is backwards logic.

catch’s picture

While every feature requires maintenance, there are plenty of features in core that core developers use regularly - whether it's the comment form, Seven, or the Views UI.

There's a massive difference between maintaining something you use, vs. maintaining something you're not bothered about, vs. maintaining something you go out of your way to avoid/disable.

RobLoach’s picture

Issue tags:+Platform Initiative
LewisNyman’s picture

There's a massive difference between maintaining something you use, vs. maintaining something you're not bothered about, vs. maintaining something you go out of your way to avoid/disable.

I think this is where we disagree. Core development is not just for core developers, we build a framework for a lot of users who do not develop for core.

I'm not saying it's not a valid factor, maintenance is always an issue, but it should not be the only factor in a decision.

catch’s picture

Core development is not just for core developers, we build a framework for a lot of users who do not develop for core.

Yes we do. And that's why usability testing is great - to ensure that core UIs don't cater for core developers. Again that's completely different from adding/keeping UIs that core developers actively avoid using because they get in the way.

webchick’s picture

Priority:Normal» Critical
Status:Needs work» Postponed
Issue tags:+revisit before beta

We talked about this on IRC, and I believe we agreed to do this.

Postponing on #1889150: Simplify overlay look, *only use for contextual operations* since that eliminates 99.9% of the complaints with Overlay.

If that doesn't get done by release, then we'll revisit this one.

RobLoach’s picture

Status:Postponed» Needs review
StatusFileSize
new121.26 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2088121.diff. Unable to apply patch. See the log in the details link for more information.
[ View ]
RobLoach’s picture

Status:Needs review» Postponed

Didn't mean to change status.

RobLoach’s picture

StatusFileSize
new124.31 KB
FAILED: [[SimpleTest]]: [MySQL] 58,682 pass(es), 7 fail(s), and 2 exception(s).
[ View ]

Meant to upload the .patch, not the .diff.

YesCT’s picture

Status:Postponed» Needs review

For testbot

YesCT’s picture

Status:Needs review» Postponed

Back to postponed

nod_’s picture

Viable alternative being worked on in #787896: Add a link so that administrators can return to their most recently visited non-admin page that would enable us to remove the overlay. Patch possible to try out in #2098713: Overlay alternative. to see what the behavior looks like.

nod_’s picture

Status:Postponed» Needs review
StatusFileSize
new180.13 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Removed jquery.bbq as well (yay!) New JS in tour module needs testing for multipage and tips filtering.

Just want to know what testbot says. git said: 43 files changed, 29 insertions(+), 4529 deletions(-), not bad :D

Status:Needs review» Needs work

The last submitted patch, core-overlay-remove-2088121-25.patch, failed testing.

nod_’s picture

Status:Needs work» Needs review
StatusFileSize
new184.92 KB
FAILED: [[SimpleTest]]: [MySQL] 59,496 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Haha, I did not expect new files to be added to the overlay module :)

Status:Needs review» Needs work

The last submitted patch, core-overlay-remove-2088121-27.patch, failed testing.

brantwynn’s picture

nod_’s picture

Status:Needs work» Needs review
StatusFileSize
new185.15 KB
PASSED: [[SimpleTest]]: [MySQL] 59,585 pass(es).
[ View ]
<swentel> nod_: that's because you remove entries in drupal-7.user_data.database.php
<swentel> it's asserting that the upgrade of that is ok

Changing a 5 in a 2, let's see what happens.

brantwynn’s picture

Status:Needs review» Needs work

I don't think everything is gone yet. Using ack to search for overlay found a few things.

core/modules/system/system.api.php
2967:  // If the current page request is inside the overlay, add ?render=overlay to
2968:  // the success callback URL, so that it appears correctly within the overlay.
2969:  if (overlay_get_mode() == 'child') {
2971:      $batch['url_options']['query']['render'] = 'overlay';
2974:      $batch['url_options']['query'] = array('render' => 'overlay');
core/modules/system/tests/upgrade/drupal-7.language.database.php
191:  'location' => 'modules/overlay/overlay-parent.js',
223:  'location' => 'modules/overlay/overlay-child.js',
231:  'location' => 'modules/overlay/overlay-child.js',
387:  'value' => 'a:16:{i:0;s:14:"misc/drupal.js";i:1;s:14:"misc/jquery.js";i:2;s:19:"misc/jquery.once.js";s:10:"refresh:ca";s:7:"waiting";i:3;s:29:"misc/ui/jquery.ui.core.min.js";i:4;s:21:"misc/jquery.ba-bbq.js";i:5;s:33:"modules/overlay/overlay-parent.js";i:6;s:32:"modules/contextual/contextual.js";i:7;s:21:"misc/jquery.cookie.js";i:8;s:26:"modules/toolbar/toolbar.js";i:9;s:32:"modules/overlay/overlay-child.js";i:10;s:19:"misc/tableheader.js";i:11;s:17:"misc/tabledrag.js";i:12;s:12:"misc/form.js";i:13;s:16:"misc/collapse.js";s:10:"refresh:cv";s:7:"waiting";}',

Also I am seeing (via manual testing) that tour module is now throwing this error on every page.

Uncaught DrupalBehaviorError: attach ; tour: Cannot call method 'querystring' of undefined

nod_’s picture

Issue summary:View changes
Status:Needs work» Needs review
Parent issue:» #787896: Add a link so that administrators can return to their most recently visited non-admin page
StatusFileSize
new1.15 KB
new184.29 KB
PASSED: [[SimpleTest]]: [MySQL] 59,078 pass(es).
[ View ]

- Removed the example code in hook_batch_alter(), might need a new code sample though.
- Removed a @todo in a js file.
- I'm leaving the overlay references in the D7 file, since that's D7 anyway.

I can't reproduce the JS problem, your browser cache is clear?

nod_’s picture

Change notice to update: https://drupal.org/node/2116417

I tested the changes I made to the tour module with nick_schuch over IRC, everything still works. Can't reproduce #31.

ParisLiakos’s picture

I cant reproduce it either

+++ b/core/modules/tour/tests/tour_test/tour_test.info.yml
@@ -4,6 +4,6 @@ description: Tests module for tour module.
-hidden: TRUE
+#hidden: TRUE

lets restore that

Besides that looks good

ParisLiakos’s picture

Status:Needs review» Needs work

also we should probably remove this hook_alter implementation from toolbar:

<?php
/**
 * Implements hook_system_info_alter().
 *
 * Indicate that the 'page_top' region (in which the toolbar will be displayed)
 * is an overlay supplemental region that should be refreshed whenever its
 * content is updated.
 *
 * This information is provided for any module that might need to use it.
 */
function toolbar_system_info_alter(&$info, $file, $type) {
  if (
$type == 'theme') {
   
$info['overlay_supplemental_regions'][] = 'page_top';
  }
}
?>
nod_’s picture

StatusFileSize
new426 bytes
new183.87 KB

Thanks, fixed :)

nod_’s picture

Status:Needs work» Needs review
StatusFileSize
new184.07 KB
PASSED: [[SimpleTest]]: [MySQL] 59,056 pass(es).
[ View ]
new828 bytes

Yeah wasn't sure about that hook. Took it out, never seen anything using it.

ParisLiakos’s picture

Status:Needs review» Reviewed & tested by the community

Thanks!
Lets do it.

This feels so awesome..

nod_’s picture

Assigned:Unassigned» Dries

indeed :D

nod_’s picture

#787896: Add a link so that administrators can return to their most recently visited non-admin page Should be committed first though. Otherwise we'd get a critical regression from UX team :)

ParisLiakos’s picture

We can also commit this now and mark the issue you linked as critical, to make sure D8 wont ship before it gets committed ;)

Bojhan’s picture

Assigned:Dries» Unassigned

I don't think this needs Dries yet. And yes, the other patch should be committed first.

nod_’s picture

37: core-overlay-remove-2088121-37.patch queued for re-testing.

head should be fixed now right?

nod_’s picture

Status:Needs work» Reviewed & tested by the community

Green, back to RTBC

webchick’s picture

Assigned:Unassigned» Dries
Status:Reviewed & tested by the community» Postponed

Back to Dries, and I guess postponed?

webchick’s picture

Status:Postponed» Needs work

Sorry, due to #2140447: Open redirect in overlay (which was a critical security issue in D7, thus taking precedence over this patch) this will need a re-roll. :(

joelpittet’s picture

Status:Postponed» Needs review
StatusFileSize
new184.29 KB
PASSED: [[SimpleTest]]: [MySQL] 58,983 pass(es).
[ View ]

Very excited about this issue... re-rolled!

nod_’s picture

Status:Needs review» Reviewed & tested by the community
Bojhan’s picture

Status:Reviewed & tested by the community» Postponed
nod_’s picture

Status:Reviewed & tested by the community» Postponed

let's try again.

Bojhan’s picture

Apparently I do not have the correct rights to mark it "Postponed". :D

nod_ suggested that we might now have to postpone twice, for it to really work.

Dries’s picture

Assigned:Dries» Unassigned
Status:Postponed» Fixed

Committed to 8.x! :)

xjm’s picture

Title:Remove Overlay» [Change notice] Remove Overlay
Priority:Critical» Major
Status:Fixed» Active
Issue tags:+Needs change record

This issue needs to be added to https://drupal.org/node/2116417 but unfortunately we can't do that until #2135759: DATA LOSS: Change record issue reference field is single-value is resolved.

nod_’s picture

Title:[Change notice] Remove Overlay» Remove Overlay
Priority:Major» Critical
Status:Active» Fixed
Issue tags:-revisit before beta, -Needs change record

Updated change record.

nod_’s picture

YesCT’s picture

So, I was thinking overlay was moved to contrib. but I just want I verify that is *not* the case.

Overlay is not gong to be a contrib module.(?)

nod_’s picture

No, as it was, it's not possible to implement from contrib.

LewisNyman’s picture

I'd be interested in having the Overlay in contrib with the changes in #1889150: Simplify overlay look, *only use for contextual operations*

nod_’s picture

You mean using modals (with an iframe or not) to handle contextual links? why not. That patch only makes sense for 7.x now. D8 version of this should use our modals.

LewisNyman’s picture

The reason I like the Overlay is because it uses an iframe. Any admin UI in the ctools modal gets completely destroyed by the front-facing theme.

Status:Fixed» Closed (fixed)

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

dsnopek’s picture

I'm going to attempt to port Modalframe to Drupal 8 when I find the time. Maybe that could be the answer for a contrib-based Overlay-like thing?