Comments

David_Rothstein’s picture

Title: Overlay D1: $custom_theme is borked » Overlay uses $custom_theme global, but it doesn't exist anymore
Component: other » overlay.module
Issue tags: -overlay

Retitling.

David_Rothstein’s picture

This is very likely the cause of the following bug reported by @webchick as part of #610234: Overlay implementation:

if I select "delete" from the contextual links on a node, I'm given the confirm form in the overlay, but in my front-end theme.

Since node/*/delete is defined in hook_admin_paths(), it should be showing the admin theme in the overlay regardless of other settings in Drupal. However, it is not - other settings in Drupal which cause the main site theme to show on this page even when the admin theme is being used for content creation (actually that sounds like a bug in and of itself?) are taking precedence...

By the way, once we get this fixed, we should also remove this line from default.install:

  variable_set('node_admin_theme', '1');

As far as I know, that was only put there in order to make things look like some mockups... but since after this issue goes in the overlay will show the admin theme on content creation pages anyway, we don't need or want it anymore. The way that setting works, it is not something that would be commonly used by most community-minded Drupal sites, so it's not appropriate for the default install profile. (See also comments by me and @cwgordon7 somewhere in #517688: Initial D7UX admin overlay where at one point this setting was taken out as part of the overlay patch, for this reason.)

ksenzee’s picture

Status: Active » Needs review
StatusFileSize
new2.6 KB
David_Rothstein’s picture

The patch looks good to me. I'm not going to bother testing it out until the other one is committed though :)

This will probably break the "demonstrate block regions" page but we already have an issue to figure out what to do about that at #655416: "Demonstrate block regions" bugs with regions, navigation, overlay anyway.

+  if (user_access('access overlay') && isset($_GET['render']) && $_GET['render'] == 'overlay') {

Although technically not required to solve this issue, it seems like this might not be a bad time to introduce a function along the lines of overlay_will_be_displayed()?

casey’s picture

+  if (user_access('access overlay') && isset($_GET['render']) && $_GET['render'] == 'overlay') {

could be rewritten to:

+  if (overlay_get_mode() == 'child') {
David_Rothstein’s picture

It can't, because the overlay mode isn't set yet when this code is called. Would be hard to know that without studying the other patch in detail, though :)

It did occur to me that we could perhaps just set the overlay mode here rather than waiting until hook_init() where the same check is performed ... but conceptually, it seems like this is the wrong place to set it.

casey’s picture

oww wait this is about hook_custom_theme() not about hook_overlay_child_initialize(). never mind.

quicksketch’s picture

StatusFileSize
new5.68 KB

I was working on porting Webform to Drupal 7 when I ran into this problem. Basically right now there are two COMPLETELY different mechanisms for defining admin paths. Node module does one thing to set the admin theme (using "theme callback" in hook_menu()), while Overlays uses hook_admin_paths() to determine if an overlay should be used. This results in inconsistencies between when the overlay is used and when the admin theme is used.

Here's an expected user workflow:
- Set the Admin theme at "admin/appearance" to Seven
- Uncheck "Use the administration theme when editing or creating content"
- Now creating a new piece of content should NOT use the overlay right? However what happens is that the overlay is brought up when creating/editing content, but uses Garland inside the overlay.

This patch makes it so that system.module is responsible for turning on the admin theme, not overlay.module. node.module then bases it's list of admin paths based on the "node_admin_theme" variable.

quicksketch’s picture

StatusFileSize
new9.63 KB

Looks like system.module was also using this "theme callback" approach for the "admin" path. It's very strange that we were using completely different mechanisms for setting the "admin theme" and determining "admin paths". In order to keep the block-preview page working, I switched the order of precedence when both a "theme callback" is specified and a module is returning a theme through hook_custom_theme(). Now if a URL requires a particular theme, that is the final say. However the ONLY path that does this now is the blocks preview page.

David_Rothstein’s picture

@quicksketch, I think the issue you want for this is actually #669510: Merge administration theme with hook_admin_paths() ... hopefully still has a shot for D7, since the current behavior is indeed confusing.

quicksketch’s picture

Oh dear, so many admin-theme issues. Yes I think you're right David, mind if I merge this issue with that one? After fixing this problem generally, the only thing necessary for overlay.module is just to remove anything relating to $custom_theme.

Status: Needs review » Needs work

The last submitted patch, drupal_admin_theme.patch, failed testing.

quicksketch’s picture

Status: Needs work » Needs review
StatusFileSize
new13.84 KB

Fixes tests (I hope).

quicksketch’s picture

Status: Needs review » Needs work

Oops, I didn't mean to upload that patch here. I'm consolidating with #669510: Merge administration theme with hook_admin_paths().

casey’s picture

Status: Needs work » Needs review
StatusFileSize
new2 KB

At least we can remove the usage of $custom_theme as it doesn't so anything anyway.

casey’s picture

Anyone?

aspilicious’s picture

Looks fine to me, but I'm not an expert, so won't rtbc.

- I did apply the patch (just to be sure and didn't see any problems with the overlay) ==> OK
- I checked for trailing whitespaces, couldn't find any ==> OK

David_Rothstein’s picture

Status: Needs review » Reviewed & tested by the community

The patch is good, and at the very least directly addresses the issue title :)

It doesn't solve the issue that the existing code had been trying to solve, so we'd still need a follow-up along the lines of "Overlay does not show all its pages in the admin theme". But perhaps that can still be solved via #669510: Merge administration theme with hook_admin_paths() anyway.

dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD.

Status: Fixed » Closed (fixed)

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

David_Rothstein’s picture

Title: Overlay uses $custom_theme global, but it doesn't exist anymore » Some pages display in the overlay in a non-adminstrative theme
Status: Closed (fixed) » Active

I was going to file a followup issue for this, but we already have lots of patches above addressing it, so easier just to reopen.

Since #662868: user edit link should open in a overlay was committed, we now have a good visible example of this in core: If you go to a user account edit page, that page now displays in the overlay using Bartik (or whatever your main site theme is). Since the overlay is intended to be an administrative feature, it's supposed to show all its pages in the admin theme, if you are using one.

I'm reopening this issue to track it, and because we could (in theory) go back to trying an overlay-specific solution to this, but hopefully we'll just still be able to do #669510: Merge administration theme with hook_admin_paths() instead. (I keep meaning to find some time to get back to that one.)

tgeller’s picture

For one example of the problem, see #930524: Type style is wrong in tabs of View Revisions page, which David closed and redirected here.

Jeff Burnz’s picture

Agree with 18, is there a follow up issue posted, I have found multiple instances of themes not loading in the Overlay, including Garland and Stark under certain circumstances.

markabur’s picture

Subscribing; the overlay theme really needs to stay consistent from screen to screen.

Is #669510: Merge administration theme with hook_admin_paths() likely to make it into d7 at this point?

quicksketch’s picture

Status: Active » Closed (fixed)

From #18:

But perhaps that can still be solved via #669510: Merge administration theme with hook_admin_paths() anyway.

No point in having two issues open, especially since this particular issue was "fixed", the other patch addresses the remaining problems.

David_Rothstein’s picture

Priority: Normal » Major
Status: Closed (fixed) » Postponed

I think this should be "postponed" instead, since the other issue is broader and it's not 100% guaranteed it can make it into Drupal 7.

If it does get bumped to Drupal 8, we should still try for a more limited solution here, since this bug is pretty bad. (With the default Bartik theme it isn't so terrible since Bartik is designed to look good in the overlay, but with other themes, it can be extremely extremely ugly.)

That said, I hope we can still do #669510: Merge administration theme with hook_admin_paths(). I believe I will have some time (hopefully on Wednesday) to work on updating the patch there. Let's see what happens there first, and then reopen this one only if we have to.

David_Rothstein’s picture

Status: Postponed » Fixed

#669510: Merge administration theme with hook_admin_paths() was committed, so this is basically fixed.

The behavior now is that the site shows all users a consistent theme within the overlay, but only shows them the admin theme itself if they have the "View the administration theme" permission. There was some discussion in that issue of whether the overlay module should always force the admin theme when one is available, so perhaps this issue (or another one) should be reopened for that. However, it then means the overlay would be overriding/ignoring the "View the administration theme" permission, so we'd have to tread carefully there.

Personally I think it's OK as is, but if someone wants to reopen, feel free. The patch to start from for that is Gábor's at http://drupal.org/node/669510#comment-3309172

Status: Fixed » Closed (fixed)

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

fejn’s picture

Version: 7.x-dev » 7.12
Component: overlay.module » other
Category: bug » support
Priority: Major » Normal
Status: Closed (fixed) » Needs review
StatusFileSize
new59.71 KB
new60.05 KB

Hi,

This support request is similar to http://drupal.org/node/615138 (this issue), except that I am getting a region of my custom_theme blocking out a portion of the overlay; I'm currently using Seven as my default theme and my administration theme.

See the attached pictures for what it does to the displays: a) the first picture shows the overlay partially covered by a region of the custom_theme; b) shows the overlay presented totally in the admin theme (the way it should).

I've found somewhat of a workaround for this:
a) uses path http://theme.com/Disclaimer#overlay=admin/structure/block
b) uses path http://theme.com/admin/structure/block (essentially the same as above, w/o the "node#overlay=" in path)

I'm using Drupal 7.12; are the patches discussed above included in this version off core? Are they even applicable to this problem?
Is there any easier/better fix than this(it's kind of hard to tell users to do this, so they can enter data into fields on a data entry form)

Thanks for any comments you might have.

Jeff

David_Rothstein’s picture

Version: 7.12 » 7.x-dev
Component: other » overlay.module
Category: support » bug
Priority: Normal » Major
Status: Needs review » Closed (fixed)

Hi Jeff, yes, these patches were committed over one year ago and are in all recent versions of Drupal 7.

It doesn't look to me like your issue is related. This issue was about the entirely incorrect theme being shown in the overlay, not about portions of the underlying page obscuring the overlay. It's possible your issue could be caused by using a very high z-index in the custom theme (higher than the z-index used by the Overlay module's CSS).

fejn’s picture

Thanks, David - I'd have never thought of that one. I found that the z-index for that area was set to 2105; I reset it to -1 and now I can at least dependably get to the admin pages w/o typing in the admin/.... path to force it to come up using seven. This may solve most of my problems. Do you know what z-index the overlays use, or where to look to find out what they want? Now my problem is that the main menu (which had z=2105) is inaccessible, an I can't click on the items, or have anything happen when I hover over them.

If I can get a relatively easy fix to that issue, this issue can probably be closed.

Jeff

David_Rothstein’s picture

Do you know what z-index the overlays use, or where to look to find out what they want?

I believe it's around 500; check modules/overlay/overlay-parent.css (or use a tool like Firebug) for the details. If you use a large z-index (but smaller than 500), you'll hopefully be OK.

fejn’s picture

It's 500 in modules/overlay/overlay-parent.css .

Thanks for your help.