Per-user setting for the Overlay

let's try not to special-case UID1

2 clearly overlaps 1, so it's downright silly to do this separately

1, 2- Add an option to user/ID/edit to disable/enable overlay?
3- Add a to-fullscreen button (like windows of an OS mostly have) next to (or somewhere else) the close button which opens the overlay in the parent window? What do the designers and usability-experts think on this one?

It would be nice to know how other large projects handled a radical transition like overlay. Does anyone have any data?

Yahoo mail, for example -- not very glamorous, but millions of users -- provided and still provides the user the ability to switch from the new, JavaScript-based interface to the Classic interface. It's right on the main pages, and not three levels deep in options.

A wise example to follow?

Sub.

This patch adds a setting at admin/config/system/overlay to allow users to disable overlays per account.

When this is enabled (it is by default), users with permission to access overlays can disable them on their account edit page.

Adding an easier button to disable overlays right from the overlay sounds like a decent idea to extend this basic functionality.

This is a lot nicer than having to go X levels deep and disable the whole module for the entire site. Thanks.

However, I agree with #7 that "disable overlays right from the overlay" to restore the Classic experience is a necessary addition for those who (as I do) find the overlay, shall we say, a less than ideal user experience. (And I speak as the user I am, as well as the content creator and developer that I am). And such a solution would, I think, be closer to the requirement expressed by the issue. (I agree that there's no reason to special case user 1, but the "Classic" solution at least allows a one-click solution there, which is both a better UX and a better DX solution).

Makes sense. Do we have to add a fieldset for one pref?

@moshe #9, if to me on "Classic"--

Re fieldset: The snow of my ignorance remains untrodden on that one...

* * *

I think we should be seeking a UX solution that's radically unobtrusive. So while I can be brought to accept an overlay -- dashboard I already love -- I'm not sure I can be brought to accept this overlay.

Here's a patch that adds the disable checkbox to the user account without a new fieldset.

+1

I like the ability to disable the overlay for users. But, I think that variable_get('overlay_user_config', 1) should default to 0 instead of 1. We want to simplify the user interface and options. If an admin or installation profile wants to make this configurable it should be an opt-in setting.

I do like the per user approach for this vs per install. After all It's very likely that many sites may want to enable it for the usability enhancements that it brings, but that they do so at the exclusion of individuals who are hit with either accessibility or performance issues that might arise for the individual.

Reroll

Changes:
- Default variable 'overlay_user_config' to 0 (see #13)
- Moved path "admin/config/system/overlay" to "admin/config/user-interface/overlay".
- Minor string update.

#15: overlayuserconfig.patch queued for re-testing.

Personally, I think we should implement the knob that was in the original design, rather than a user setting. I can't find the issue but there was a dedicated issue for that.

I am not aware of such an issue. Anyone?

+function overlay_settings_form($form, &$form_state) {
+  // Overlay settings.
+  $form['overlay_user_config'] = array(
+    '#type' => 'checkbox',
+    '#title' => t('Allow users to disable the overlay for their account.'),
+    '#default_value' => variable_get('overlay_user_config', 0),
+  );
+  return system_settings_form($form);
+}

Why is this a variable? If we need it at all, why not just make it a permission - rather than adding a whole settings form for it? That's simpler and a lot more flexible.

***

While it's definitely a bit annoying that there's no way to turn the overlay off for user 1, that is also true of so many other things in Drupal... if you actually manage your site with user 1, I think you just get used to that :) And per-user settings don't seem to have a great history, do they - who uses per-user block visibility, and who ever used per-user themes? So it seems like potential overkill to add this in the main overlay module, but I guess I can sort of see a reason for it also.

I have no idea what the "knob" refers to either. Please enlighten us :)

Why is this a variable? If we need it at all, why not just make it a permission - rather than adding a whole settings form for it? That's simpler and a lot more flexible.

Overlay already uses a permission. If that's the way to go, this issue is fxed.

Agreed, there is nothing to fix here.

No, my suggestion was to turn the 'overlay_user_config' checkbox + settings page added by the patch into a permission of its own - 'configure own overlay access' or something along those lines, since that would avoid adding an entire new page to the admin area. (Or, just to get rid of that setting entirely and automatically let anyone with 'access overlay' configure it from their user account page.)

I'm not totally opposed to marking this "won't fix" since in general the way Drupal works is that user #1 has access to things whether they want to or not. Plus it should be pretty easy (especially with #743908: Skip overlay initialization if already done) for someone to write a contrib module that makes it per-user. However, if we're going to mark it "won't fix", let's make sure it's for the right reasons :)

I think overlay annoys enough people that they should have the ability to turn it off individually. I haven't had a chance to review the patch itself but I strongly support having a user-level flag.

