As pointed out at #484820: Initial D7UX admin header, the added page_top region was used for the admin toolbar, but it is not part of block positioning, so putting it into an actual region does not have much of a point. Blocks cannot be put above and below it and the admin cannot use blocks administration to move the toolbar around. We either need to make the toolbar an actual block or get rid of the region and make the toolbar a hook_footer() implementation. @sun and @catch suggested the latter, so I've opened this issue for that. Patch coming up.
| Comment | File | Size | Author |
|---|---|---|---|
| #89 | page_top.patch | 4.97 KB | catch |
| #85 | page-top_2.patch | 7.79 KB | catch |
| #81 | page-top.diff | 4.46 KB | moshe weitzman |
| #77 | system-regions.patch | 3.29 KB | gábor hojtsy |
| #57 | page-top_1.diff | 5.15 KB | gábor hojtsy |
Comments
Comment #1
gábor hojtsyThis was a bit farther reaching then originally planned. The hook_footer() implementations were expected to return actual HTML strings, so adding in our renderable array was not nicely doable. Obviously I could have made toolbar module render the array in its hook_footer(), but better is to render it later. I Made it render in theme_closure(), but this might not be a big enough of a step. We should somehow add these to the page array, let modules alter it and then render it later I'd say. Will call in page rendering experts to look at this one.
Comment #3
gábor hojtsyBTW the reason hook_footer() implementations need to return keyed arrays is that module_invoke_all() merges the array, so we need to have a top level item in the arrays returned, so the subitems are not merged. This is the same like hook_permissions() or hook_block().
Comment #4
gábor hojtsyUpdated patch minus PHP error.
Comment #5
moshe weitzman commentedI would personally go for making the toolbar a block that is enabled and positioned in page-top during install profile. Seems most standard to me. OSX and Windows let you move the Dock/Start Menu after all.
Comment #6
alpritt commentedSince this will change the location of the outputted HTML in the document, two issues to consider:
1. How will this affect IE6 which falls back from using fixed positioning?
2. Won't this cause accessibility issues with screen readers, keyboard navigation etc? Visually the header will still be the first thing on the page. But as far as accessibility is concerned, this navigation will now be the last thing on the page which I don't think is right. (There are already similar accessibility issues with the toolbar, but I'll open a new issue for that since they won't affect this issue.)
--
@#5: That kind of flexibility would open a huge can of design worms with no obvious benefits. There is perhaps an argument for wanting to dock it on the left or right of the screen because of the real estate of widescreen monitors, but there is no way you could do this by simply turning it into a block. Also, this is a fixed element so the CSS will put the toolbar at the top of the screen no matter what region it appears in.
Comment #7
gábor hojtsyYes, as it is evident from the patch, there is no CSS change required to migrate from a region to hook_footer(), since the CSS already does that. At least in my testing with Firefox and Safari. Did not test with IE. @sun had a very strong position in the page_top patch, that an admin toolbar like the one we added should not use actual regions and should not be an actual block, because he found this to be impossible with admin_menu (mainly due to incompatible themes). That we'd require a theme to have a specific region to be compatible was not deemed a good answer there.
Either way, we are inbetween having a block and using hook_footer(), since we actually use a region, but not as a block. It is probably best to go to either end, since the current situation is pretty non-standard.
Comment #8
tic2000 commentedI find this issue quite ironic.
In the issue to add page_top to core the toolbar was the strongest argument. "We want the toolbar to be a block. We don't want to play with css, so we need a new region which is full width, no margins. etc"
Every patch for the toolbar mentioned that you need to apply the patch for the page_top region.
And now we end up with a toolbar that is not a block and which can be placed in almost any region, because the css rules will always place it at the top, full width.
It can be currently placed in the header region too with the only change being to add
display: blockfordiv#toolbar div.toolbar-menuso it overwrites display inline which header region has.Now that the rant is finished it may give an answer to alpritt's second concern. Place the toolbar in the header so it will be the first item on the page anyway.
Comment #9
alpritt commentedRe. incompatible themes, if #469242: Move <head> outside page.tpl.php goes in we could put $page_top in the proposed html.tpl.php which themes should be far less likely to override.
If we don't like using a region for this, then perhaps we should treat $page_top more like $closure.
Comment #10
gábor hojtsyAn admitted disadvantage of using hook_footer() (at least until it is made render-aware and does not render its output before returning) is that there is no way to alter the footer elements, so one needs to have his own suppression function for elements. Such as admin_menu, toolbar module would need a suppression function, so it can be removed from the overlay for example. Also, obviously, we'd remove the page_top region, if we don't use it. Updated patch attached to cover these.
Comment #11
gábor hojtsy@tic2000: Fully agreed that the new region did not fulfill its promise. Making it possible to actually move around the toolbar would require additional styling. Easy would be to tie it to the bottom of the page, if you move it to the "footer" region. That would make the code default to top positioning, but would switch to bottom if in the footer region. Putting it on the side would not really be possible, at least it would require design work to make it look sane in that case too. (In effect, we cannot rotate text 90 degrees in browsers, let alone how usable that would be).
So a blocks based implementation would only distinguish between two states: top and bottom. It would not require the page_top region either, since it would be "top" in all cases except when in a designated "bottom" region. We'd designate "footer" as the bottom region via code, so themes implementing that would let you move your toolbar there. Otherwise moving the block around would only change its position in markup. Now that it is fully described, I don't think this would be intuitive at all to the user.
Comment #12
damien tournoud commentedI'm with Moshe. There is very little advantage in not making this a block.
Re: @alpritt#6: I see no reason for the toolbar do always be on top of the screen. A theme could choose to support putting the toolbar as a sidebar or as a footer bar. Mandating that this toolbar be on top of the screen makes really little sense, and sounds very "un-Drupalish".
Let's make that a block and be done with it.
Comment #13
gábor hojtsyDamien: What do you think about people moving around the toolbar in the regions and nothing happening? If that your ideal user experience? What should happen if someone moves the toolbar into a sidebar or the help region, or...?
Comment #14
damien tournoud commentedThe following patch makes the toolbar a real block, that you can place everywhere you want on the page. Now that we have a proper page_top region, there is no reason to force the position of the toolbar to the top of the page. It also removes some unneeded and harmful CSS hackery. Drupal default theme (dubbed stark) should stay as simple as possible. Fixed positioning and IE6 fixes have nothing to do in Stark.
Comment #15
damien tournoud commentedComment #17
gábor hojtsyDamien: The reason the toolbar was force on top of the *viewport* was that it moves with the page. It has no relation with whether we have a page_top region. Your patch removes these features:
- The shadow below the toolbar. If you intend to remove this, then the image needs to be updated as well.
- The toolbar moving with the page. If you scroll down, the toolbar disappears.
As said above, I'd like to see how does the toolbar appear in different regions and whether we want that feature, so people can move it to the content region's bottom or somesuch. Moving the block around is clearly more possible with your patch, but except the margin and padding-less page_top region, it does not see to make sense to move the block to any other region available in Drupal core. Maybe if there'd be a page_bottom region, that would make some sense, but if it does not scroll with the page, people will not find out that it exists until they scroll down.
Comment #18
Bojhan commentedSo my opinion here would be is, go with the normal implementation. Turning it in to a block, and not allowing it to scroll down with the page. I know Leisa is away this week, so she won't be able to give feedback yet - so lets just move on and fix it later if they want it diffrent.
I am not really decided on the experience direction, so for now lets just continue.
It doesn't really make sense to move the block, but maybe some admin themes will want it in the bottom or whatever - so lets not fix it to the top.
Comment #19
gábor hojtsyReformulating what I said above: if we let admins to move the toolbar to any region, they might end up moving it into a 200px wide region which has overflow none and then not see most of the toolbar, since only the first 200px of the toolbar will show. From there, there is no way on the UI to fix the situation. Not sure we need to make it so simple for people to break their sites. If we make the toolbar a block, we need to somehow designate safe places for it to be moved to. I don't have a good idea how.
Comment #20
damien tournoud commented@Gabor: if I understand correctly you are trying to think ahead to a world where the admin interface has been completely refactored. We are *not there yet*, and there is no proof we will manage to get there before the code freeze.
We don't to think too much ahead and risk to end-up with an half-baked Drupal 7.
Comment #21
damien tournoud commentedI removed the user_access() wrapper by mistake. This one should pass.
Comment #22
gábor hojtsyThere is no thinking ahead required. To demonstrate what I am trying to say I've just applied your patch and moved the toolbar to the right sidebar in the default admin theme in Drupal core. Now how can I fix this? Hint: no way, except if I know the path for the admin/blocks page or the DB table to fix. Is this the good UX we promote?
Comment #23
damien tournoud commented@Gabor: I'm not sure what you mean. You moved the block to the right region on the block admin page, you can put it back to the page top region on the same block admin page. What kind of usability issue are you trying to solve by disallowing block movement?
Comment #24
gábor hojtsyDamien: It could very well be that it would be outside of the part of the page I am seeing (there are 4 more blocks above it in the same region), and I did not notice it is gone from the top. So I'd navigate away from the page, and never being able to fix it again. We can say this is an edge case, I am not sure about it, but seeing you intend to see users if they always know what the consequence of moving stuff around is.
Comment #25
damien tournoud commented@Gabor: the same has been true for years about the navigation menu block, and I don't see that "I disabled the navigation block and now I can't navigate any more" is one of our most frequent support requests (for the record: "I put my website in maintenance mode and now I can't login anymore" is still our #1 support request).
Comment #26
moshe weitzman commentedI think Gabor is saying that the user can't find the link to get back to the blocks page.
I propose that we add a property to blocks called 'locked'. That means that it can't be moved using the UI. A contrib module could come along and unlock it though. triggers/actions actually have this same notion. We restrict a trigger to only certain actions but contrib can easily undo this with a variable. This property should probably be enabled for the main content block
Comment #27
damien tournoud commentedBy the way, with the patch in #21, the toolbar doesn't show automatically on a clean install (except if you visit the block admin page). This is because of #512464: Auto-enable a block's 'status' with a non-empty 'region' when a module is installed.
Comment #28
tic2000 commented@Gabor, I only said the page_top was forced on us or as you put it, it didn't fulfilled it's promises. I said put in the header, so even if you use a screen-reader you would "see" it as first on the page. I didn't say anything about putting it to left or right side. I know what that change would require.
After the page_top region was forced on us, now I see the "block" approach being forced on us.
@all Even if the region doesn't uses overflow: hidden would still be ugly to see it in the left sidebar or right sidebar.
The argument that maybe a theme wants it in a different place has the same power in sustaining the toolbar as a block, as the toolbar had to sustain page_top region.
What do you need to move it? CSS.
But if you really want to make it a block, than either you make it so you can't place it inside left or right sidebar (like main content block already does), or write CSS rules for that case so the bar doesn't look strange placed there.
Comment #29
gábor hojtsy@Damien: I am not saying there are no other ways to screw up the site, I was pointing out this will be another one, and we might not want to go there. I think this is up for core committer decision, I've shown how this patch works in a sidebar, and said it will be broken in most any regions.
Also, seems like there was no feedback on the removal of the drop shadow. While it might seem eye candy, it does separate the header from the page visually, and it would be a pity to loose on the grounds of outdated browsers possibly needing some CSS hacks to make it work. That would be a step back to developer land from user experience IMHO.
Comment #30
manuel garcia commentedI'm not sure why anyone would want the admin toolbar in a sidebar, In my opinion it adds very little UX enhancement, but I do understand the added flexibility that handling it as a block gives you, so I won't push against it.
However, this does imply redisigning the toolbar for different sidebars, and at the very least we should provide with different CSS depending on which region the toolbar is in. At least for the default drupal provides. Other themes should handle this themselves I assume.
Comment #31
catchI'm not keen on the drop shadow - it obscures the top few pixels of the site, I might have very good reasons to want to see those.
For this to be a block which you can move, I'd want to see it themed so that it works vertically - it doesn't, so we should go with hook_footer() IMO.
Comment #32
tic2000 commentedIf you want this to be a block to give freedom to users and/or themers to move it where ever they want it at least go all the way and make different blocks for the 2 parts of the toolbar.
But I don't like the idea of this being a block either, but I don't like it being in the hook_footer if it affects screen readers (I don't know if it actually does though).
Comment #33
catchI'd think that having the content at the top of the page rather than a bunch of links isn't that bad for accessibility - would a #skip-to-nav help there?
Comment #34
webchickCould someone summarize what advantages we gain from this being a block? As far as I can tell:
- Allowing users to move it around means that themers need to provide the CSS to allow for a vertical nav bar display in their themes, which sounds like a colossal pain in the you-know-what.
- Failing to do so means users can lock themselves out of their admin interface accidentally (#22)
- We would end up doing double-duty on permissions: A block can have per-role permissions, and Toolbar module itself also exposes a permission. We could always remove the Toolbar permission, but I think since the toolbar doesn't "look" like a block, and you turn it on/off with a module, most people would think to look at admin/user/permissions before admin/build/block.
- There would be no way with a block to keep the bar at the top of the page at all times like now, which is something that's in the design write-up at http://www.d7ux.org/header/: "The header will remain at top of screen all the time (even when scrolling)"
alpritt's suggestion of a $page_top analog to $closure (and buried in html.tpl.php so it's not forgettable) is kind of interesting. I haven't had a chance to review that patch yet so don't take that as a "+1 from a core maintainer" yet, but of the proposed options it certainly makes the most sense.
But can someone explain the benefits (if any) of making this a true block? If it's only about maintaining some level of flexibility (in addition to the ability to shut it off and use something else or nothing at all), is there any particular reason that someone who was really, *really* adamant about moving that dang thing couldn't just use hook_page_alter() to do it?
Comment #35
gábor hojtsy@webchick: The admin header can be put away already by revoking permissions or disabling the module altogether, so there are already two ways to get rid of it, no need for hook_page_alter(). That said, hook_page_alter() is not yet capable of removing from the $footer, since it is pre-rendered. My latest patch above (#10) does some steps to make hook_footer() rendering aware, but is not enough yet to make it alterable (it still renders the footer just a bit later, but not late enough).
I agree that having a $footer counterpart on the top of the page could be a good idea. Also, we probably want to rename $footer anyway, so we can make up symmetric names for these two variables (the top variable would obviously not be $header ;).
Comment #36
senpai commentedIn answer to @webchick in #34, and speaking from a common sense theming perspective, I'm with Gabor on this issue in that I firmly believe this thing should not be a block, it should not be movable by administrators, and it should not show up in the admin/build/block list.
Think about this for a second. You take your car to a racing mechanic to try and make it faster, and they put it up on the lift for a minute, then tell you that you need four wheels. You say, "but I've already *got* four tires! I drove here didn't I? I just want the engine to run faster!" "No", they tell you, "Your wheels aren't here. Want some?"
I don't know about you people, but I always assumed that wheels were an integral part of what makes a car. After all, a car without tires isn't really an auto *mobile* now is it? :)
Why are we trying so hard to cling to the Drupal Blocks system here? Isn't this a Toolbar that allows us to control the very Blocks system itself? Is it not an integral part of the car, unable to be removed without a serious amount of effort? Imagine us proudly releasing a D7 system which can be utterly destroyed at the hands of virgin sysadmins who inadvertently disable their car by removing it's wheels in a single click instead of being 'forced' to get out an impact wrench and remove 16-20 lug nuts in order to disable their car because they really, really wanted to remove the wheels. Gawd, people, this is another one of those "Why does Drupal allow uid1 to be canceled just because it provides a way for user accounts to be canceled?" (BTW, that patch is almost RTBC. Woot) #46149: Prevent account cancellation for uid 1
Lest you think I'm all angry and stuff, I'm not upset, I'm just voicing an opinion here. :) And while I'm on a roll, can we please call this something descriptive like
<?php print $admin_toolbar; ?>rather than $page_top? It would certainly make a lot more sense to themers around the world since it's not truly a block or a region, but right now it's sure trying it's best to stand behind one at the party and pretend nobody notices. :)Comment #37
damien tournoud commentedMy point on this is that we are hardcoding something, and it is really not the direction Drupal is generally going. Yes, you can disable the module, yes, you can remove the permission, but we are basically forcefully inserting an "alien" element into the page, that the theme layer cannot really control. That sounds really weird.
On top of that, we now have (1) Mozilla and Safari custom CSS directives and (2) IE6 hacks... in a Drupal core *module*. That sounds really not nice either.
Comment #38
catch@Damien, yes it's hard coded, but unless someone steps up to make it work in all sizes and shapes of region, then putting it in hook_footer is better than shipping something broken simply because it's more standard. Also adding a $page_top region just for this is 'hard coding' since it'll be a mandatory region required for every single contrib theme, to support an optional core module.
For IE6 hacks, see #308865: Drop IE6 support in Drupal core which didn't get any support.
Comment #39
webchickOk, if it's not possible for Movable Admin Toolbar module in contrib to use hook_page_alter() and some CSS to allow this to be moved to the side of the viewport, then I agree that that's a bug that should be fixed. It's not clear to me how making the toolbar a block negates the requirement for IE/Safari hacks, but I agree that it'd be nice to remove those as well.
But, is there no way to do both of these without leveraging the block system? There's a pretty powerful conceptual mismatch between the admin toolbar and a "normal" block.
Comment #40
alpritt commentedre. @33 on content before navigation:
[...]
source: http://www.usability.com.au/resources/source-order.cfm
To be honest, that is not the strongest study ever conducted (too few participants), but here is another perspective.
The above is for screen reader users. Sighted users who rely on the use of a keyboard to navigate a site are going to have a different experience. They will see that the navigation is the first thing on the page, but when they try to navigate to it, they will find they skip right over it and enter the main content area of the site (or more likely the header). It is not until they reach the bottom of the page that they will find themselves jumping back up to the toolbar navigation. I can't see that being anything other than confusing.
----
Removing the fixed positioning would solve many implementation problems (most notably sticky table header conflicts), but at the same time it would be a shame to lose it from a design perspective. In any case, that's not the only part of the CSS that makes the block system not work here.
----
Re. the shadow, other CSS hacks, etc: while I agree there are implementation problems, these are really for another issue.
Comment #41
webchickGábor and I had a long discussion in #drupal about this.
This issue basically boils down to Block vs. Not Block for the Toolbar module. The current state is we added a page region for the toolbar to be a block, but the implementation doesn't use it. This issue isn't about drop-shadows or not, sticky or not, fixing the page rendering system, etc... It's about preserving the existing behaviour, but reconciling this logistical snafu.
While blocks on the surface sound good, because it allows flexibility for user to re-order them, move them to a different region, control what pages they are visible on, etc. they bring with them a host of problems. They imply a flexibility that is not actually there (see #22 for what happens when you try and use this flexibility), they break the existing "sticky" feature of the admin header, they provide duplicates of a bunch of functionality (per-role visibility, disabling, etc.), and in reality, there is fundamentally very little about the admin header that is "block like" at all.
The other option suggested was a hook_footer() approach. This means the behaviour is more "hard-coded" and also has an issue with source ordering for accessibility. The $closure variable is not renderable as the rest of the page is, and hook_page_alter() currently has no way to "inject" a new region into a page's output. Going this way means that we lose some flexibility for contrib modules modifying things, other than through CSS.
While in general I'm highly in favour of more flexibility, it seems undesirable to me that we take very first patch of the D7UX movement and try and shoe-horn it into Drupal's existing mechanisms, and end up creating a usability #fail as a result. Further, I think that Toolbar module has exposed a real use case in the page rendering system that is not accounted for (I'm a module who wants to expose my own region and stick some stuff in it), and that we should fix it, but it's not the job of the toolbar to fix the page rendering system. Finally, the current situation is we have to have every contributed and custom theme making a region for an *optional* core module. This is also not desirable.
The consensus we reached was:
- Add a 'sister' variable to $closure: $html_top and hook_html_top().
- Rename $closure to $html_bottom, and rename hook_footer() to hook_html_bottom().
- Implement the admin header as an implementation of hook_html_top(), which will preserve the source order.
If we fix the page rendering system, such as in the html.tpl.php patch, then we can look at refactoring this once more to use the hook_page_alter() method. But we have to use the tools that core gives us, and right now blocks seems clearly to be the *wrong* tool for the job, and that leaves us with top/bottom variables outside of page regions.
Comment #42
gábor hojtsyImplemented as webchick summarized.
Comment #43
gábor hojtsyActually the $main argument was never passed on to hook_footer() before (now hook_page_bottom), so if nobody missed it, why not get rid of it altogether? People can use drupal_is_front() with ease. No other changes.
Comment #44
gábor hojtsyTime to retitle.
Comment #45
gábor hojtsyUgm, and we should not use static directly, but should use drupal_static() instead.
Comment #46
tic2000 commentedNow I can say I like the approach and it's the first time since the toolbar was mentioned in the page_top issue.
Comment #47
webchickApart from the fact that lines like this:
ought to be lines like this:
...and that toolbar_suppress() could use some documentation about why someone would use this, and what the $suppression parameter is supposed to be, this looks pretty good to me.
However, I'm too "close" to this issue now so would appreciate some thorough reviews by others.
Comment #48
gábor hojtsy@webchick: The Garland template does not use those ; markers on line ending, so it would be out of place to use it on this one. If it does not follow standards, we'd need to fix it in another issue I guess.
Agreed on the docs, will follow up with docs later (not today). Other reviews still welcome.
Comment #49
Everett Zufelt commentedAs a user of multiple screen-readers I would agree that in the current paradigm of site design navigational and control functionality is * almost * always expected prior to content. The usefulness of providing a "skip navigation" or "skip to content" link aside, it would be disorienting at best for a new Drupal administrator, who is also a screen-reader user, to have to navigate to a toolbar at the bottom of a page.
I also agree with the comment for non-screen-reader keyboard only users. there is a burden placed on these users to have to chase the focus around the page when focusable elements are not consistently ordered with the page's visual display.
Comment #50
tic2000 commentedThe patch looks good to me and most important it works :).
To add one more nitpicking to @webcick's comment
+ * Implementation of hook_html_top()."Implementation of" should be "Implement".
Comment #51
moshe weitzman commentedIMO, this latest patch is kinda WTF. Lets keep this toolbar in our nice renderable array. That way its links can be changed dynamically and made context sensitive and all that. Now I am OK with the toolbar not being a block. Our current technique of
looks quite natural to me.
See http://api.drupal.org/api/function/hook_system_info_alter/7 for creating the region. And the current toolbar module for how to populate it. I really think thats better than the approach we have here so I set to CNW. If webchick/gabor disagree though, set it back to CNR or RTBC.
Comment #52
gábor hojtsyMoshe, using http://api.drupal.org/api/function/hook_system_info_alter/7 as you suggest does not make the region output printed out in page.tpl.php. When one introduces a region, a manual template file change is involved to get it actually output. That stops modules from adding dynamic content to wherever. So far the only place which was not a region and modules can add stuff was the $closure. The patch suggests adding a pair to that and renaming them for consistency.
We can also go to a different direction by adding a page_top *and page_bottom* region and migrating all hook_footer() code to hook_page_alters() on the page_bottom region. We are trying to come up with a consistent way to do stuff here, so let's think about the overall picture.
If we introduce actual regions for such stuff, that does indeed make the output available for altering but it also becomes confusing on the interface (why the hell is that region there, when there is no block inside and also how can I put blocks before and after these non-blocks in that region?).
So to be able to put stuff into the alterable page array, we need an actual region, which is output via page.tpl.php, but that also comes with a UI for these regions, which can be confusing for system-used regions, which would not be added for actual block placement. Looks like you favor convenience on the coders for a cleaner UI on the user. How can we resolve this issue so that it would not be confusing on the user either?
Comment #53
moshe weitzman commentedspoke to gabor about this. my latest suggestion is to add the ability for regions to be hidden from the block ui. this would be a small change to the .info file syntax for regions. we would then keep current mechanism for adding the toolbar output to the page-top region (i.e. not a block). this keeps our goal of putting data into the $page array and theming it later. i am not happy with data and theming happenning via the "backdoor" which is what preprocess these html_top and html_bottom variables are.
Comment #54
moshe weitzman commentedAs promised, here is a patch as per #53. Modules may now specify that certain regions are 'hidden'. That excludes the region from the blocks UI. Modules and themes are still free to populate the region during page building or altering. The page-top region is marked as hidden in the patch. This patch does not change how the toolbar gets added to the page (i.e. toolbar does not become a block; it continues to be populated in toolbar_page_alter).
If committed, I will update the theme upgrade guide as the syntax for declaring regions changes from
regions[header]=Headertoregions[header][description]=HeaderComment #55
gábor hojtsyInstead of strings, I'd use use REGIONS_VISIBLE and REGIONS_ALL constants. Also, to keep consistency with other parts of Drupal, a short name is a "title", not a "description", I'd say.
Otherwise this approach is fine with me and would support conversion of hook_footer() to now standard hook_page_alter() hooks with a page_bottom hidden region as the target. (Not to be part of this patch, since we had enough detours, but just to highlight the concept of why this would be useful). Just as a recap, the usefulness of this approach is that developers can alter the output of modules putting stuff into regions, while people putting stuff into "custom" variables like $footer cannot be altered on the same level.
Comment #56
moshe weitzman commentedNow using constants as goba suggested. Also changed description to 'label' as that matches field api terminology.
Comment #57
gábor hojtsyYou missed applying the constant in the first hunk of the patch. Updated.
Comment #58
gábor hojtsyApplied this to our Google core repository at http://code.google.com/p/d7ux, fixed the admin theme and reinstalled. After testing the functionality, it does hide the block regions on the admin page. I also testing .info file functionality, especially since the admin theme has its own regions defined in the .info file, so used this:
It did hide the region. If it was not set to hidden, it was not hidden. Looks like it is working as intended.
Comment #59
webchickEr, really? This seems like we just passed the WTF to themers:
as opposed to:
If anyone gets a WTF, I'd prefer it be developers. We can fend for ourselves much easier.
The fact that we need to add the ability for block regions (which by their nature are a visual container) to "hidden" in order to support this feature is a really good indication to me that regions are not the right tool for the job here.
Are you sure we really need to do this? Does the html.tpl.php issue not resolve the rendering issue for devs? Frando seemed pretty excited.
Comment #60
webchickComment #61
catchI also thought the html.tpl.php issue would allow for $footer and other variables to go via drupal_render() which currently can't.
Comment #62
gábor hojtsyUhm, that issue currently just moves around these variables, and does not make them available to altering or late rendering.
Comment #63
catchI think it allows for that though - and that change would be less intrusive than the region changes (we'd still need the $page_top variable here though of course).
Comment #64
senpai commentedI understand where Moshe is going with this, and I also share Webchick's concerns. How 'bout this instead?
Keep the declared regions in a theme's .info file as simple as possible for themers, e.g.,
And then require a function in the theme's template.php which would declare both any required regions and also lock those region, e.g.
That will keep the crazy config stuff out of the .info file so it remains simple and easy to grok. The two mandatory regions are still "controlled" by the theme at render time rather than being hidden away in core. It also gets us a pair of regions that are semi-independant of the 'main' regions people use for placing their custom blocks at admin/build/block.
Comment #65
moshe weitzman commentedWhat Gabor said - the html.tpl.php patch does not bring the toolbar into $page. It bypasses $page and builds the toolbar in the preprocess layer. toolbar is not a simple theme variable like sidebar-both. It is a major page element. If someone can elaborate on how that page allows the tolbar to be altered in hook_page_alter(), please set this issue back to CNW.
@webchick/senpai - you make it sound like the themers are learning nuclear physics with this patch. They just have to make regions[name] into an associative array. The've been doing this for a long time with the stylesheets variable. One could argue that we are doing each regions a favor by making it an array. Look at the cool stuff themers are inventing with .info file arrays (fixed this link. ~Senpai). Lets ask Jacine how many themers complain that skinr's .info file syntax is too complex. I'll acknowledge that the new syntax is a tiny bit less pretty but the advantages far outweigh the disadvantages. context sensitive, role sensitive toolbar, anyone? It is quite possible that toolbar functionality does not progress before code freeze .. .and senpai's suggestion to use a separate function is convoluted IMO. Every theme wants to hide page-top anyway.
I feel like I am playing whack a mole with various issues that are bypassing $page based rendering model.
Comment #66
tic2000 commented@Gabor
It makes them available for late rendering, but not for altering. For that you need a hook_footer_alter in current HEAD and maybe replace that with hook_html_top/bottom_alter in future.
@moshe
The html patch does not bypass anything. The only think would need to change is to change toolbar_preprocess_page to be a toolbar_preprocess_html which only handles the classes added to the body tag and not the toolbar build itself which remains in toolbar_page_alter and can be altered by other modules.
Try in current HEAD to do
in toolbar module and tell me if it works. It will work with the html.tpl.php patch. This is one of the benefits of that patch together with what the OP arguments. Making $closure alterable is not the goal of that patch.
But I see this issue being sidetracked a lot.
If you want the toolbar to be altered IMHO the normal process would be open an issue to add $html_top and change $closure to $html_bottom and provide a hook alter for those. After that gets in move the toolbar in $html_top.
Or you can leave it as it is (solution I don't like).
Comment #67
catchedit: massive crosspost, this was in reply to Senpai.
The problem with that is we'd want those regions hidden for every theme by default so they don't show up in blocks admin.
Comment #68
webchickRight, it's extra code that needs to be added to every single theme, and another "except for" thing to explain to every single themer and site admin: Themes define regions, and those are visible in the block administration page, and you can drag your blocks into them. Oh. Except for hidden ones... [enter long, convoluted explanation about how we added hidden regions to allow for the random possibility that maybe someday some module out there might want to alter the toolbar rather than just replace it with another module].
Seriously, I don't get why this is a preferable fix to Gábor's previous patch.
Comment #69
naheemsays commentedregions (and blocks etc) are also useless if the css forces the "block" into an absolute position.
Since the toolbar stays at the top of the page, it is not really a block in the traditional sense. (also, moving it to a different region would not stick it to the bottom of the browser window or the left/right side.)
As a tinkerer, I would plead that things be kept simple.
Comment #70
senpai commentedSo lemme get this straight.
1. We want a $page array that contains everything which will be rendered into the browser's viewport window.
2. This $page array will be editable and controllable by a custom theme, overridden theme function, or contrib module's hook_page_alter().
2. Inside this array, we want to let themers defile, I mean define, 'regions' which any privileged user can place blocks into using the admin/build/block form.
3. Then, we're going to make two extra regions in this $page array that themers can't use, privileged users can't place blocks into, and yet 600+ themes have to define these two regions and print these two regions or their Drupal site won't have an /admin section?
4. Once these two hidden, locked regions are supported by every theme in existence, core is going to place blocks into these regions that nobody can see, edit, change, or turn off?
Would somebody tell me this doesn't sound like whack-a-mole? :)
I want a user story. Give me a single use case as to how a user might possible leverage these two hidden, locked regions in the $page array at some point in the future. I'd like to know why they must be in the $page array and be locked there by all contrib themes.
Elaborate, please.
Comment #71
webchickContext-sensitive, role-sensitive toolbar is called "Better Toolbar module." You use it rather than the core Toolbar module.
I understand Moshe's ferocity around the whack-a-mole fight with preserving the page rendering feature, but this issue seems to have reached the outer limits of where the rendering system makes sense. My feeling is that in Drupal 6, 0% of the page is renderable, unless you're on a form or a user profile. In Drupal 7, 100% of the page is renderable, except for the admin toolbar, which can easily be disabled if you don't like it. It's still a massive win for developer flexibility. OTOH, abusing block regions for something that is in every respect not a block creates a weird duality for regions and some very specific disadvantages from an end-user, themer, and (arguably) developer perspectives.
Now it's possible we need to invent a new concept that is a "renderable region" which is programmable only and has no tie-in to themes... somehow... but that is not for this issue to fix. This issue is merely about fixing the fact that we have a block region we don't actually use for anything.
Comment #72
jacineI don't mind with having a $page_top, $page_bottom, and I don't think the .info stuff proposed is hard to grok - being able to control whether a region is disabled sounds great to me, but these are separate issues IMO.
I don't think the admin toolbar belongs in a block/region. I don't see any compelling reason for it to be a block, and there are lots of drawbacks, as noted throughout this issue.
It sounds like a job for preprocess_page() + theme settings (Display admin toolbar = 0/1), just like any $site_name, $site_slogan, etc... Unless the plan is to make all of these settings blocks, which I wouldn't agree with, this issue is a WTF.
Looking at the API for D7, I like how it's being done right now, except for the limited ability to do advanced theming at the menu level, which is a separate issue.
-1
Comment #73
tic2000 commented#519782: Change $closure to become a hidden region like page_top
@moshe I wait for your input there.
Comment #74
damien tournoud commentedregion-<region_name>.tpl.phpand default page.tpl.php to something like:print render($page);This approach is a no-go for me.
Comment #75
gábor hojtsyI had an idea that we can just add our stuff to the page array in $page['html_top'] and $page['html_bottom'] (replacement for $closure) and require themes to output a $html_top and $html_bottom. That is nothing more complex then requiring the $closure and would get us the needed required element on the page top to add our toolbar. However, adding stuff to $page['html_top'] does not make it actually rendered into $html_top in the theme, so one still needs to do render($page['html_top']), which however does not add our attached CSS and JS (obviously, since we are too late to add it to the page variables), so this would be a no-go. If we'd use two non-regions in the page array to output stuff and require them as variables, that would need to be rendered in a core module for the theme.
Food for thought.
Comment #76
damien tournoud commentedGabor: the CSS/JS rendering order is the main reason to create a html.tpl.php file. Before that, we could render those pseudo-region into template_preprocess_page().
Comment #77
gábor hojtsyDamien: ok, I also found out it would be easiest to render those pseudo (I called system) regions in template_preprocess_page, where all other regions are rendered. This can make system regions available to the $page array and can make the new mandatory $html_top variable available to the theme.
(This would include the migration of $closure / hook_footer() to html_bottom under the same umbrella and also some page.tpl.php docs and update docs, if the approach is approved. I'm trying to just show the idea here (but the patch works for me already)).
Comment #78
tic2000 commentedI think this solution will make everyone happy (at least most of us). It's not a region, it's not changed into a block and it can be changed in hook_page_alter. And on top of that the info will help in html.tpl.php patch.
Not that it really matters since the text is not displayed anywhere, but "Top of output" doesn't sounds so good to me.
Comment #79
gábor hojtsyYeah, we can just change Top of output to whatever, it is never used or displayed :)
Comment #80
tic2000 commentedWell, it's RTBC if you ask me.
Comment #81
moshe weitzman commentedI made a short blog post that hopefully will get everyone thinking the same way about our render system. Please read http://cyrve.com/d7render.
Agreed (except the phrase 'block region' is confusing. just say 'region'). Here is an approach suggested by catch. page-top remains a region like in HEAD. It is hidden from the block UI by hook_system_info_alter(). Thus, the themer's .info files do not change at all. We support an optional additional property called regions_hidden but thats not used by core. AFAICT, this approach addresses all DX concerns for themers and site builders (i.e. no .info file changes for themers and no odd region on block admin page).
Update: #77 is a pretty good patch too. I like this one a bit better.
Comment #82
naheemsays commentedThat does not seem to solve the main underlying problem that there is a block that is not a block.
(I personally like the idea of having a usable region at the top of a page that is the full browser width, but the problem in this issue is that the toolbar is not really a block.)
Gabor's approach in #77 looks like a better alternative to my untrained eyes.
Comment #84
gábor hojtsy@nbz: well, Moshe suggests that it is not a block and it does not need to be. And regions do not need to be limited to blocks (especially if they do not show up on the blocks admin UI).
Comment #85
catchFixed the notice and tweaked the comment on the info alter hook implementation.
The main advantages to this one are:
themers don't have to do anything except declare the region (using our current syntax) and add the variable to page.tpl.php. If they have a need for another 'hidden' region, that's easy to add via regions-hidden[] = region_name - but that's a new feature which is just a by-product of this approach and not required for anything.
Developers can use hook_page_alter() on page-top and we can do the same treatment here for $closure - and if we do, then that'll be a very easy thing to explain in the theme upgrade documentation - 'changed $closure from a magic variable to a region, added $page-top to mirror it, by default these are hidden from the blocks UI since they are reserved for modules to populate via hook_page_alter()'.
Comment #86
webchickGreat! This approach looks like it satisfies all parties. Let's see what testing bot says.
Comment #87
tic2000 commentedTesting bot is happy.
Shouldn't "Remove page-top..." start from a new line?
I'm happy (but that is easy to achieve).
Comment #88
kika commentedThis approach might influence the #503810: Convert primary and secondary menus to blocks implementation.
Comment #89
catchFixed the comment by making it inline within the function consistent with other hook implementations - in case we ever alter something else in there too.
Comment #90
tic2000 commentedCan I?
Comment #91
johnalbinsubscribing. Must sleep now.
Comment #92
dries commentedReviewed and committed to CVS HEAD.
Comment #93
sunWrap-up:
Given the current work on JavaScript'ifying Drupal's administration, there's not much to discuss about accessibility.
Wrong assumption. All browsers behave differently with regard to fixed or absolute positioned elements.
You forgot about Lightbox, Thickbox, and many other ways to render pages differently. A toolbar_suppress() function is still required.
If there was not an additional "shortcuts" bar that collapses/uncollapses below the common links.
Add this to the list of "Why pre-generated sprites are bad for Drupal core."
The Toolbar is like Comment module today. It is more. Insanely compressed into one bloated thing. It has to be considered as separate unit or it will horribly fail.
It not only obscures your site's design, but it is also not always attached properly, especially when the toolbar "should" (will) wrap properly.
One more reason to enable the "Management" menu block upon disabling the "Toolbar" module.
The Toolbar is an alien for the rest of a Drupal site. It's not system.module or any other module. As of now, it's not alien enough to make any sense of it yet.
IE6 hacks are required for aliens like this. (1) is not.
...and developers, because documentation about all of these changes is missing in the module upgrade docs.
Now, even after searching and reading for hours, I still did not find the issue that removed hook_footer().
One of the recently committed patches clearly needs module upgrade docs. If it's not this one, then I'm sure upgrade documentation is missing for this one as well. And I appreciate a pointer to the "other" (whichever) issue.
Comment #94
naheemsays commented@ sun - #519782: Change $closure to become a hidden region like page_top removed hook_footer.
Comment #95
gábor hojtsy@sun: not sure why you picked up some of the comments which were for now outdated patches. In terms of how a supression function is required or not, now that this is based on hook_page_alter(), stuff from the $page_top or $page_bottom can be altered out via that function. Documented changes of page_bottom and page_top on the update docs:
Still needs theming update docs which I'm working on.
Comment #96
gábor hojtsyAdded this to the theme update docs at http://drupal.org/update/theme/6/7#closure
Comment #97
gábor hojtsyOpened #534414: Fix missing docs and consistency issues with $page_top and $page_bottom as a follow up, since $page_top was never documented in page.tpl.php and its markup is not consistent with $page_bottom.
Comment #98
Everett Zufelt commented@#6
@#94
I'm not sure that I can agree that there's not much to discuss about accessibility. Not having followed this issue thoroughly I can say that having different display and source orders for focusable items is an accessibility issue for screenreader and keyboard-only users. I am not sure what the comment about "javascript'ifying" means, but most modern assistive technology can handle JS.
Comment #99
annmcmeekin commentedSubscribing.
Comment #100
gábor hojtsy@Everett: I'd think sun meant that there might be issues if the Javascriptification is not implemented with correct fallbacks. I did not see a case where there was no fallback for an accessible version where eg. drag and drop or hidden form elements or other JS magic stuff was implemented, and Drupal strives for being able to work with JS disabled, so I don't see this happening anytime soon either.
Comment #101
Everett Zufelt commented@Gábor Hojtsy
Most screen-reader users don't surf the web with javascript disabled, I'd wager that most, like most users in general, don't even know how to disable javascript. So, functionality like drag and drop (see #448292: Drag and Drop for table rows is not accessible to screen-reader users) needs to be accessible, even with javascript enabled.
Comment #103
David_Rothstein commentedDid this patch intentionally remove the translatability of region names??
Discussing that now at #226587: Default sidebar region labels are confusing (wrong) for RTL languages....
Comment #104
David_Rothstein commentedTo follow up, removing the translatability of region names was definitely a bug, and there is now a patch to fix it at #1050686: Theme region names are no longer translated.