Over at 775084: Allow users to opt-out of the overlay during installation for accessibility, we've been spun around the maypole several times. Currently, we're being told that the issue calls for a per-user solution, which sounds a lot like this issue.

Just thought you folks should be aware that Drupal 7 cannot conform to international accessibility guidelines or United States law if Overlays are on by default. The overlay interface is inaccessible to people who use screen readers. You are actually on the right track from the standpoint of accessibility: Let each user decide whether this interface works for them. The only thing is that we need to start with Overlays off, not force people to turn it off.

Oh, by the way, if this issue and 775084 are consolidated, then because of the accessibility implications this issue needs to be changed from "feature request" to "bug" and from a status of "normal" to "critical."

Assuming this issue is about managing the overlay after install and #775084: Allow users to opt-out of the overlay during installation for accessibility is about managing it during install, here's another thought:

1. Create a menu callback in overlay module that toggles it on or off for a single user (which assumes a patch similar to #15).
2. Include that link as a default in the shortcut module, which is enabled in the default install profile.

From an accessibility point of view, perhaps overlay should be turned off by default, and opt-in during installation and configurable from there on... personally, I'm annoyed at the overlay and wish I could turn it off...

EDIT: Its fine that its in the modules section, but there should still be an option during installation. Also, perhaps instead of adding a new setting, just have a link to the modules/overlay area in the description of the "Account Settings" section, maybe under "PERSONALIZATION" there could be a small link stating, "If you wish to disable the administration overlay interface, please click here and disable the overlay module..."

Two patches, only one of which should be used. One leaves overlay enabled by default when it's installed, the other disables overlay by default when it's installed.

There's no reason to have a menu callback and an overlay.admin.inc file for a single administrative page with a single checkbox.
The setting is actually a permission anyway and should be handled as such.

Otherwise, this is a reroll @ HEAD of #15.

Both patches:

• Address points 1 and 2 in Alex's comment #2.
• Add a checkbox to the user profile form, "Use overlay", that enables or disables the use of overlay site-wide.
• Define a permission "disable overlay" (or "enable overlay" for DEFAULT_OFF) that controls the visibility of a checkbox field on the user profile form.
• Disable or enable the use of overlay in overlay_init based on the user's setting.

The DEFAULT_OFF patch is included mostly as a proof of concept because it seems clear at this point that Overlay is here to stay and is to be enabled by default in spite of the bevy of legitimate issues against it.

Regardless of whether Overlay is included in the standard install, not allowing the overlay feature to be disabled is a usability / accessibility bug in the Overlay module that will render a Drupal install unusable for a subset of users; hence this is "priority-major".

I do think that 659480-user-overlay-setting-ON_BY_DEFAULT.patch will work for our needs. I would like to see the 'Disable the administrative overlay' permission enabled by default. I think this way it allows users to decide (and after all how often is an admin going to know or care go offer users that ability).

However, even as it is a blind user should be able to create a site, disable the overlay as user/1 and then change the permissions for other users.

Now do we know if overlay still needs to be enabled to upgrade between versions.

Think we're very close here!

Honestly believe that adding this checkbox in user profiles will add the same type of confusion that user selectable themes brought. Adding confusion to a large majority of new users who dont know what an overlay is. I still dont get the allow an ability to easily disable overlays - is there a true usecase for disabling overlay for only a couple users? After all there is a easy way to disable overlays - the checkbox on the module page.

That said, I see no objection against adding a permission. But perhaps I am missing something.

This is not critical, nor major. All accesiblity issues should be handeld in #716612: Overlay is not accessible to screen reader users and not sidetracked by this feature request. I am really sorry @mgifford but I dont like any issues that sidetrack the actual problem by creating a setting, permission or anything. Decisions should be made in that issue, not in one like this.

I agree with Bojhan; I don't see any rationale for why this is a bug or how it would improve either usability or accessibility. And the OFF_BY_DEFAULT patch seems a little silly - I think we can reasonably discuss removing the module altogether in #659488: Properly test the overlay to determine if it belongs in core or contrib, but having the module be there but do nothing in particular when you first turn it on doesn't seem to make a lot of sense :)

That said, if we're going to do this, the ON_BY_DEFAULT patch looks like it's on the right track. The permission is definitely an improvement over a setting, but I still don't really understand why we need either of them. Why not just automatically show this checkbox on the user account page of anyone with 'access overlay' permission? That's already likely to be a small, limited group, so adding something else on top of it which allows you to forbid certain users from being able to turn off the overlay while allowing other users the choice to turn off the overlay really doesn't seem useful to me. It just adds complication, and also seems to have some bugs, e.g.:

1. +  $use_overlay = user_access('disable overlay') && (!isset($user->data['overlay']) || $user->data['overlay']);
   +  if (empty($mode) && user_access('access overlay') && $use_overlay) {
   

   Either I'm reading this wrong or that's a bug; it seems to make it so that you always need both 'access overlay' and 'disable overlay' permissions in order to see the overlay?

2. +function overlay_form_user_profile_form_alter(&$form, &$form_state) {
   +  if ($form['#user_category'] == 'account') {
   +    $account = $form['#user'];
   +    if (user_access('access overlay', $account) && user_access('disable overlay')) {
   

   I'm not totally sure about that second access check being against the current user always. So an administrator editing someone else's account can't configure this setting unless the administrator has 'disable overlay' permission themselves (e.g., even if they have 'administer users' permission)? Hard to say what the right behavior would be here.

In short, I think this patch might be simpler and better if it just didn't bother introducing the 'disable overlay' permission at all.

Another small issue:

3. +        '#title' => t('Use overlay'),
   +        '#default_value' => isset($account->data['overlay']) ? $account->data['overlay'] : 1,
   +        '#description' => t('Open content creation and administrative pages in an overlay.'),
   

   Is the title really necessary here? - in other words, could it just be removed and the #description moved to the title instead?

   Also, the wording of the description seems a little long. The 'access overlay' permission description just says "View administrative pages in the overlay" and it probably makes sense to make those consistent. Maybe both should just say "View administrative pages in an overlay above the website", or something like that.

Bojhan: thanks for pointing out the other issue.

Re #32: I agree these points with one exception: the string "View administrative pages in an overlay above the website". For some reason I'm hung up on the word "above". I don't think it adds anything over the existing description, ie. it doesn't help me understand what an "overlay" is in the first place.

So, attached patch simply adds a checkbox on the user profile form that enables/disables the overlay for users with 'access overlay' permissions. Overlay remains enabled by default when the module is enabled.

@Bojhan - I'm not confident that we're going to see a resolution for [##716612] - I think that it's entirely likely that Overlay will need to be entirely rewritten if it is going to work for popular AT. If we were making good solid progress on this issue then sure, but other than dropping Overlay down to a secondary core module (like OpenID) I think we have to look at other options. I don't think this is going to be as easy to find a fix for as it was for Drag/Drop (and that wasn't easy as you know). I'd be interested in hearing perspectives on this from others too. Perhaps we're closer to a solution than I think.

A use cases for disabling overlay for just a couple users. So, we're implementing Drupal in my organization for virtual project management. Most people in the organization have never used Drupal before, have no disabilities & would think that Overlay is just great! However after it's implemented you start to hear concerns from people. Jan in research uses a screen reader and wasn't considered when this first rolled out (because people assumed it would be accessible). Bill connects in remotely 2 days of the week but is on dial-up and is using an older computer at home some he is finding it difficult. Plus the ED just hired Betty to be his new ED and although she has lots of Drupal experience it's all with Drupal 6 & she just wants it to work like it used to.

So, the option that most admins would take (rather than looking for or writing a module to do this) the admin just turns off Overlay for everyone. Now some people are grumpy because they can't take advantage of this flashy new feature they've gotten used to.

That's the best I can do for a use case.

@David, I'm going to leave @aaronbauman to address the code changes. It worked in my tests mind you.

Following up on #35:

However after it's implemented you start to hear concerns from people

We should be more worried about the concerns we don't hear. For every one person who takes the time to learn how to file an issue, 10 people will just give up and switch to another system.

Further, it's been said by others, but worth repeating: Drupal has enough users that there are no edge cases. According to Dries, Drupal powers 1% of the Internet. If we say that this issue effects only 1% of Drupal users, we're talking about as muas as .01% of the entire Internet!

It would be a shame to do nothing because there's no resolution on the other usability issues, or because this "feature request" was submitted after feature freeze. Exceptions should be made for Overlay because of its exceptional status as one of the first ever straight-to-core modules.

@mgifford I get that we might not find a solution, still doesn't mean we should be sliding in a solution here - then this needs to be postponed on the decision made in the main acceisiblity issue.

@aaronbaunman I agree that there are no real edge cases, but there are loads of things that should be in core but arn't because of other arguments. The one being here is that; 1. Users will not understand a "disable overlay" setting, for them there is no concept of overlay. 2. We never added a setting for an interface-element before, because users potettionaly might not like it - why is it neccairy here? 3. This is an use case that could more beautifully live in contrib-land, I could see per role as a usecase with this to.

Anyway looking past the accesiblity part, I still dont see why we would need this kind of setting. But I will leave it for others to decide now.

Not that this thread needs another comment, but I don't see this as a useful per-user setting. I think that's a bit overkill for core isn't it? That would be akin (in my view) to having a per-user "should fieldsets be collapsible" setting... I just don't think it's used all that much.

Now, there is no problem with creating a contrib module to address this. There is already a way to shut it off site wide, a contrib module could easily add a per user setting. There are edge cases, and this is one of them IMO. Sure, Drupal powers a lot of websites, but that doesn't mean that some features aren't more central to the lives of more users than others right?

I don't see people abandoning Drupal because users who are "semi-admins" are seeing an overlay and the admin wants to use it but give the "semi-admins" the opportunity to selectively disable it... sorry, just don't see it.

For the record, I don't like it all, but that's not the point.

Re #37
Couldn't let this go without comment:

We never added a setting for an interface-element before

This patch closely reflects the way Drupal core's Block module allows block admins to manage user customizable settings, which creates a fieldset with all user-configurable blocks on the user profile.

#716612: Overlay is not accessible to screen reader users has started to discuss solutions for the problem of this issue. So moving over commenting here.
As to webchicks latest idea http://drupal.org/node/716612#comment-3137782

This may be a start.
The trick will be now to find a middle ground between too much in-the-face and to cryptic and inconspicious. What you propose looks not bad, but would be very far on the can-be-overlooked side for me. This needs at least a popup explanation what this is about - which means it can be done more intuitive ;)

As to Mgifford's use case and the general direction of this discussion: wouldn't be a per-role setting work perfectly fine?
In a multi-user environment this should be taken care of by the admin.

Just as Bojhan I can perfectly see a little contrib module providing the per-user setting if a per-role one is not fine-grained enough. Having the checkbox aaronbaum provides on the user page should not lead to checkbox overkill, either. Have not applied the patch, a screenshot is welcome :)

Depends on where you put the checkbox. At the very top or further down. As the top and bottom are the most memorable places.

I'm opposed to the per-role setting as roles are set by the admin not chosen by the user.

If I have to wait for IT to disable something I could possibly be waiting for months (ok, not in my case, but in many organizations).

It's the user being able to choose, much like they are usually allowed to do with the choice of enabling or disabling the rich text editors. They might be comfortable with it, but on the other hand why should the administrator need to control this.

This is about allowing the user to select the best choice for them.

Here's a Screenshot what it looks like.

This is definitely not intrusive or cluttering, but rather the other way round. This needs its own label and a small help text (aka Description). Might be seen as overkill, but a little help text in the real help region with two small screenshots showing the overlay on and off would make sense.

As much as I am opposed to redundant descriptions below everything, in this case it makes sense IMHO.

This looks good to me. Thanks for posting the screen shot!

1. We definitely need a per user setting as part of core, this must be easy to find when disabling and enabling overlay.

2. It is most helpful to me when screen shots come with a description, so that I can better understand what is being discussed and considered.

And here it is like it should look like IMHO. Help page yet to be created, wording may be improved.

@Everett the Screenshot shows the user account edit page with patch of #34 applied.

The patch from #34 adds a checkbox to the user profile form with the value "view adminisrative pages in the overlay". The checkbox appears below the existing "Roles" checkboxes at the bottom of the "Account information" fieldset.

The screenshot in #46 suggests updating the patch from #34 by:

• adding a #title of "Overlay" to the checkbox
• adding a #description of "The overlay shows administrative pages with a transparent black background over the page you started from"
• and adding a "? Help" link floated to the right of the checkbox.

I'd really like to see the patch go into core as it looks in #46.

However, there's resistance from the usability folks on this one.

I'm wondering if this checkbox (and associated text) could be hidden with an element-invisible class so that this is visible only for screen readers?

If it's themable then this could be made visible by default in some themes (or possibly removed in others). However, I think this would not increase confusion for sighted users while allowing this to be very easy for blind users to disable.

tagging

I'm wondering if this checkbox (and associated text) could be hidden with an element-invisible class so that this is visible only for screen readers?

That would only fix half of the problem. There are a lot of non-blind users that want this feature as well. It's not just for accessibility, but rather for preference and minimizing annoyance.

-1 for hiding the checkbox.

A per user option seems ok to me. Besides AT users and overlay-haters there are others who could benefit from such an option like smartphones and other small-screen devices.

I'm just trying to walk through ways to resolve this. I'm not sure that we're going to reach consensus on the patch in comment #34. What are some other options that allow us to move forward & introduce more options without adding undue complexity to the user interface.

Having the form visible to screen readers & hidden to others helps advance some of this. If it's hidden in the theme layer using CSS (Garland/Seven/Bartik) then it doesn't present any additional UI problems for core. Themes & modules can just remove the CSS if they want all users to be able to actually see the form.

The addition of the option is added in core overlay_form_user_profile_form_alter() function in core as well s logic to disable it:

  $use_overlay = !isset($user->data['overlay']) || $user->data['overlay'];
  if (empty($mode) && user_access('access overlay') && $use_overlay) {

I'd like to see this visible too, but I'm looking for a compromise that will meet the needs of our usability experts. I do also understand that this is a slippery slope and there are all kinds of other features that others might want to add to any of the forms that Drupal gives to the users. Many folks see Drupal as just being too complex and this is a concern for our whole community.

After so much Discussion a little checkbox should be o.k. in user profile.
While I can see UX people fight every superflous checkbox. But User Profile is not install page.

A lot of people expressed the need to being able to easily switch of the overlay (hence the issue title).

What was and is a main concern for the pro-Overlayians (like me) is not to make it _too_ easy to switch it off. It is kind of a new feature, and maybe you need to use it at least for ten minutes until you love it. So having the checkbox in the user profile should be definitely O.K.

I rather see discussion arising if this place is obvious enough and if people will find it.
In Drupal 6 login takes you to your user profile by default. Don't know if this is the same in D7. But if it was, we could add the checkbox or a link to it on the "view" tab of the profile if there is a need to make it more obvious.

Adding it here (and to other users editing pages too) should be fine user/1/edit

I think sticking a checkbox on the view tab would be wrong though. It's not where people expect to see checkboxes and it would be confusing.

Now why the views page doesn't display to users their Email address, Roles, Contact Settings & Locale info I don't know. I'm always annoyed that the email address is only visible if you hit the edit tab. That's not what I expect when I want to view a users email address.. However that should likely be another issue (if it isn't already).. Sorry for the digression.

So @elgentor, from your discussion are you able to mark this RTBC?

@mgifford: someone has to reroll the patch with the proposed changes in #46. I gonna test it and if it works and looks as expected, set it to RTBC.

I'm going to upload a few slight variations as I don't think we're going to be able to get to your screenshot for a few reasons (now I've dug into the code).

First issue is that the help link might be something to consider for D8, but I don't see it used in D7. I've looked pretty thoroughly around and am missing the little blue question marks. If there's a pattern, particularly in the Forms API to do this, please let me know. Also, I do think that getting a help page added isn't trivial. Again, the help system will be revised in D8, but I don't think it works like you've outlined. It's more module based than function at this point I believe.

This just adds the description sentence to below the checkbox..

I think this is the most consistent approach to the existing forms & this page in question.

The Overlay is embedded in a collapsible fieldset (as are Contact settings & Locale settings).

It is distinct from the account information as per the image in #46 (and frankly it isn't account information so doesn't belong in that set)

There is no help link, but this too is consistent.

This is my preferred option.

I agree #58 looks best. I wonder a bit about the fieldset title, though - if we are saying no one is really going to know what "overlay" means then highlighting only that word won't mean much. Maybe "administrative overlay" or something more generic like "administration settings" or whatever. Either way, if we're going to go ahead with this at all I think this is probably the best we're going to get.

Trying out the patch, it doesn't work though. The data doesn't actually save. Contact module saves its settings via implementing hook_user_presave() to get it into $user->data; maybe we either need that, or it looks like the earlier patch in this issue accomplished it by having the form_alter code put it in the exact right place in the form array, with 'data' as one of the keys, etc.

+      return $form;

This shouldn't be necessary; hook_form_alter() doesn't have a return value.

@David_Rothstein - Yikes, not only do you want it to look right, but you also want it to work!

Seriously though, thanks for pointing that out. Can't believe I missed that phase of testing. I rebundled the patch with the hook_user_presave() function added as you suggested (that was great too) saved a bunch of time to look at the Contact module and how they approached this.

I nixed the return $form; - thanks again.

I've got a new patch and screenshots (with the value saved as Overlay disabled), changed title 'Administrative overlay' and also a screenshot if you don't have access to Overlay (in which case you won't see this).

Sounds like we're very close to RTBC with this.

Screenshots are with the Bartik theme.

-1 here, especially: (1) not without a way to disable this intrusive option, (2) clearly not on its own fieldset

We already have a permission 'access overlay', so I guess this issue is nothing more then a feature-request for the per-user setting. Retitling as such.

@Damien Tournoud - I need more info about #1 & #2.

The field can always be hidden in CSS in any theme. Also not sure why this is clearly not it's own fieldset...

#60: 659480-user-overlay-disable_3.patch queued for re-testing.

I just sent this to retesting incase it's the bot.. However I suspect that hook_user_presave() may be messing up OpenID

Apparently John's the problem (isn't it always the case though).
$user = user_load_by_name('john');

296     $identity = url('openid-test/yadis/xrds', array('absolute' => TRUE));
297     $this->submitLoginForm($identity);

330     $identity = url('openid-test/yadis/xrds', array('absolute' => TRUE));
331     $this->submitLoginForm($identity);

364     $this->drupalPost(NULL, $edit, t('Create new account'));

367     $user = user_load_by_name('john');

399     $this->drupalPost(NULL, $edit, t('Create new account'));

Any idea

I've modified the OpenID test to address that we've added content to $user->data and so therefore it won't be 0.

Hopefully this gets by the bots & thanks @Heine for the help on IRC.

Hm, I wonder if that failing test actually indicates a problem, though, or at least a weakness. Perhaps we shouldn't be saving the default overlay setting in every single user's account that gets saved, given that we don't expect most users to have access to the overlay anyway. It is kind of a waste to store that for everyone.

I don't have time to look into this now, but will try to look more carefully later.

Well, the approach we took for Drag/drop was to toss this information in a cookie. I think that was a reasonable compromise for Drag/Drop, but not sure it would work for Overlay. @chx mentioned on IRC that he'd want just a link to disable overlay that showed up somewhere. I'm not sure how that would fit into the UI though.

Still, it's a change that should be considered. Who should review what gets stored by default in $user->data??

Please don't put stuff in users.data. That should have died in D7 and will definately die in D8 #347988: Move $user->data into own table. Adding data just increases our work later. Use an overlay_users table.

I don't believe that the current approach makes the setting easily discoverable and understandable. Remember, this is not a preference, it is a requirement for accessibility (although I think I have long since lost that argument).

That being said, this is a much better approach than having no option at all, and I can't really think of a better location for the seting than on the user page.

So let's add an "Overlay off/on" to the profile display. This should be only visible for the user himself and admin. And to make this even more findable we can think about where to reference to it. Does the overlay have contextual help? Because in the help text quite at the top we could write "Don't like it? Switch it off here." Link this to the user's profile edit page, including an anchor that scrolls to the setting.

@moshe weitzman - thanks! Too bad about user->data in that it looks like we'll need to find another solution.

I don't know if adding a whole new database table is a good idea though. Seems silly to do that for just a single value.

Is there anywhere else to store this information? Perhaps it should be done at the cookie level, although I'm not sure how to introduce either a link (as we did in Drag/Drop) to disable the cookie.

I'm kinda at a loss here of how to best store this user preference information.

IMO a cookie is a bad idea, as it would not be very persistent.

Agreed that a cookie is a bad idea, and that {overlay_users} would work but seems a little heavy. I don't see what's so bad about using $user->data, though. The option of doing that exists in Drupal 7, contact.module uses it the exact same way, etc. And the latest patch in #347988: Move $user->data into own table creates a dedicated table in the user module for this and moves things to it in bulk, meaning that putting it in $user->data won't complicate the upgrade path at all - and that adding a separate {overlay_users} table here would actually be somewhat inconsistent with the proposed D8 solution.

Anyway, if someone wants to reroll with {overlay_users} they can feel free to, but I'm not even sure this whole patch is a good idea anyway, so I'm not going to :)

Here, though, is a simple reroll along the lines of what I described above - we can only store overlay data it in $user->data when we need to, rather than for all users, which is less wasteful and also should allow the OpenID tests to pass without having to modify them to look for the overlay.

Patch works as expected, uses a fieldset and thus a generic UI Element that fits the use-case.

I have no take on the $user->data vs overlay_users argument. But given that there are not many alternatives and an entire new database table for one value appears to be more intrusive than the current solution, the patch appears to take the better path.

Let's get this in, and work on any refinements in followup patches.

From my standpoint this is RTBC.

Like Damz said in #61, this shouldn't be in it's own fieldset, there are too many single-item ones on that page already. Add it to the first 'account information' one.

Suggested text for the checkbox and description:

[√] Use the overlay for administrative pages.
Show administrative pages on top of the page you started from.

O.K. so let's get into the UI nitty-gritty ;)
The text is not going to be an easy one.
As to our new standard, descriptions should not try to explain technical concepts.

Concerning this, you are right with the shorter text
"Show administrative pages on top of the page you started from."

Still this does not explain much. On top of the page? huh?

What we need anyway is a help link here, because the overlay is a new concept, that might get some people wondering.
Wonder if we should do this in this patch - probably not, else this delays commit a lot. I'll open a seperate issue for that
later.

Still: if we have a description text, it must really give a hint.
How about writing "Show administrative pages in an overlay?"
But no, this would be redundant with "use the overlay for administrative pages".
So I'd opt for making it longer, as in #76 or be really daring and not give a description
at all and only provide the help link.

As to fieldset or not: you are probably right about too many elements - and
modules will be adding on their own stuff so the place gets crowded again.

Still this depends on how much highlighting you want to give to this setting, and having
seen all that fighting over the overlay, more highlighting might be the better choice over
less. But I may be wrong. Let's do a visual mockup how it looks inside the account settings.

@eigentor isn't that what we have (nearly) in #43? Not identical, but in terms of seeing what this checkbox looks like in the 'Account information' fieldset?

@yoroy As far as the number of fieldsets on that page... We could combine them into a single 'User preferences' fieldset. It is a bit silly that there are so many and that you've got single fieldsets that are collapsible, but not collapsed. However, I think that could well be a separate issue. At the moment it is consistent with what is on the user/*/edit pages. It may be a bad pattern, but it is the pattern that's there.

I'm also concerned that there isn't a good way to provide a distinct title to any checkbox (like even the' Personal contact' form) without putting them in a fieldset and then trying to theme out the sub-fieldset. But again, that's a for a different issue.

Ok, lets go with yoroy's suggestions. I agree it feels a bit off, to not have this in a fieldset - but thats mainly because most of us are still suffering from fieldseteritus, eitherway not critical to solving this issue.

I still don't feel much for a checkbox for this, but if thats the only solution to all our accesiblity issues - I guess we have to.

Re: #78:

Show administrative pages on top of the page you started from.

...would be pretty confusing to most IMO. Maybe something like: "Show administrative pages as a pop up on top of the page you started from."?

Would it be possible to append something like the following to the description.

"May cause accessibility problems for some browser and screen-reader combinations.", or "May cause accessibility problems for some screen-readers.".

This could be wrapped in .element-invisible so as not to confuse users with no knowledge of what a screen-reader is.

In #78 @yoroy - suggested text for the checkbox and description:

[√] Use the overlay for administrative pages.
Show administrative pages on top of the page you started from.

much like I's suggested in #57 above.

Well if it inside the "account information" box, it must at least have its own heading like "Roles" or "Status". Else I'd think people will just overlook it.
Did mgifford's comment in #80

I'm also concerned that there isn't a good way to provide a distinct title to any checkbox

mean that this is technically not easily possible?

Ok. In that case I've added a prefix.

@mgifford, you already provided a great explanation in #58 of why it is a bad idea to do it this way, and no one has directly refuted that, because it's irrefutable :) Putting it in the "account information" fieldset makes no sense, because it has nothing to do with information about the account.

This should be in its own fieldset like in #76 (perhaps no need to make it collapsible, though). It's important that we be consistent with the overall UI pattern on the user account page. If people want to change that pattern I think that's reasonable, but it's also a separate issue - we shouldn't change it in the overlay module and nowhere else.

I think we've got patches now for most possible options.

@eigentor any response to my comment in #58?

I want to do what I can to address folks concerns and move this to RTBC.

And yes, we can change the pattern on this page (there's good reason to do so too), but definitely it should be as part of a new thread. Possibly with a guideline about when fieldsets should be used and how to group together different elements for maximum usability.

I'm all in for a fieldset. This is a very important setting that needs that much highlighting IMHO.
Having it inside the account information is inferior both in semantic way (yes, it has nothing to do with account information) and lack of sufficient highligting.
There was an objection in #78 - yoroy, speaketh thou, now that we have screenshots of most possible combinations.

Please someone reroll #76 against latest head and I will set it to RTBC again...

#76: 659480-user-overlay-disable-76.patch queued for re-testing.

Still applies, trying to retest

#76: 659480-user-overlay-disable-76.patch queued for re-testing.

Am not sure about Re-Testing results for #76, but I guess it has passed the re-test, queued in #90.

RTBC based on comments #87 and #88

Just to be safe let's re-upload #76 if that's the one we're talking about now :)

#659480-61: Per-user setting for the Overlay still stands:

- please don't add an option on the user page that cannot be possibly disabled by the administrator
- or at least don't make it its own fieldset

O.K. we had a little chat on IRC.
So if a fieldset is a no-go, having the solution of #86 might be a good compromise.
As the overlay definitely needs a help link, this would be my final happiness:

Sure this would need another patch to add the help link and create the help page, but for UX design.
Am not quite sure where the help icon belongs - do we have any kind of standard here?

As the setting should be inside of one of the fieldsets - having it completely outside of a fieldset is not much different from creating its own one - Account settings is the best bet, as "Picture" and "Locale settings" fit even a lot worse.

Once you have duked this out please make sure the overlay screen itself has a fasttoggle-like link so that first time users can quickly get rid of the overlay should they not like it or their screenreader make a mess out of it.

Again, it shouldn't be in the "Account information" fieldset because it is not part of the account information. This is a user preference. We really can't get around that basic fact.

If it's unacceptable for it to be in its own fieldset at all, then the only acceptable solution (short of not doing this at all) is to redesign the user account page so that other similar settings (locale, contact, etc) aren't in their own fieldsets either, and then this issue will probably balloon to 300 comments, but it will work and make everyone happy :)

Well we could do that (totally redesigning User Page), but does it really make sense in D7. Would expect it to be ready for 7.2 then ;) I foresee, again, the main argument from other sides about the setting would be about it being prominent enough or not. The alternatives we proposed so far are pretty similar as to that IMHO. People might chime in saying it does not belong on user settings page at all, which would really make a different story.

But it being on the user page I rather see us discussing if the carpet should be in a darkish blue, or a middle blue leaning towards more violet, or it being middle blue.

Would also expect the discussion to really start when people start to use it - and having requests for total change of it then. So how about a 80% solution here, as we are not changing any APIs.

There are countless Admin pages that need to improve we are going to be really busy in D8, and user settings and user display is sure to be one of them. So ist't this change really small?

I've just created this isse #856436: Improper use of fieldsets on user page to continue the discussion about re-ordering the fieldsets. Maybe we aren't clear about what is account information, but we should be.

Please bring up what other groups you might want to see in this new issue.

In the mean time this thread is about following the existing model & ensuring that there is a checkbox that allows users to disable Overlay.

@chx - not sure that's going to get through. I'm not opposed to it. I think Overlay should be an optional upgrade rather than a default that needs to be disabled. However, again, this isn't the place to discuss this. We need a means for users to disable Overlay whether or not it is a default option.

Alright, so as much as I dislike this issue as it still introduces a completely non-understandable checkbox - it being in a fieldset on its own should in no way block the criticalness of this issue as I understand it, this solution would allow us to solve the accessibility issue with screenreaders (in a somewhat hacky way).

@eigentor Please stop saying things will take 3 months. We can do it in a few days, its a fairly simple page. But lets do it in a separate issue.

@David You didn't get in yoroy's text suggestion.

@Everett Can you assure us this solves the general critical accesibility issue? IT doesn't make it accessible but at least we have a good turn-off feature.

If thats in and DamZ complaints are met (apart from the fieldset one), I will put this RTBC

This is not a critical accessibility issue; it is a non-critical feature request, filed because people wanted the ability to turn it off for user 1...

AFAIK, we have a working patch at #716612: Overlay is not accessible to screen reader users for overlay accessibility. Which doesn't change the fact that some screen reader users (like anyone else) might choose to turn it off if this feature gets in, of course :)

@David Per discussion with Everett he said it was critical for accessibility to have an ability to turn it off, as accessible as we make it in #716612: Overlay is not accessible to screen reader users it will be an outstanding issue per screenreader. For purely that sake I feel this is good, for any usability reason I still think its bogus to add a user option for a presentation layer feature, but I said that enough now :)

Can someone reroll it with yoroy's text suggestion?

Rerolled with the updated text.

My point is mainly that I think it would be premature to commit this for accessibility reasons while #716612: Overlay is not accessible to screen reader users is still actively in progress... In any case, I leave that up to others :)

Let's also add a weight to the fieldset so it shows up below the "picture" fieldset (in a more appropriate order).

Screenshot shows what the user account page looks like with this patch and with all core modules enabled.

Patch needed to be rerolled after #615138: Some pages display in the overlay in a non-adminstrative theme was committed. Here's the rerolled patch. The patch looks very good to me, and if it were not for the need to reroll it, I would have been comfortable marking it rtbc.

I'm marking this as RTBC. I believe all outstanding issues have now been addressed.

Oh ya, I did this after applying it to http://drupal7.dev.openconcept.ca/ & testing it there.

Agree this is RTBC. No matter how screenreader-friendly we end up making the overlay, it is still an accessibility improvement to let people opt out individually. The checkbox is in a logical place pending #856436: Improper use of fieldsets on user page. The data is being stored in the right place for D7. Let's go ahead with it.

Alright, I agree with what we arrived at; i.e. a checkbox in its own fieldset until we figure out a better way to group settings in #856436.

It's not a pretty setting but if people feel strong about it -- especially for accessibility reasons -- we should add it. Committed to CVS HEAD. Thanks.

I've just applied #108 in D7 alpha6, but I see no magic :(

...could I be doing something wrong, or is there some other patch I need to apply first. I did try applying #15 from #615138: Some pages display in the overlay in a non-adminstrative theme, but still nothing.

btw (of-topic), ... I've noticed in this issue that after post #104 it jumps to #107(?). Perhaps a couple of posts got deleted or something, but patch in post #107 is numbered 105 and the one after that in post #108 is numbered 106. I mean, patches are numbered as post numbers should have been, but the posts themselves seem to 'skip a beat'.

I've just tried latest 7.x-dev (2010-Jul-31) that includes changes from both issues, and still fail to see the fieldset/checkbox in question :(

Confirmed! I just installed latest dev (2010-Aug-14) and I now see the checkbox available.