Attached is a first version of the D7UX overlay we've been working hard for some time. Specs for the overlay was that it would provide specially themed admin pages on top of the real website, so one can navigate the admin area of a site on top of the actual site via the Drupal 7 admin toolbar. Goods example of the admin overlay in action are at http://www.d7ux.org/content/
Initially Paul Lovvik and I took the Popups module and ported that to Drupal 7 (#466732: Drupal 7 port) to build a layer on top of that to implement the overlay. It turned out that Popups has quite big of a custom JavaScript library on its own to manage rendering and display of the pages. It uses an Ajax based model where the popups shown are merged to the same DOM document displayed on the page. This results in interesting problems when you have elements with the same IDs on the page, or you want to theme the displayed popup window radically differently (such as with a different theme). Classes and rules in the two themes can easily clash.
So on advice following on my blog post (http://hojtsy.hu/blog/2009-jun-10/starting-make-some-drupal-7-ux-concept...) we also started to look at the Modal Frame API, which reuses jQuery UI (in core in Drupal 7) and a much smaller JavaScript library to handle the popups. It also uses iframes, which have several advantages. We can use iframes to theme the overlay content with a different theme to the original page without any problems of clashing styles or rules. It also supports the notion of browser history naturally, the back button is not lost. To open pages in the overlay, the toolbar module in Drupal core already adds a "to-overlay" class, on which we can bind and change link behavior to open in the overlay and not in a new window.
So we took the Modal Frame API code, updated that to Drupal 7 (#491224: Port modalframe API to Drupal 7) and started tinkering with a mapper module for our style/layout and linking style. Because Modal Frame API was still a bit beyond what we need for the admin overlay, we refactored some of the code we built on top of it and built all into one module we called the overlay module. I believe the jQuery UI dialogs this module reuses are already there to provide a level of abstraction, so we looked at providing a thin layer on top of it to provide the overlay functionality for Drupal 7.
Interesting challenges posed by the task:
- most links opened from the admin toolbar should open in the popup; the top menu bar was specified to not retain an active state when clicked, so that users will not assume that the lower bar is context sensitive; the lower bar should retain state to show which button's action is activated
- when pages open in the overlay, the page should use the admin theme; we used the "render=overlay" GET param to achieve this, so when this is in the URL, the page is themed with the admin theme in a style suitable for the overlay (only the content and help regions are perserved); otherwise if on an admin page, Drupal itself displays the admin page in the admin theme
- with pages displayed in the overlay, links clicked should still open in the overlay (we get this with iframes by default), but to keep the overlay theming, we need the "render=overlay" param passed on with the links
- with forms displayed in the overlay (such as with most admin forms), the results of the form submission should still be displayed in the overlay; when should we actually close the overlay on form submission and when should we keep it is up for discussion; it could highly depend on the button clicked in the form and could require Form API level information
TODO:
- links with "class=to-overlay" get a click handler; this amusingly disallows right-clicking; ideally if you right-click and open in a new tab, that would not use the overlay theming, but only left-clicking would do the overlay theming and overlay display
- we need to identify links which should not keep being displayed in the overlay; from "Find content" for example, one finds posts; when admin links on the posts are clicked, that should open in the overlay; when the post link itself is clicked, that should break out of the overlay
- same goes for form submissions as detailed above; multistep forms, "save and edit" type of buttons, etc. should keep the overlay, but when a node is submitted for example, the form should close, and the new node should be redirected to in the site's theme in the main window
- more tricky are node previews (and any other type of preview which unlike Views would like to display the output in-place in the real site theme); the D7UX effort's answer to node preview's is to break out of the overlay for a preview and display a "minibar" at the top (like in http://www.d7ux.org/edit-on-page/) to "Edit again" or "Publish" based on the previewed look
- some tricky forms and admin pages still redirect to non-overlay-themed versions of the admin pages; such as when you manually run cron or check for clean URLs; we either need a systemic way to keep the "rendering mode" or need to identify these places and fix individually
Not all of the TODO items should be solved in the initial committed version of the overlay I'd say, since one patch can only attract so many collaborators. However, if anyone is interested in collaborating on fixing issues with the overlay patch as it is so far, feel free to ask for contributor access to our open Subversion repository at http://code.google.com/p/d7ux/ (Check out testing versions of the overlay coupled with the D7UX admin theme from there too).
The overlay works with Garland and should work with arbitrary admin themes, but ATM works best with the initial d7ux admin theme from #484860: Initial D7UX admin theme. This is how it looks:
This patch has contributions from at least the original Modal Frame API maintainer (Markus Petrux) as well as Charlie Gordon and Philip Vergunst.
Comment | File | Size | Author |
---|---|---|---|
#416 | overlay-517688-416.patch | 68.54 KB | Gábor Hojtsy |
#409 | overlayinsideoverlay.png | 216.9 KB | RobLoach |
#401 | overlay-517688-401.patch | 63.91 KB | ksenzee |
#400 | overlay-517688-400.patch | 65.43 KB | David_Rothstein |
#400 | interdiff-391-400.txt | 524 bytes | David_Rothstein |
Comments
Comment #1
Gábor HojtsyOur overlay also lacks the "Help: On" indicator as designed on the overlay. That should be another TODO item, which I believe is again not about to be part of the initial patch and needs very similar treatment for the admin theme in its non-overlay mode, so will be done in a follow up patch.
Comment #2
cwgordon7 CreditAttribution: cwgordon7 commentedHere is an updated patch that features cleaner code (a bit of dead code removed too), including the introduction of a new reusable forms API property, #after_build_recursive, which applies an array of functions not just to the element it is attached to, but all of its children as well. This saves the module the effort of iterating through all the form elements itself, and can leave that to form_builder(). It is quite possible that other modules would want to use this ability as well, so instead of hard-coding it, I made it into a form property. This patch also adds the module to the default profile (though this is probably up for discussion). The images and screenshot given by Gabor in #0 still apply.
Comment #3
cwgordon7 CreditAttribution: cwgordon7 commentedWhoops, sorry, patch was missing a recent bug fix.
Comment #4
skilip CreditAttribution: skilip commentedsubscribe
Comment #5
moshe weitzman CreditAttribution: moshe weitzman commentedsubscribe. wasn't the overlay discussed at length in another issue? seems a bit shady to just walk away from the feedback there.
Comment #6
Gábor HojtsyMoshe: I don't know of another issue. If you can find one, we can move the feedback over here or we can move the action over there.
Comment #7
cbrookins CreditAttribution: cbrookins commentedsubscribe
Comment #8
Dries CreditAttribution: Dries commentedLooks great based on the write-up, but haven't reviewed it yet.
Comment #9
Bojhan CreditAttribution: Bojhan commentedNot sure when anybody is going to say it. But, ugh iframes? I get there are quite some advantages, but I know that our designers will not like us for this.
Comment #10
catch@Bojhan, there's a difference between iframes in html markup, and iframes generated by javascript, afaik the latter is OK.
Just applied the patch - clicked on admin/content - then content, then scrolled to the last item in the list, and clicked edit - the iframe worked, but now the top of the overlay started half way down my viewport. This is on firefox 3.0/ubuntu. Attached screenshot of it happening on block edit, this is recursive the more links you click - I had to scroll down about 45 cm to even find the top of the overlay on this page.
Comment #11
Gábor Hojtsy@Bojhan: since we need to present essentially two pages in one browser window (a regular site page and an admin page on top of it) with two different Drupal themes, doing this by merging in the two themes on the page would get us a lot more hatred from those designers IMHO :) Building a theme so it would not clash with an arbitrary other theme sounds a bit too adventurous.
@catch: this positioning question is interesting indeed. We can always position the overlay on top of the page, but then if you scrolled into two thirds of the document, then you might not realize there was an overlay opened. If the toolbar moves with the page, the overlay should open on the viewport top where you are. However, when you scroll up, it does look odd, that the overlay was not opened at the top of the page. Should it move up with your up-scrolling but not move down with your down-scrolling? Not sure about the right answer here.
Comment #12
karschsp CreditAttribution: karschsp commentedsubscribe
Comment #13
mfer CreditAttribution: mfer commentedI wasn't able to give it a good once over but on a brief review 2 things jumped out at me.
First, In a few places like:
the settings is not passed locally. The new function signature should have it read
And we should refer to the settings variable rather than the global Drupal.settings. This local version of the settings is the one we should pass around as well. For more detail see http://drupal.org/update/modules/6/7#local_settings_behaviors.
Second, Is the overlay module dependent on the toolbar module? The .info file doesn't say so but the following line makes me think it is:
Comment #14
Dries CreditAttribution: Dries commented@Bojhan, euhm, not quite ... there is proper iframe usage and ugly iframe usage. This would classify as proper iframe usage.
Comment #15
catch@Gabor - if I scroll down first, then open the overlay, the positioning is off and I get a big blank space at the top of my screen anyway. Pretty sure this is because the actual size of the screen changes - so when I open the overlay, my relative position is adjusted by the browser. So it's currently not working for either case. I hadn't thought of opening the overlay when you're half way down a page, that does make this tricky.
My preference would be for the overlay to open at the top, and to scroll the main page up with javascript at the same time (we either do or did have a similar thing when opening collapsed fieldsets to take you to the top of the fieldset). That way it's always in a consistent place and you won't end up with 5 metre long browser windows.
One other niggle - there's currently no [x] button that I can find to close the overlay, this doesn't seem to be mentioned in the TODOs. Note I'm being deliberately annoying and reviewing this in Garland for extra breakage.
Comment #16
Gábor Hojtsy@mfer: Note taken on the settings passing.
@mfer: On toolbar module dependency, some parts of the JS are not clearly written to make it optional but should. Needs to be fixed. There is only one place where the permission is referenced in the module, and looking at that, it does not seem to actually require the toolbar in any way. So looks like we can eliminate any hard dependency on the toolbar. For positioning and management of the active state of toolbar links, we still need to reference the toolbar, but can make those references optional.
Comment #17
Gábor Hojtsy@catch: are you not seeing the close button which is shows on the opening screenshot at the top? (I can see it on Safari and Firefox on my Mac). Maybe you did not uncompress the images to the images directory below the module?
Comment #18
catchArggh, applied the patch from #3 and forgot the images in #1, sorry. Maybe we can persuade Dries/webchick to commit the images early so reviewers don't need to remember that step.
Comment #20
drewish CreditAttribution: drewish commentedsubscribing.
Comment #21
drewish CreditAttribution: drewish commentedre-rolling around the changes to way profile dependencies are listed. setting to needs review to get test bot feedback.
Comment #22
xmacinfoLooks nice! My concern is for long admin pages and the close button position, which I would duplicate at the bottom of the screen. I've not tested this yet, though.
As for the images required for the overlay, is there any consensus to commit them before RTBC?
Comment #23
Dries CreditAttribution: Dries commentedI can commit the images early but they need to be renamed not to use abbreviations, and all underscores need to be changed to dashes. Eg. ovrly-shw-bt-lt.png needs to become overlay-shadow-xxx-xxx.png.
Shouldn't some of these be added to the sprite instead?
Comment #24
joshmillerAn enthusiastic... SUBSCRIBE!
Comment #25
Bojhan CreditAttribution: Bojhan commentedGábor: Can we please get a demo running? It's kind of getting annoying to review any D7UX stuff - due to unable to test anything.
Comment #26
sun#after_rebuild_recursive needs to be removed. That is against the nature of all other FAPI properties.
Please write this in one line.
JSDoc summaries should be on one line; additional description can follow in the JSDoc block afterwards, though.
Missing settings.
Duplicate definition of the same method.
Given the recent discussions around JavaScipt code in Google Analytics module, I'm not sure whether this will lead to major performance/memory issues.
...
Given that I see so much code to properly handle IFRAMEs, I am questioning whether it would not make more sense to follow the AJAX (Popups API) approach.
Also, it seems like we're trying to move 2-3 contributed modules into core with this patch. The maintainers of those modules have to sign-off this patch at least. (Can we please stop to repeat mistakes, please?)
Comment #27
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commentedGabor, if you want to you can use my domain and hosting environment for setting up a demo website for this UX7-stuff.
Unfortunatly I can not review/test the patch because Drupal 7 HEAD is uninstallable for me at my localhost due to problems with installation profiles.
But I'm happy to give you access to my server and let you use it for demo this stuff, so we get more people to review it..
Comment #28
eigentor CreditAttribution: eigentor commentedDemo Video here: http://vimeo.com/groups/drupalux/videos/5637226
Comment #29
Dries CreditAttribution: Dries commented- I was also frowning upon #after_build_recursive -- weird concept. Maybe it is the right thing though.
- @sun, other maintainers are welcome to participate, but they do not need to sign off on this. Given that there are various 'competing modules' already, it is unlikely that we'll get everyone on the same page. Plus, Gabor has been very open about this from day one -- he started with blog posts 2 months ago so maintainers have had plenty of time to get involved, and can still get involved.
Comment #30
eigentor CreditAttribution: eigentor commentedA medium experienced Drupal user who had never seen Drupal 7 before and was not acquainted with D7UX did a 20-minute test of the overlay. She tried content creation and gives also some opinions on sections like the permissions page, that are actually not relevant to this issue, nor even directly to Drupal 7. So just filter out the stuff that directly affects the theader. I think it removes the "create content" link and makes it visible only in the header? Some troubles ahead when not logging in as admin...
http://vimeo.com/groups/drupalux/videos/5657241
Comment #31
Everett Zufelt CreditAttribution: Everett Zufelt commentedtagging
Comment #32
Everett Zufelt CreditAttribution: Everett Zufelt commentedI'd love to audit the accessibility of this patch. Is there a testing environment setup somewhere that I can gain access to? Or, can someone give me the exact steps to get my fresh install of head patched with this patch and a path where the functionality can be accessed?
Comment #33
kika CreditAttribution: kika commentedPerhaps a dumb question: will ui.dialog() and similar popups work when overlay in on?
Comment #34
Bojhan CreditAttribution: Bojhan commentedShould the overlay not close, when you click on the dark background?
Comment #35
cwgordon7 CreditAttribution: cwgordon7 commentedOk, updated patch, I think this addresses all the feedback. Changes include:
- Various coding style changes, both pointed out in this issue and ones I just noticed myself,
- Removed #after_build_recursive, changed the patch to use hook_element_info_alter() instead,
- A click on the dark background will now cause the overlay to close,
- Some weird alignment issues occurred previously, they are gone now; the overlay always opens at the top of the page, and you are scrolled there automatically,
- Added a way for non-administration links on admin pages (such as the node listing page), to escape from the overlay by adding an 'overlay-escape' class; this is not yet implemented for all administration links in an effort to keep this a reasonably sized, reviewable patch, but it is implemented for the link to content on the administer content page as a demonstration,
- Added a way for forms to request for the overlay to be closed; this is currently implemented as a demonstration for the node add/edit form, which, if used from the overlay, will close the overlay upon submission,
Response to some questions:
- I have no idea whether the images should be added to the sprite or not, it will probably lead to faster loading times, but this pattern is not implemented for other core themes or modules, I'm not entirely convinced it should be used here. I'd welcome input from some people who are into the design side of Drupal.
- To audit the accessibility of this patch, you need to apply it to the latest Drupal 7 code (see http://drupal.org/patch/apply). An administration toolbar will appear at the top of the page; clicking on any of the links in it will bring up an "overlay" iframe. Alternatively, there seems to be some talk of setting up a live demo site, you could wait for that.
- I just tried it, ui.dialog() still works, so I'd assume other popups will work as well.
Comment #36
Bojhan CreditAttribution: Bojhan commentedEdit: It seems some are addressed by the last patch, let me retry.
Overlay
1. All tabs (meta for example) seem miss aligned.
2. I am having a horizontal scroll bar.
3. Could the height be flexible to the last item? Now you see a small difference between content mangement and user management.
Comment #37
cwgordon7 CreditAttribution: cwgordon7 commentedMy apologies, the last patch had references to the wrong filenames because I forgot to properly roll the patch with the CVS hack to add a directory. The attached patch should be properly formed.
Comment #38
cwgordon7 CreditAttribution: cwgordon7 commented@Bojhan - I think you're confusing some of the administration theme patch with this overlay patch. Feedback for the administration theme (such as the capitalization or alignment or secondary tabs theming) can go to #484860: Initial D7UX admin theme - I see you posted there as well(?) I think most (all?) of your points should have been addressed in the latest patch, I'd be interested to know if any of them remain applicable.
Comment #39
Bojhan CreditAttribution: Bojhan commentedI tried the last patch, updated my original issue and also attached the tabs alignment issue.
Comment #40
Gábor Hojtsy@Bojhan: the mockups from Mark specify the tabs to not be aligned to the end of the overlay. Look at the closup image at http://drupal.org/node/472126#comment-1719664 for example. The tabs are specified to align with the actual content of the overlay, so they are padded with the same padding from the right side like the content of the overlay. This is clearly visible on your screenshot of our implementation and at the actual mockup shot at http://drupal.org/node/472126#comment-1719664
Comment #41
Dries CreditAttribution: Dries commented1.
- Added a way for forms to request for the overlay to be closed; this is currently implemented as a demonstration for the node add/edit form, which, if used from the overlay, will close the overlay upon submission.
I haven't looked at the code yet, but the form API doesn't need to know anything about overlays. Let's keep proper separation of APIs. As said, I have not looked at the code yet so maybe this is a non-issue. It just sounded worrisome from your comment.
2. I'm perfectly happy to have the tabs align as suggested by Mark. Looks nice to me.
Comment #42
Bojhan CreditAttribution: Bojhan commented@Dries 2. It doesn't look right. But the complications of changing it seems rather large, after talking to Gabor.
Comment #43
cwgordon7 CreditAttribution: cwgordon7 commented@Bojhan (#36/39):
1. Gabor addresses this in #40, and Dries addresses this in #41 #2. I'm happy having them either way; if I recall correctly, I originally aligned them as far to the right as possible so it lined up, but some extra padding was added on the right to make it fit the mockup.
2. On which page(s) and with what size of a screen? At some point a horizontal scroll bar becomes necessary, but it shouldn't be on a reasonably sized screen and a page of reasonable width; I can try to debug this if you give me the page and screen size.
3. I don't understand what you mean by "could the height be flexible to the last item" - do you mean you want a minimum dialog height such that it's almost (but not quite) causing the page to scroll vertically?
@Dries (#41):
1. It's just a function call to overlay_close_dialog(TRUE) in form submission handler - I don't think there's anything wrong with that.
Comment #45
leisareichelt CreditAttribution: leisareichelt commentedI see that we've implemented clicking on the dark background = close the overlay.
I think we should remove this functionality as it is too easy for this to be done in error and there is no easy way for a user to recover from this error, there is far more potential to create a poor user experience than a good one, and closing the overlay is not a difficult task.
Clicking on the dark area around the overlay window should have no effect. (we did toy with doing something fun if you double clicked tho!)
There may be an argument for having the close button follow you down the screen, but I suspect that might be over complicating things as well.
Comment #46
qbnflaco CreditAttribution: qbnflaco commentedI've taken this for a spin on the d7uxdemo. It seems on the find content overlay, if you have one item on the list and click edit, the whole form has to be scrolled through the tiny gap at the bottom of the overlay. Conversely, when you have many items, you aren't able to scroll to the very bottom of the edit form. I think clicking this edit link should affect the height of the overlay and add the new edit div's height to the overall overlay.
Comment #47
sunYou don't need hook_element_info_alter() here; hook_elements() is sufficient.
What about element_children() ?
Other modules need a chance to get triggered in those cases as well. (Also note the wrong indentation for the second case)
This is a form. And forms can be altered by overlay.module.
Likewise changes in here. If overlay module needs to do something somewhere in between, then we need to refactor the other module's code. This overlay patch/module, however, must not alter anything in other core modules.
Comment #48
Gábor Hojtsy@qbnflaco: the d7ux google repo has the "accordions for flooded state screens" patch applied. Sounds like your feedback is related to how that one behaves, see #492834: Hover operations for flooded state screens
Comment #49
xmacinfo@cwgordon7 A click on the dark background will now cause the overlay to close
Is the overlay supposed to act as a regular modal dialog box? I think we should keep overlays non-clickable. The only ways we should close an overlay (and its content), is by clicking the close button or pressing the escape key.
Here we already have the close button. As for the 'esc' key, I have no clue if it is planned to support it. :-)
As for the close button, we need to think about large content pages. When at the end of a long overlay document, do we need to scroll all the way up or press the home key to reach the close button?
Comment #50
leisareichelt CreditAttribution: leisareichelt commentedI'm not sure what browsers we're supporting but the overlay has suddenly gone a little skewiff in Camino
Comment #51
Bojhan CreditAttribution: Bojhan commentedI have been, using the overlay for the past day - to actually really use it. And the click and close is quite bad, as for example in vista where you click between screens you constantly close it, heh. Lets remove that again, sorry!
Comment #52
Gábor HojtsyWhile testing this patch without the toolbar, it looks like it has some bugs where it adds ?render=overlay to form result pages even if the form was not initiated from the overlay. Looks like this can be consistently reproduced.
Comment #53
cwgordon7 CreditAttribution: cwgordon7 commentedThis is still at needs work because not all stuff has been addressed yet, but this patch should fix the bug in #52 as well as the one that caused Drupal not to install for the test bot.
A few quick comments though:
@#47 - Mostly valid criticism, I intend to roll those changes into my next patch (this one is just a quick critical bug fix reroll). I actually disagree about the use of hook_form_alter() by the overlay module to alter the node administration page, however. This is not a case of the overlay module using a node/system API; this is a case of the node module using the overlay module's API. A similar case is the search module - as the node module implements the search module's API and not the other way around, the node module is responsible for calling search_reindex() in node_delete_multiple() even though the search module could implement hook_node_delete().
@#49 - The consensus seems to be to remove the background-click behavior, so I will do that in the next patch. The escape key should currently be working though - are you having a problem with it?
@#50 - I have no idea where to even start debugging that. I'm on windows, I don't think I can test with that browser(?) - can anyone provide more information about the specific CSS that's causing those bizarre red/green/blue horizontal bars?
Comment #55
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commented@ comment#50: It's caused by the opacity.
I'm not sure, but I guess you are changing it somewhere to '1'. Which is causing problems in most Gecko browsers. Try to give it 0.9999 instead of one..
In the past I had some good experience when doing an opacity: 0.5999 instead of 0.6 to fix these kind of things, although it also depends on the rest of your XHTML-code and CSS.
Good luck...
Comment #56
cwgordon7 CreditAttribution: cwgordon7 commentedUm, I attached the wrong patch, sorry.
Comment #57
Bojhan CreditAttribution: Bojhan commentedThe last checkout of the Google repository seems to give me all kinds of errors.
http://screencast.com/t/VsZX8y2tTHD
Horizontal scrollbar?
http://drupalbin.com/10745
When creating a node.
http://drupalbin.com/10746
When creating a menu link - and it trows me in the normal admin theme, without the overlay.
Comment #58
qbnflaco CreditAttribution: qbnflaco commentedthe close link for the overlay doesn't seem to work in IE6 or 7.
Comment #59
eigentor CreditAttribution: eigentor commentedDo we have a plan how to control the tab madness? The currrent state does not really take care of it - I saw some of them stay on the "regular" node form and some go on the overlay. Might be a way to clear up the clutter, as some setups go wild in tabifying the node form (and other places like the user account / profile).
Ath the moment the layouts look nice and clean, but with a wealth of five or six tabs ... ( translation, revision, even media gets one now =8-0 http://www.flickr.com/photos/mverbaar/3758839274/sizes/o/in/set-72157621...)
Comment #60
Everett Zufelt CreditAttribution: Everett Zufelt commentedI did a coarse accessibility evaluation of the d7ux admin theme and overlay. Primarily looking at the "Content" menu item.
FF 3.5.1; IE 6; SAfari 4.0.1; JAWS 10.0.1154; JAWS6 6.00.410; NVDA 0.6p3.2; VoiceOver OS X 10.5.7.
Recommendations:
1. It would seem to me that for best accessibility the tabs, close button, etc. should always be placed inside of the iframe.
2. Links that makeup tabs should be identified as such non-visually. Some non-visual indication of which "tab" is selected should be provided. See #521852: Local tasks lack semantic markup to indicate an active task.
3. Proper headings should be made apparent to all assistive technology (all browsers) to allow users to quickly skip to the beginning of the overlay content.
The full d7ux admin theme evaluation can be found at http://openconcept.ca/blog/everett/first_glance_accessibility_evaluation...
Comment #61
eigentor CreditAttribution: eigentor commentedA new User Test of the Ovelay is online:
http://vimeo.com/5994325
Quite some of the issues identified do not relate directly to the overlay. But one is interesting: the user just did not find the close button (might have to do that far right is quite out of focus physically). Eyetracking would be of some use in this case :)
The main other issue (not so specific for the header, but to seperation of admin/live site in general): where am I? How do I get back to my live site? how do I go to the front page? Just have a look yourselves.
Comment #62
xmacinfo@eigentor: Agreed. I have not looked at the video, but I expect that the close button for the overlay will be hard to find.
I would suggest to give to overlay a real titlebar, complete with a more standard close button. To complete this, at the bottom of the overlay we should also display a regular form Close button.
The overall overlay should look like a real window so as not to confuse newbies.
Comment #63
cwgordon7 CreditAttribution: cwgordon7 commentedHere's a new patch that addresses all of the above feedback except the accessibility issues outlined in #60 which I'm not quite sure how to address yet. Changes include:
- Fix for the IE bug in which the overlay would not close.
- Fix for the errors in #57
- Opacity fix explained in #55 (using 0.9999 now instead of 1)
- Another bug not mentioned here was fixed - if the iframe is resized dynamically through javascript, such as with collapsible fieldsets, the overlay does not expand to keep up with the expanded iframe. To solve this, I have the javascript checking every 150 milliseconds for a resized iframe. Although this is admittedly a somewhat ugly solution, I thoroughly explored other possible solutions, such as some javascript event (no fitting events exist) and requiring Drupal javascript to trigger the overlay's javascript when the page is resized (even uglier than this solution as well as bad developer experience). I'm open to better suggestions, but I think this is the best we can do.
- Rethemed parts of the overlay to match changes to the Seven administration theme.
Comment #65
markboulton CreditAttribution: markboulton commentedI think, watching the video more closely, that the issue isn't perhaps the close button, but rather one of orientation. That aside, we should provide more feedback and more ways in which the overlay can be closed. We should perhaps consider a 'tooltip' like hover when hovering over the space outside of the overlay to provide more feedback. I also think the transparency could be a shade lighter so it's obvious that the overlay is overlaying your site, and to get back, you just close the overlay.
The issue of Where Am I?
We've got a few issues I think. Firstly, the notion of content types being 'in site' eg job posts, comments, forum posts. And content types being 'in admin'. The current prototype deals with this by showing an overlay for the latter, and the non-javascript admin theme for the former. This, in my opinion, is wrong and could be adding to this user's confusion. Going right back to one of our core UX principles: 'I should know where I am'. As a user, I should do 'in site' content things in the site, and not in the admin. And as an admin, I should do admin type stuff in the admin and not on the site. This distinction is important for the 80% of content creators and this video clearly shows.
Lastly, it's worth noting that this is just *one* user test. I don't think we should be making changes based on feedback from one person's experiences.
Comment #66
JuliaKM CreditAttribution: JuliaKM commentedI'm not sure whether you are looking for specific bug reports at this point. I noticed a few problems with the modal box after installing the patch in #63 and testing using Safari 4.02 on a Mac.
1. When I go to Content > Find content and am on the Comments tab and then click the Content tab, the box juts out to the site. Basically, what happens is that the text box below the Content tab moves to the right and then back in place to the left.
2. Similarly, when I switch to any menu item with tabs, the box will move the right and then back again in place to the left. When I go from a navigation item with tabs to one without, the right side seems to go out to the right and then come back in.
3. I'm also seeing a scroll bar on the Appearance, Configuration and modules, Help, and Content pages. (screenshot attached)
4. I could not seem to find the close button. I eventually realized out that you could close the box with escape, but I spent a few minutes searching around puzzled. The little box with a circle on the right isn't showing up for me. (screenshot attached)
Comment #67
Gábor Hojtsy@markboulton: I've opened an issue to migrate our "all admin node editing"/"all non-admin node editing" to a notion of admin and user content types (per type): #546186: Migrate overall node admin theme setting to per-content type
@JuliaKM: Looks like Charlie did not post updated images. Just use the images from the start of the issue. Some of them are not needed anymore, but it should be fine for testing for now. The close button will show once you extract the images under the overlay module into an images directory.
Comment #68
eigentor CreditAttribution: eigentor commentedYeah, we definitely need more user testing. Am a bit out of the loop at the moment due to other work.
Maybe some of the people Leisa triggered with her call for testing for the Header prototype can be re-enthused?
There were quite some people doing tests.
Comment #69
mthart CreditAttribution: mthart commentedsubscribing
Comment #70
Gábor HojtsyI've made some changes in the google repository (not rolling new patch, since we have other outstanding issues):
- removed border from the tabs, they are not supposed to be on the overlay tabs, only on the standalone theme tabs; the overlay tabs stand out in themselves from the background
- removed some extra margin from inbetween the tabs
- tried to make inactive tabs again show up 1px above the overlay; this is not consistently doable with our current CSS/markup in Safari and Firefox for some reason
- pushed the tabs back 20px (same as the padding on #page inside the overlay) as on the mockups and as highlighted by feedback from Mark and Dries
- moved from 0.8 opacity to 0.7 as per Mark's suggestion to make the overlay look more above your site and not obscure your site as much
There are some other issues I found but did not look into fixing:
- if you resize the parent window, the overlay does not go with it; I'd suggest looking into binding to the parent window's resize event: http://snipplr.com/view/6284/jquery--window-on-resize-event/ the background resizes already but the overlay itself does not
- the tabs are still not spot on with 1px margins; there is still too much space inbetween them
- even on a relatively speedy machine like mine (core 2 due), I see the scrollable table headers jump to hidden when I open an overlay with a table in it (this may be an issue with the table headers, but others like JuliaKM on #66 experienced similar jumping around due to slowness)
- related: when a new overlay is opened or a new tab is clicked, the overlay becomes small and the drop shadow shows on the white background while the page loads; not nice
- you forgot to include the Seven style.css changes which make the messages look better with added padding, so they don't go to the edge of the overlay
- paths like /admin/* and node/add/* (if configured to be admin themed) should open in the overlay; ie. we need some kind of path rewriting to include the render=overlay GET param; maybe not the exact scope of this patch but a workflow issue to keep in mind nonetheless; note that the above user testing showed that there can be considerable issues due to not implementing this
Comment #71
qbnflaco CreditAttribution: qbnflaco commented- paths like /admin/* and node/add/*
How about node/*/edit too? It's a little inconsistent that when editing a node, the same overlay doesn't come up as when you first created it.
Comment #72
Gábor Hojtsy@qbnflaco: of course it would be best if the Edit tab would not show on the node or would at least lead you to the overlay for editing just like when you submitted the node.
Comment #73
webchickCould we please get a new patch here? I'd really like to test this, but don't fancy testing an old, crusty patch, and the with the d7ux repo I can't tell what's part of this issue and what isn't.
Comment #74
cwgordon7 CreditAttribution: cwgordon7 commentedHere's a rerolled patch as requested, it still needs the issues pointed out in #70 to be fixed, hopefully a patch containing those fixes will be up later today, but here's a reroll that just incorporates everything currently in the d7ux SVN repository.
Comment #75
cwgordon7 CreditAttribution: cwgordon7 commentedOk, I beileve this patch should resolve all the current issues:
- The overlay now resizes with page resize (only a resizing of the width is necessary, as a design decision was made earlier when we decided that an overflow in overlay height would cause the page to scroll, not the overlay, to avoid a scroll bar within a scroll bar scenario).
- The overlay height problems should be fixed now, it's a lot smoother opening/closing the overlay.
- Tableheaders.js had a slight problem, I am not quite sure why it was not showing up on normal pages too, but this patch now fixes tableheaders.js to avoid the flash of the sticky header.
- Changed seven.css so messages set in the overlay don't force the messages beyond the boundaries of the overlay.
- Caught a minor spelling error and removed a dead comment.
As you said, the workflow issues here are somewhat outside the scope of this issue; I happen to think it's a bad idea to always open certain links in the overlay (if the node/add page is opened outside the overlay, is the right thing to do to open individual node/add/* pages in the overlay each time?), but that discussion can go in another issue.
I think that should address everything, and this patch is ready for reviews again. :)
Comment #77
cwgordon7 CreditAttribution: cwgordon7 commentedSorry about that, patch didn't apply to default.info, rerolled version attached.
Comment #79
cwgordon7 CreditAttribution: cwgordon7 commentedNo idea why this was affected by this patch, but locale.test seems to be failing because the regular expression
!admin/config/international/translate/edit/(\d)+!
is matching the pathadmin/config/international/translate/edit/10
as0
rather than10
, so a minor change in the test case's regular expression fixed the failures for me. (I don't understand why this wasn't failing before).Comment #80
cwgordon7 CreditAttribution: cwgordon7 commentedApparently HEAD changed the paths on me so the locale.test stuff no longer applies, this one should work for real now though!
Comment #81
Everett Zufelt CreditAttribution: Everett Zufelt commentedHave the accessibility concerns (particularly 5.1 and 6) in comment 60 above been addressed in this most recent patch?
Comment #83
sun.core CreditAttribution: sun.core commentedComment #84
sunComment #85
sunI considered this patch as one suitable target for the code sprint today and thereby assigned this issue to me.
Please note that I'm not really good at properly expressing all of my thoughts in reviews like this. I've been told in IRC that I can post this review - I don't mean to insult you, because you put a lot of effort into this, but I also put a lot of effort into my review, so please don't take it personally. I'm trying to stick to the technical issues.
- The first and foremost major issue I have to question is: Why is UI fanciness tackled before actual functionality? I see that this overlay properly handles a fancy shadow below and next to the overlay area, a close button that is nicely aligned to the right of the overlay, and a title along with tabs that are nicely placed at the top-right above (outside) of the overlay. The actual overlay functionality, however, is taking a wrong approach and is not at all commit-worthy.
- The stylesheet depends on Seven theme, because certain CSS selectors are used that simply hide the page header and footer in the output. Disabling Seven theme or changing the admin theme to something else shows the entire page output in the overlay - so the entire JS/CSS functionality is tied to Seven theme and it does not work in any other theme. This approach will not work out, because themes can apply arbitrary CSS IDs and classes, and it is not the job of an overlay feature to determine them correctly. The new action links that were committed today already break this assumption.
- The overlay functionality simply outputs page content in an IFRAME as if the user would request the same page without an overlay. This means that we load and render a lot of unnecessary data in the header and footer and do not take advantage of faster page request times that could be possible if only the requested page fragments were delivered. Themes are able to build a lot of things around the main content, and it is absolutely not clear how we want to enforce a CSS ID of "#content" for the main content in all core, contributed, and custom themes to make this work consistently and properly.
- When clicking links in the overlay, it is easily possible to display the entire, regular site content in the overlay instead of the parent window. Some links in the overlay try to open a new window on behalf of the user instead of using the parent window. All of this is most probably caused by the previous issue I mentioned, i.e. the overlay simply does not know which link belongs to the overlay and which does not, because the overlay does not only contain the content we actually want to display - it contains anything that is possible to do in a theme.
Those issues should be solvable, but in the current approach taken, they require a major re-thinking on how the entire overlay functionality is supposed to work. The current implementation, which just outputs the content that a(ny) menu callback returns, is not going to work out - instead, that should have been the primary focus of the implementation - leaving UI fanciness aside.
Yes, this is an overlay functionality. But aside from jQuery UI's built-in features, I have a hard time to understand why this needs so much code to open an IFRAME - because that is all what it (properly) does currently (for me during testing).
I am sorry - but this is a won't fix to me - at least for D7. It's very unlikely that we find a completely revamped solution, from scratch, for D7, in the next 7 days.
Comment #86
Everett Zufelt CreditAttribution: Everett Zufelt commentedI hope that any reworking / redesign of this component takes accessibility into consideration as well as usability. As always, I am available to assist with accessibility considerations at any stage of the development process for core components.
Comment #87
markus_petrux CreditAttribution: markus_petrux commentedReally glad to see this is happening. :)
Subscribing.
Comment #88
catchSun makes a lot of good points in his review. When I tried the overlay out, a few weeks ago, my main complaint was watching the spinner for some time before the page opened - this on localhost with a reasonably fast machine.
When something loads in an overlay, you notice the time loading it a lot more than when it opens in a browser tab, because overlays/AJAX are supposed to be fast - and progress is displayed more prominently, so this needs to be as optimized as possible, which absolutely means only generating what we need to see.
Seems like this ought to be possible to handle in hook_block_list_alter(), which would avoid running block callbacks at all. Or if that's no good for some reason, hook_page_alter() would at least save them being rendered/themed then thrown away, But doing it in preprocess is really, really wasteful.
Comment #89
xmacinfoLet's postpone this until we have a better implementation. Sun's review is well done and goes to the point. The overlay functionality is not ready for Drupal 7 and may need to be re engineered from scratch.
Comment #90
Bojhan CreditAttribution: Bojhan commentedJeez, it is not nice to mark something postponed, just because you don't like it - reviews are meant to be addressed - they have not yet been, therefor this issue by far cannot be put on postponed.
Comment #91
eigentor CreditAttribution: eigentor commented@sun
While your review adresses major issues, it puts you in a clear position to provide patches. I have seen something very similar happen to plugin manager after it had been worked on a long time.
Issues are there to be raised, but whoever raises the criticism takes on a lot of responsibility.
Yeah, the long loading time is a bit annoying, but not really that bad to me. So let's try to improve it!
Comment #92
markus_petrux CreditAttribution: markus_petrux commentedBoth, an AJAX request and a full page request are supposed to perform a full Drupal bootstrap, so probably the difference is basically located on the amount of stuff a full page may require, but this can be reduced a lot when css/js aggregation is enabled, also page compression, etc.
Aside, there maybe times when AJAX is ok, but sometimes a modal dialog taking the content from a full page request may make things a lot easier. All you need to do is optimize the page processing to avoid all the stuff that would only be required for a real full page, but not when the same exact page is rendered within a modal dialog. This is where optimization would have to focus if this is going to get in.
When I wrote Modal Frame API, I tried to find a way to make it possible without touching core, but... when someone has the key to change core, things change a little. The whole page template processing can be tweaked to take into account the differences between a full page and an overlay.
Comment #93
Gábor Hojtsy@sun:
1. On UI fanciness: we built tools for modules to (1) let links open in the overlay (2) keep all links and forms in the overlay by default but let modules specify links which open outside the overlay and this is what is required API-wise from the overlay; it is another way to display admin pages and we need a way in and out for modules; sizing is specified inside the overlay; its looks are specified in part by the framimg (that is overlay module) and in part by the theme.
2. Seven vs. the overlay: hey, you just explained that we should do stuff like we do :) Open the bottom of themes/seven/style.css and you'll see that Seven specifies overlay specific styles. You say the overlay requires Seven, but this is not at all the case. Themes can specify similar overlay specific looks so they fit into the overlay better. Steph from Top Notch Themes has an issue to let themes opt out from being turned an admin theme, and others suggested (in the Appearance page issue) to let themes opt out from being the front end theme, because doing a good UI for both is hard (see admin specific themes and try to apply them to the front end). While it would be ideal to have all themes work nicely for admins and front end themes, it is not the reality even without the overlay (the given issues were opened without consideration for the overlay even).
3. On the iframe, this was discussed above as well. Markus points out that we already *almost* only generate the output that is required to be displayed, the CSS hides only a little amount of the items on the page and PHP alters out the rest. @catch points out that we do that in a wrong way and should alter the block list instead. Rightfully. Generating the same output via an AJAX request would still require us to push all the data through themed in HTML or we'd need to reimplement the Drupal rendering system in JS to overcome downloading the whole rendered HTML. On #content, this supposed inability to enforce certain things in core and contrib came up with the toolbar issue as well. We are not doing a contrib module here, so if people do themes and those do not work with our tools they will figure out that we had an important requirement there.
4. Clicking links in the overlay, the overlay rightfully does not know about the nature of links. Therefore we have ways for modules to specify which links are not administrative and we assume all other links to be administrative on administrative pages. This is not the task of the overlay as you nicely explained, it does not know about the details and it does not need to know either.
@markus / @sun:
On minimizing the page output, modal frame API has a separate template for stuff opening in the modal frame, which we dropped in favor of reusing the original page template and just dropping some regions. This made it easier to adopt existing themes to the overlay, since the theme does not need to have specific optimization code to remove blocks generated to non-displayed regions. As said above, the overlay needs to do a better job at removing blocks earlier on, so that they are not generated. Also, I'd assume @sun run his D7 environment in a development mode without JS and CSS cache turned on, which as many people said can also be an issue here.
Comment #94
markus_petrux CreditAttribution: markus_petrux commentedTo minimize the stuff that's generated during page template preprocess, maybe a way would be that there's an attribute that can be consulted in these preprocess functions so they know an overlay mode for the page has been requested, and then there are a few things that can be completely ignored, those that make no sense for an overlay output. This would solve the issue that we do not only not send certain stuff to overlays, but we also do nothing that consumes resources for nothing.
Comment #95
eigentor CreditAttribution: eigentor commentedWhile sun is taking on the role of the heretic here, he might have a point.
Stepping back for a moment from the technical implementation, we should reconsider what this overlay is trying to achieve.
1. User knows he can close this anytime to get back because he sees the page he just left through the transparent gray background
2. Help orientation
3. Provide a unified look to admin pages even when not using seven as admin theme. (black background unifies)
All user tests that have been performed so far, http://rufzeichen-online.de/user-testing-d7ux-screencasts-and-writeup clearly hint at that the users do not get point 1. Use it yourself, you'll see. Even if it is transparent, you just see pitch black. It feels like another page. (and if it would be more transparent, this would only be confusing) This leads to:
Is an overlay, that covers the entire page, anymore an overlay?
Technically yes, but we are targeting at UX.
for point 2. Orientation is a completely different issue, that should have all our focus.
So what do we gain by the overlay?
Faster loading time - no - rather slower, especially the initial load.
Better orientation? no, not atm.
As for point 3., the unified look: I actually like this a lot, as a clear "this is admin area" indicator. Still we don't need an overlay for that, but could load any admin theme into an Iframe without Javascript.
To get the user back to the page he entered the admin area from, I am sure we could find another implementation for (some kind of keeping the starting page as a destination argument?)
Technical implementation: I have seen all kinds of funny effects while using different iterations of the overlay. Pages do not load as expected, something is not loading at all, the newest is the "Window inside a window" effect, that duplicates the entire admin header (to be seen ont the first video)
Knowing about Drupal's many moving parts, even not being a coder, I'd say it is pretty hard to herd all these cats (moving parts) reliably into an overlay and have no strange effects. Is this really worth the pain?
ATM we are wasting time on a tricky technical matter that we should rather use on improving orientation, and rethinking what we are actually aiming for here.
Anyone involved in this, please do a quick user test with a non-involved person. It definitely changes your vieving angle.
Comment #96
Gábor HojtsyTesting feedback: the modules form or the admin/people filter form return to a non-overlay mode when submitted (and this is probably a pattern that applies to other forms too, but already noticed on these two).
Comment #97
Gábor HojtsyWindow resizing works flawlessly most of the cases but it sometimes gets confused and can size the overlay too small or too wide. I cannot find a predictable reproduction recipe for this, so just posting this observation here. It did not seem to be related to what page I am on.
Comment #98
markboulton CreditAttribution: markboulton commented@eigentor: Some feedback regarding your comments:
Point One: Steps have been taken to reduce the darkness of the overlay. This will help show the site beneath.
Point Two: Orientation. See below.
The overlay is response to our #1 UX principle: As a user I should know where I am. (eg, I'm in admin, overlaying my site. I can close this overlay and then my site is just there). This has performed well in all of the testing we've conducted to date. I'd argue the problems your users had were related to orientation, not necessarily the overlay. These issues of orientation will be helped enormously by the new IA, the breadcrumbs and the dashboard.
One thing that did not help the orientation was that the non-javascript version of the theme is being used in places (eg from Edit in place) - this of course is going to cause confusion. It's certainly my recommendation that this should be removed asap and be replaced with the overlay.
Comment #99
eigentor CreditAttribution: eigentor commented@mark
Well, actually the users had no problems with the overlay. It is just they do not recognize it as such. To really verify that point I sure would have to target the tests more towards that.
The motivation of doubting the overlay is rather that of not so small technical problems. They may not be unsurmountable, but at the present they are using quite some of our limited dev power. And the main things we want to archieve do not need an overlay. We could do with or without the it. But what we have to work on is orientation inside the overlay-like interface.
What I would love to see is a lot more user testing, for we are right at the point where we need that. This would also overcome the maybe subjective outcomes of my tests.
Comment #100
webchickI think what Mark's saying is that this testing already took place, and was shown to really help things, when taken in context with the whole of D7UX improvements, of which we are only about 70% there. I don't think that's a reason to throw on the brakes and not proceed further until we get *more* user testing, I think that's a reason to get this stuff polished up and in so that we can then test the whole instead of individual parts and make a sound decision at that point as to how we present admin stuff.
Comment #101
Bojhan CreditAttribution: Bojhan commentedOk so lets keep implementing
Comment #102
cwgordon7 CreditAttribution: cwgordon7 commentedThis patch should address most, if not all, of the feedback above:
- Uses hook_block_list_alter() instead of hook_preprocess_page() now for performance.
- Works using any theme as an admin theme - does not look as nice using garland, but is acceptable, completely usable, and looks ok.
- Uses a smaller dialog width so that it is more apparent that the overlay is actually an overlay - it was too easy to miss the background before, no matter how transparent it was. I confirmed that the smaller dialog width was better using an extremely small and statistically insignificant test sample (1 person with the larger width, 1 person with the smaller width), and asking them to go from a user page to admin > config & modules > modules, enable the tracker module, and return to the original user page. I understand that this isn't really a scientific way to determine whether it's more usable this way or not, but it justified the change of width in my mind. :)
- Changed the weird redirection behavior - you were right, it was just weird, and is much better off removed and forgotten.
- Changed the resizing behavior to address the bug Gabor ran into in #97 - interesting problem involving negative or partially negative iframe widths (not a good idea).
- Added a real way to get rid of the toolbar - we were still calling toolbar_suppress(), which, unfortunately, was not an actual function and therefore did absolutely nothing.
- Fixed a few interesting JavaScript errors, which should also help to speed up the overlay.
- There is a class that can be added to links to have them open outside the overlay; this can be added automatically or manually to links coming from admin pages, but this is probably best to do in a followup patch to avoid this patch becoming huge.
Thanks everyone for the feedback, I hope we'll still be able to get this patch ready before code freeze. Also, please remember, if you're testing this patch outside the d7ux repository, you'll need to download the images from above.
Comment #104
cwgordon7 CreditAttribution: cwgordon7 commentedI can't confirm those failures locally, retest?
Comment #105
eigentor CreditAttribution: eigentor commentedAh, sounds good.
I guess the newest patch is always in the version in the google repository?
Comment #107
Gábor HojtsyThe latest thinking on navigation around with Seven includes a breadcrumbs. (Now that #548806: No breadcrumbs in Seven is committed). So the overlay should also include a breadcrumb to help navigate around in the admin interface. Latest mockups for the overlay include the breadcrumb styled with the existing right arrow image in the theme.
Disregard the + button on the title here, that is related to customizable shortcuts. See #511286: D7UX: Implement customizable shortcuts.
Comment #108
Everett Zufelt CreditAttribution: Everett Zufelt commentedI haven't tested the most recent patch, but if the tabs are still being rendered outside of the iframe it will mean major accessibility problems for screen-reader userson the Mac.
Comment #109
mcrittenden CreditAttribution: mcrittenden commentedSubscribe.
Comment #110
eigentor CreditAttribution: eigentor commentedSome Issues with the overlay:
when I open screens, e.g. for adding a node, the form is moved to the top a bit and the heading is lacking. This only goes away when you enter text into the title field. May be because I just dowonloaded
The resizing of the white area is a little jumpy. It's ok it works in steps, but you should include more of them, since I quickly get this (a bit wtf-y) display:
There must be something wrong with the calculation of the screen width.
+1 for reducing the width. I now clearly see it is an overlay. This might be a good compromise between confusing and making clear it is an overlay.
Though there is one issue, that is maybe unsolvable (and may be due to me using an old CRT display :P haven't counter-checked reliably on my notebook):
During the day, when the room is bright and there is much more light, the background appears almost pitch-black as I wrote earlier. But at night with only my lamp on, things are completely different, and I clearly see Garland shining through. Hard to find the right compromise here :(
Comment #111
cwgordon7 CreditAttribution: cwgordon7 commentedThis patch should fix the problems pointed out by eigentor in #110 and by Gabor in #107. Also includes some minor CSS changes to Seven that were necessary to get the alignment to work properly both inside and outside the overlay.
I still cannot get it to fail tests (either on ubuntu or windows), so hopefully this one will pass. Otherwise, if anyone can confirm the failures / tell me which assertions are failing, that would be awesome.
Comment #112
webchickThis needs a quick re-roll for the hook_page_build() patch that went in. Reviewing now.
Comment #113
cwgordon7 CreditAttribution: cwgordon7 commentedHere's the quick reroll for the hook_page_build() patch (plus an unrelated conflict in tableheader.js).
Comment #114
dergachev CreditAttribution: dergachev commentedThis looks nice, but I'm concerned that users may accidentally lose a lot of work in the node creation form by hitting escape.
Either this should be restricted to less complex tasks, or a javascript warning should appear.
Also, perhaps "escaping" could just hide the overlay, so there's some way to bring it back?
Comment #115
cwgordon7 CreditAttribution: cwgordon7 commentedFine, you want a confirmation box, here it is.
Comment #116
ronald_istos CreditAttribution: ronald_istos commentedSafari 4.01 pages are just stuck on "Loading..." on current head with only this patch applied.
Comment #117
cwgordon7 CreditAttribution: cwgordon7 commentedTry clearing your cache first. The problem should go away. Also make sure you're really on current head because the .once() patch broke all JavaScript in Drupal for a few hours yesterday.
Comment #118
Gábor HojtsyWorks for me on Safari 4.0.3. (@istos: make sure to apply the security updates). I'd have two notes:
- I think the width of the toolbar is not as designed. It would be best to work towards implementing the design and tinker with it based on volume testing.
- The background and theming of the breadcrumbs were not as designed. At least it would be good to have it on white. We can hopefully fix the image arrow in place of » later, if not in this patch. See #107 above for the mock for the breadcrumb.
Comment #119
robertgarrigos CreditAttribution: robertgarrigos commentedmmm trying this patch on a Mac (safari and firefox) and cannot see the cross button to close the overlay window. it should be there ,right, as I see in the screenshots?
Comment #120
Gábor HojtsyUploading the remaining images needed to make the overlay close button, "loading" and shadows appear as intended. Copy these to the modules/overlay/images directory.
Comment #121
cwgordon7 CreditAttribution: cwgordon7 commentedAnd here's an updated patch for the breadcrumb styling.
Comment #122
robertgarrigos CreditAttribution: robertgarrigos commentedIt works almost as expected on Mac (safari and firefox):
1.- I miss some padding when previewing a node (see the attached image).
2.- I cannot see the arrow in the breadcrumb. Definitely the image is missing in the previous .zip but I cannot tell if that's all what happens.
3.- When publishing a node a confirmation box appear asking permission to close the overlay because I might loose unsaved work. While this is useful when closing directly the overlay it's confusing when hitting the save button.
Comment #123
cwgordon7 CreditAttribution: cwgordon7 commented1. Whoops, fixed to be a more general CSS solution that should work in all cases of contributed-module junk thrown into pages.
2. This is really a seven theme issue and I would prefer to keep it out of this patch. The » would appear to be fine for the moment.
3. Yes, this was a bug, I added a Drupal overlay close skip warning flag which is used on the automatic closes.
The updated patch is back to needs review.
Comment #124
robertgarrigos CreditAttribution: robertgarrigos commentedIt works great now. thanks! we just need make sure that those images get shipped with the module also.
Comment #125
webchickHm. I'm getting some rather obvious funky behaviour testing this patch.
Table headers get momentarily duplicated when I flip back and forth between tabs:
The next thing I did was go to "Find content", click the "Add new content" link, see the node form. Trying to use my trackpad to scroll down to the bottom of the form to find the save button resulted in some serious delays, but eventually made it.
Once I clicked it, I get this:
After about 30 seconds or so I finally get forwarded to node/2.
Performance on this seems unacceptably slow (FF 3.5/OSX). Is there something that can be done about this?
In general, it feels like this patch has not really been reviewed thoroughly before marking RTBC. :\
Comment #126
Everett Zufelt CreditAttribution: Everett Zufelt commentedHave any of the accessibility concerns in #60 been addressed in the current patch? Is Overlay set to be enabled by decault with a d7 install?
I feel that if there are still unaddressed accessibility issues that the overlay should not be enabled by default in a d7 install, if for no other reason than having it enabled may make it more difficult for some users to disable it.
Comment #127
ksenzeere #126: I believe overlay will be enabled by default. However, usability issues can be addressed post-freeze, and that's definitely a usability issue.
This is a subscribe post in a cunning disguise. :)
Comment #128
robertgarrigos CreditAttribution: robertgarrigos commented@webchick: I did try my best to test this patch. I tested on OSX (safari 4 and firefox 3.5.2) and I'm not having, didn't have either, the problems you are having when flipping back and forth.
I did also the test of the 'find content', then click 'add content' and click on a type of node with no delays problems, both during my previous test and now again. It is true that I tried on two different sets of firefox: with and without firebug. And that with firebug enabled it is slower, yes, but many webs are slower with firebug enabled anyway. In any case, I had only a few seconds delay once of several clicks, and had no delay with firebug disabled. I've also navigated thru the overlay with no problem to other pages.
Maybe I should have asked for a test from someone else before marking this issue as RTBC, but I indeed didn't have any problem, so my test was, and is still, absolutely fine.
@Everett: sorry about this, but I did not read all the comments (my fault) and I was unaware of this. cwgordont7 states at #63 that hi doesn't know how to address it. I couldn't find any other reference to the accessibility issue, so I guess there has not been any change on this. I don't have the tools to test it myself either.
So leaving this issue as it is. Maybe someone else will do it better.... :-(
Comment #129
David_Rothstein CreditAttribution: David_Rothstein commentedSome feedback while I subscribe...
Currently the patch does enable it by default, but like anything else, that should be up for debate, no? :)
Because of that, if this patch is going to enable the overlay by default, then it should also remove this line of code that is currently in the default install profile:
That feature is not a good one for most kinds of Drupal sites, and the overlay seems to remove whatever rationale there was for having that enabled by default in the first place.
Comment #130
RobLoachAdding the awesomeness tag, because it is awesomeness. You can remove the tag if you disagree. kkaefer and tha_sun would be good reviewers to have too ;-) .
jQuery Once this stuff! :-)
$('a.to-overlay').once('overlay').click(function()) {
// We rock the socks!
});
jQuery Once might work here too ;-) .
The problem with just adding UI Core and any other jQuery UI files without the library is that it becomes problematic when we update jQuery UI through contrib (jQuery UI module in Drupal 7).
I tried using "drupal_add_library('system', 'ui.dialog') and it reverted to the default jQuery UI dialog styling. Not really wanted in this case, I guess. Maybe Overlay can be a special use case here, since it's meant to be part of Drupal core anyway.
I don't think this should really be touching Node module at all. It makes sense for what it is doing, but couldn't this live in a form alter in the Overlay module instead? We ran into this problem with Comment and Node modules a while ago and to my knowledge, the spaghetti was fixed. Can we move this stuff out of node module?
I'm on crack. Are you, too?
Comment #131
lut4rp CreditAttribution: lut4rp commentedOK, I tested this with FF, Opera and Safari:
- get the images, you need to get the images (other than the latest patch file), see #1.
- clear out your browser cache, or just bypass cache for the overlay iframe, that solves MOST problems with the latest patch.
IMO, this is RTBC! \o/
Comment #132
webchickI just about locked up Dreditor, so here is my review from the bottom up, about halfway through. Still haven't gotten to major architecture stuff yet, but noticed a few kind of weird things. Have only scanned the issue, not read it in depth yet.
Also, apologies about my previous reply which a) sounded WAY snarkier than I intended, and b) was a problem on my end; I hadn't cleared my browser cache, and once I did that everything worked much better. :) Really sorry, Robert! I really wasn't meaning to take your head off! :)
Summary of major points:
- OMG this looks GREAT!*&@!@ I found no issues from a user-perspective while I was testing this patch.
- I think we should talk about where to invoke this API. Sticking calls to it in node module feels really weird.
- The hooks feel like they could use some clarification of the names as well as the PHPDoc.
- I'd like to talk about how to resolve the accessibility issues Everett brings up.
Why is this a setting and not just embedded in overlay css?
Also, it looks like shadowPath is not referenced anywhere else in the patch, so can likely be removed.
From reading the calling code, it was not clear enough to me exactly when/why these hooks are firing. Is it when the overlay is first being shown? Is it when the overlay appears? etc.
Both the name and the docs could use some work here. Ideally this would explain to me why I would implement this hook in my module.
JavaScript
Please document the possible values of $mode. Preferably with an overview of how this works, since there's a lot of discussion on this in the issue (and likely off the queue) about why things were done the way they were.
Maybe also change the name to "overlay_set_mode" if it's setting the mode.
space between "switch" and (
Let's add a verb to this hook to make it more clear what's going on. overlay_parent_initialize()? That's kinda long, but something like that which tells me as a module developer what's going on in the guts of overlay module without reading the code.
No more drupal_function_exists()
These are now ['classes'] afaik.
I really don't like adding overlay module-specific code to Node module. Node module is a required module, and an API module. Overlay is nice shiny pretty.
Charlie pointed out in #53 that the Search module integration already works this way, so there is precedent in core. But I'm not sure... it feels like this isn't an "API" per-se. We should discuss this more.
-1 days to code freeze. Better review yourself.
Comment #133
webchickOh, we encountered a bug. If you click "Find content", then click on a node title to view it, you'll get prompted to proceed or cancel because you'll lose your changes.
a) It'd be nice if it didn't do that, but I understand that fixing this is out of scope of this issue.
b) However, the "Cancel" button didn't have any effect, it still directed me to the page.
I know there's a contrib module somewhere that does this, so maybe you could look at that code for some inspiration?
Comment #134
Everett Zufelt CreditAttribution: Everett Zufelt commented@webchick
I am definitely available to discuss accessibility and the overlay. Anyone interested in brainstorming solutions can reach me at everett@openconcept.ca to find a time to discuss over phone, skype or IRC.
Comment #135
cwgordon7 CreditAttribution: cwgordon7 commentedThanks everyone for all the feedback! Quick thought before I start addressing it all:
@Everett Zufelt - Just a quick thought, could we add one-pixel transparent images with appropriate alt texts ("Close" and the names of the various tabs, if present)? We could put those inside the overlay and they would not affect the experience of normal users but would appear for users with screen readers, right?
Also, adding back the awesomeness tag, I believe it was removed by mistake?
Comment #136
Everett Zufelt CreditAttribution: Everett Zufelt commented@cwgordon7
The recommended approach to make content invisible to all but screen-reader users is to use class="element-invisible" whichs is defined in system.css as : .element-invisible {height: 0; overflow: hidden; position: absolute;}
I don't think that we should duplicate the tabs, as most screen-readers recognize them properly, only VoiceOver for the Mac does not recognize them, or more importantly, it recognizes them, but usrs are unlikely to find them.
I am not sure where focus is set when an overlay window opens, but can we set it to before the tabs, and not the iframe itself?
Also, is there a site where I can test the most recent patch so that I can give more detailed feedback on all of the issues I earlier identified?
Comment #137
robertgarrigos CreditAttribution: robertgarrigos commented@webchick: no problem :-)
Comment #138
webchickHey, Charlie! I know I owe you a review on this but I am just simply too zonked today, despite twix bars and blue gummy bears. :( It also seems like the next best step other than spending another half hour on the rest of the patch is to set up a demo site with this patch for Everett so he can take a look. Is that something you (or anyone else in this issue for that matter) could take care of?
Comment #139
int CreditAttribution: int commentedadd tag
Comment #140
Everett Zufelt CreditAttribution: Everett Zufelt commentedI applied the patch in comment #123 to a clean copy of head. I tested with several browser / screen-reader pairs.
Test:
1. Enable overlay module.
2. Select "Content" toolbar item.
3. Select Add Content.
FF3.5 and JAWS10.0.1154
1. When the overlay loads JAWS buffer needs to be refreshed to find overlay and focus is not moved from Content toolbar menu item ** known problem with JAWS 10
2. At the bottom of the page (DOM order, the way screen-readers navigate the web) after the standard page footer I find:
a. "Content" * perhaps this and all overlay dialog titles could be wrapped in an h1 fore easier navigation. H1 is recommended as this is the beginning of completely new page content.
b. A close button (tested and working)
c. "Content" and "Comments" tabs (working and #521852: Local tasks lack semantic markup to indicate an active task will help screen-reader users tell which tab is active).
d. "overlay-element" frame. Could this frame be given a title that is more specific to the type of content loaded in the frame, for example "Administer Content Dialog"?
3. When clicking on Add Content the JAWS buffer needs to be refreshed (see bug in 1 above).
FF3.5 / NVDA 0.6p3.2
1. When the overlay loads focus is taken to the iframe element. The same elements that appear before the iframe with FF/JAWS appear before the iframe here and users would be unlikely to find them. Can the focus be set to the dialog title "Content" suggested to be a h1 heading above?
2. When clicking on "Add Content" the dialog is properly refreshed with the Add Content page and focus is taken to the top of the iframe, once again it would be better if focus was placed on the dialog title "Add Content" marked up as a h1 element.
IE6 / JAWS6
1. When the overlay loads focus is taken to just before the page footer (a couple lines above the "Content" dialog title).
2. The "Content" and "Comments" tabs do not appear.
3. Clicking on the "Close link (not a button anymore?) does not close the overlay, even after clicking Ok in the dialog which appears.
4. Clicking on Add content does load the Add Content dialog and focus is taken to just above the page footer as in 1 above.
Safari 4.0.1 / VoiceOver 10.5.7
1. When the overlay loads the focus is moved to the iframe and the user is "trapped" in the iframe. VoiceOver users an object interaction model where a user interacts with container objects in order to reduce the navigable area. It is possible for a user to stop interacting with the iframe by pressing Shift + Control + Option + Up, but users may not know that they are interacting with the iframe and may not know to do this. Another reason to place focus on the dialog title "Content" and not the iframe.
2. When I stop interacting with the iframe I find the dialog title, "Content" and "Comments" tabs and Close button (which works) directly before the iframe.
3. When I navigate back to the iframe, once again I notice the iframe has the title "overlay-element". It would likely make it easier for VoiceOver users to decide to interact with this iframe (once the focus issue in 1 above is dealt with) if the iframe had a title specific to the role of the dialog.
4. When I click on Add Content the Add Content page is loaded correctly and focus is taken to the top of the iframe (once again VoiceOver is "interacting" with the iframe).
Recommendations:
1. Wrap the dialog title in an h1 element.
2. Have focus placed on the dialog title when an overlay window is loaded.
3. Give the Overlay iframe a title specific to its current role e.g. "Content Administration dialog".
4. Figure out why the tabs are not appearing for the IE6/JAWS6 combination.
Comment #141
cwgordon7 CreditAttribution: cwgordon7 commentedThanks everyone for the reviews! Responses below and updated patch incorporating your feedback attached.
"Title" dialog
.I think this is up for needs review again, as I believe I have addressed all outstanding concerns. Thanks!
Comment #142
markus_petrux CreditAttribution: markus_petrux commented1. In reply to comment #132/2 in #141 above: "I changed the code to place focus on the dialog title when an overlay window is loaded."
The following CSS would prevent a border on focus in Mozilla.
2. This replaceWith() trick could be moved to the open event handler of the dialog settings, in Drupal.overlay.create(). I mean just after
self.iframe.$container.dialog({
. That way, the jQuery html() method could be used in the place where it is now using replaceWith(). Note that bindChild() method could be called several times, after a child document has been loaded, but at that time, only the text of the title needs to change. The actual HTML tag used for the title can be set just once during the life time of the modal dialog if using the open() callback of the dialog.3. tabInxed could probably be tabindex. I also made this mistake in modal frame api. Oops!
4. This is looking impressive! :)
Comment #143
catchSearch module needs to get out of node module, so both are incorrect.
Comment #144
cwgordon7 CreditAttribution: cwgordon7 commentedThanks, updated to incorporate the suggestions in #142 and #143.
Comment #145
Everett Zufelt CreditAttribution: Everett Zufelt commentedI tested the patch in #144 with clean head.
This patch addresses the recommendations 1-3 made in #140.
For the title of the iframe, I don't think the quotes are necessary around the name of the node e.g. '"Content" Dialog' should be 'Content dialog'.
As for 140.4 (IE6 / JAWS6), I think that this is a critical issue, I am not familiar with other JS malfunctions, but not having access to the local tasks which control the content presented in the overlay dialog does appear to be critical to me.
If someone can show me the code that makes these local tasks appear I can do some research to see if I can determine the source of the problem.
Comment #146
cwgordon7 CreditAttribution: cwgordon7 commentedThe code that runs the tabs is:
Also updated the patch to just say
Content dialog
rather than"Content" dialog
as per the suggestion in #145.Comment #148
marcvangendI noticed two issues with the breadcrumbs in the overlay.
1. The breadcrumbs contains a 'Home' link which opens your home page in the iframe. I considered adding javacript to return to the home page in the parent frame, but that would be awkward because two things happen at the same time (close overlay and load the home page in the parent). We (drupalcon codesprinters) agreed that the best solution is to take out the Home link completely in the overlay.
2. Contrary to most themes, the breadcrumbs are below the title. That's why we figured it's more logical to have the page title appended to the end of the breadcrumbs (text, not as a link) - but only in case there is a breadcrumb.
I added the following lines to overlay_preprocess_page():
New patch attached.
Comment #150
cwgordon7 CreditAttribution: cwgordon7 commentedLeft an extra
print 'here'
in there, sorry.Comment #151
Everett Zufelt CreditAttribution: Everett Zufelt commented@cwgordon7
I think that the problem for IE6/JAWS6 is with the removing and replacing the ul in the DOM. I recall speaking to the folks at http://fluidproject.org who had a similar problem. I will see if I can get more information from them and report back.
Is this the only place in the Overlay where a removed DOM element is being added back into the DOM?
Comment #152
Everett Zufelt CreditAttribution: Everett Zufelt commentedSetting back to NR.
Comment #153
cwgordon7 CreditAttribution: cwgordon7 commentedOk if someone here is working on the overlay at the codesprint come find me / ping me in IRC to figure out where I am. Here's a patch that incorporates both changes.
Comment #154
cwgordon7 CreditAttribution: cwgordon7 commented@Everett - yes, that is the only case of removing and readding content to the page, as far as I know.
Comment #156
cwgordon7 CreditAttribution: cwgordon7 commentedTry again.
Comment #157
cwgordon7 CreditAttribution: cwgordon7 commentedHad accidentally removed function overlay_close_dialog() in the previous patch, thanks to Manuel Garcia for finding this bug. Attached patch should fix that bug.
Comment #158
Manuel Garcia CreditAttribution: Manuel Garcia commentedJust confirming last patch takes care of that bug =) nice job
Comment #159
Manuel Garcia CreditAttribution: Manuel Garcia commentedIn the comment administration part of the admin/content, if you have no comments it will cause problems hiting the update button. This however is not a bug with the overlay, but rather with comment module, and I have already filled in a bug report for it. Just for your info =)
#569196: Comment bulk update: warning if no comments selected.
Comment #160
cwgordon7 CreditAttribution: cwgordon7 commentedRerolled for the new hook_drupal_goto_alter(), which should coincidentally fix the problem mentioned in #159, as well as a problem for the node/add form if you only had one node type.
Comment #161
Everett Zufelt CreditAttribution: Everett Zufelt commentedI did some local testing with appending removed elements with jQuery re: IE6/JAWS6. It looks like this is indeed the problem.
I'm pretty sure that there is going to have to be another solution for moving the local tasks to the title bar of the dialog, as the remove() / append method is not going to work for users of older versions of JAWS.
Comment #162
cwgordon7 CreditAttribution: cwgordon7 commentedOk, lut4rp just tested on IE6 without JAWS, and it does not work either, so special cased IE < 7 to not move the tabs, which should solve all the accessibility issues? :)
Comment #163
Manuel Garcia CreditAttribution: Manuel Garcia commentedJust confirming that patch #160 fixes the issue mentioned in #159
Comment #164
Manuel Garcia CreditAttribution: Manuel Garcia commentedA minor thing that should be fixed is the dotted border on active title of the overlays, this can be fixed by adding:
on overlay-parent.css.
Basically that technique can be used for any element we don't want to highlight through the standard outline that the browser uses.
Comment #165
Everett Zufelt CreditAttribution: Everett Zufelt commentedTested the patch in #162 and would give this a +1 for accessibility. There are a few cleanup tiny accessibility improvements that can be dealt with in a new issue when this is committed.
@Manuel
I would recommend against removing the dotted border, as this may be useful to keyboard only non-screen-reader users to identify where they can find the focus.
Great work on improving the accessibility of this module.
Comment #166
marcvangend@Everett: I appreciate your concerns about accessibility and the focus border, but I think it's not really doing anything there. AFAIC, there is no good reason to focus on that title after loading an overlay page. In fact, that title is outside the overlay's iframe, so I guess that screen readers would have a hard time with that anyway.
Comment #167
marcvangendI think we still have to do something about the blocks/regions that are shown in the overlay. Since 'content' is an actual region, other blocks in that region are currently shown as well. Probably all blocks should be unset except system-main and system-help. I'm not attaching a new patch yet because I'd like to know what you all think.
Comment #168
Damien Tournoud CreditAttribution: Damien Tournoud commentedOverlays inside overlays: open the overlay, click administer (in the breadcrumb), click "by modules" and click on "configure permissions". Tada!
Comment #169
Damien Tournoud CreditAttribution: Damien Tournoud commentedAnother concern that I have is about the shadow. Outputting two divs from javascript just for the shadow is pretty ugly. I suggest we use CSS3 to generate the shadow, and simply do not bother with IE6/7/8. It will all look as great without the shadow anyway.
sun added that all the HTML markup that is generated by the javascript should go through
Drupal.theme()
, but I personally consider this to be a minor issue.Comment #170
marcvangendNew patch, implementing #164 and #167.
Comment #171
cwgordon7 CreditAttribution: cwgordon7 commented@Damien Tournoud - how do you suggest we use CSS3 to avoid the extra divs? Sorry if I'm being slow, but I don't see how that would work.
New patch fixing #168 coming when I get home tomorrow, hopefully. (I've got the code here, but CVS is insanely slow and Internet is charging per minute).
Comment #172
Damien Tournoud CreditAttribution: Damien Tournoud commented@cwgordon7: I meant, just use the box-shadow property, and be done with it. It is supported by FF3.5/Safari 3+. The design without the shadow will be good enough for every other browser.
Comment #173
marcvangend@Damien: I more or less agree with your point about the shadows; extra divs aren't my favorite and the design still looks fine without shadows. However, using the box-shadow property (or rather: -moz-box-shadow and -webkit-box-shadow) will not work with the current design, because there are rounded corners in it.
Comment #174
cwgordon7 CreditAttribution: cwgordon7 commentedAccording to the link, we can do rounded corners if we also use border-radius?
Comment #175
eigentor CreditAttribution: eigentor commentedas border-radius is also not supported by older browsers this may cause a headache :(
CSS-Gods to the rescue...
Comment #176
marcvangend@#174: you're right, I didn't think of that. You could give the close button a property like "-moz-border-radius: 0 100% 100% 0;" to round the box and it's css drop shadow.
@#175: On older browsers, both the rounded corner and the drop shadow would disappear, so following the line of thought in #169, this will actually save us one or two headaches.
Comment #177
eigentor CreditAttribution: eigentor commentedAnyone can make a screenshot how this looks without rounded corners? Would help a decision...
Comment #178
xmacinfoThe main Seven admin theme uses border-radius and falls back nicely to square angles for browser that do not support border-radius.
So here we should use box-shadow as well as radius-border as well. Older browsers will fall back to what they support.
Comment #179
marcvangend@#177:
Here are two images, one showing what you can achieve with css3 properties box-shadow and border-radius, the other showing what that same page would look like on browsers that only support css2. These screenshots are made using Firefox 3.5 and Firebug to manipulate the css, without the shadow images from the zip in the original post.
Comment #180
eigentor CreditAttribution: eigentor commentedHm, this is a bit of a hard one. The lack of rounding on the close button really changes aesthetics.
So let's have a look for statistics.
According to this (Firefox only counts when version is 3.5)
http://marketshare.hitslink.com/browser-market-share.aspx?qprid=2
only 14% of the people would see the nice rounded corner :(
ah, no, IE8 supports it as well?
Then it would be 29%
Comment #181
marcvangendI just did some more css/firebug tests and i found a way to make the rounded corner work in every browser. I used a close button image that is rounded, but without shadow. When that is used as background image of the
<a>
, you can then round it's corners and apply box-shadow to create the desired shadow effect, while older browsers still see the rounded image.By the way, when we look at today's browser statistics, remember that probably D7 will not be finished before 2010 and that it will still be in use in 2012.
Comment #182
eigentor CreditAttribution: eigentor commented@marcvanged: this solution sounds good.
Yeah, hopefully we will see a lot of switching from Firefox 3.0 to 3.5, which would get us close to 50% :)
Have almost given up hope that Microsoft will manage to promote their own browser. Windows 7 might be a ray of hope since it generally gets good perception and maybe even I might upgrade from my beloved XP...
Comment #183
cwgordon7 CreditAttribution: cwgordon7 commentedThis patch should fix all outstanding problems. It now uses box-shadow CSS property instead of images, so a new image zip is also attached. I decided not to use the border-radius CSS property because (a) it looks fine without it, you can barely notice a difference with it on, and (b) it caused the dialog to look weird when at first it had a border-radius when it was just a div and then the border-radius vanished when the actual iframe loaded. This also fixes the problem relating to links with fragments.
Comment #184
cwgordon7 CreditAttribution: cwgordon7 commentedAnd the images. :)
Comment #185
eigentor CreditAttribution: eigentor commentedShould we create a meta-issue for the overlay and split it up into smaller ones?
There are some recurring issues, that need to be fixed one by one. (window-in-a-window, top padding missing, a.s.o.)
The patch is not a small one and seperating it into several smaller might help.
Also it might be hard for charly to keep track of all the different issues posted here, it is easy to overlook stuff.
I ask before I do this because it does not help to have even more issues some time....
Comment #186
Damien Tournoud CreditAttribution: Damien Tournoud commentedI think this is nearly good enough so that we could commit this, and fix the remaining issues separately.
One thing that need to be fixed: there are still remains of the div shadows in the patch.
Comment #187
cwgordon7 CreditAttribution: cwgordon7 commentedSorry about that, I think I removed the last remains of the div shadows in this one. The images from #184 are still the current images.
Comment #188
mcrittenden CreditAttribution: mcrittenden commented+1 for committing this in its current state and catching the remaining problems in follow up issues. This patch will last quite a while longer if we try to get it absolutely perfect here.
So if testbot likes #187 patch, then let's RTBC?
Comment #189
Damien Tournoud CreditAttribution: Damien Tournoud commentedI opened two follow-ups:
Comment #190
marcvangendOne more iteration of the patch... I changed three things.
1) The overlay container had a CSS drop shadow, but the close button still had a drop shadow baked into the png. I applied a css dropshadow to the close button using the method described in #181. This looks good in both older and newer browsers. I'm attaching a new set of images containing a shadowless close button.
2) Change 1) made it possible to remove div.ui-dialog-titlebar-close-bg, which is why we started with the css3 properties in the first place.
3) I added a color (
rgba(0,0,0,0.5)
) to the box-shadow declarations. According to the CSS3 specs, user agents are allowed to define their own default color, so figured we'd better take control ourselves.@#188: yes, I think this is RTBC now.
Comment #192
marcvangendSorry, I made an error in that last patch (forgot one line of CSS). Please use this one together with the images from #190.
Comment #194
cwgordon7 CreditAttribution: cwgordon7 commentedSomething in your patch is failing to apply... are you rolling against an up to date Drupal HEAD?
Comment #195
sunSome files in the patch are cut off.
Comment #196
cwgordon7 CreditAttribution: cwgordon7 commentedyeah... marcvangend, you need to fix this, I've got no idea how to reroll from your broken patch state. I'd be happy to make a reroll from my latest patch, but it seems like you've made substantial changes...
Comment #197
marcvangendYes, I did roll against the latest head, and on my machine I was able to apply that patch on a clean checkout... Anyway, I did a new one, hope this works now.
(I guess I still have some things to learn about the simpletest process... What's the best way to test a patch before uploading it to the issue queue? I tend to test it, but obviously my method isn't fail-safe.)
Comment #199
cwgordon7 CreditAttribution: cwgordon7 commentedmarcvangend, some of your files are being cut off, such as overlay-parent.css(?) Please come onto IRC so we can help you debug?
Comment #200
marcvangendSorry for the mess guys, this doesn't look pretty for someting that is supposed to be almost RTBC :-)
Anyway, I used another tool now to create the patch, let's hope this works. I'll be back tomorrow, it's time for bed now.
Comment #202
cwgordon7 CreditAttribution: cwgordon7 commentedTry again, please?
Comment #204
sunI was not able to test this manually due to the broken patch file. However, here comes a (partial) code review upfront:
Please squeeze some spaces in those rgba() values.
The JSDoc needs more information - why, when, how, from where; also don't forget to document @param.
At least the JSDoc needs more information; also don't forget to document @param.
Some inline comments describing the calculations in there wouldn't hurt either.
(and elsewhere) Object/array notation should start with a space before the first element and should end with a space after the last element's value. (Does not apply to empty objects/arrays.)
Duplicate "page".
I fail to see why we need to pollute defines with those.
We can just use 'parent' and 'child' as strings instead.
I don't think that this is the right place and module to switch themes - at least not unconditionally. This should be configurable. And ideally triggered by a query string containing the theme name.
Access to administration pages != using overlay.
Your mileage may vary, but there are countless of use-cases for Drupal. We have to cater for those.
Woah. That needs to be toggleable or configurable.
Just because that Seven theme doesn't implement any regions that doesn't mean...
Why is the current page added in the breadcrumb? That is against the current paradigm/understanding of breadcrumbs in Drupal. If we want to change that, then we should change it in drupal_set_breadcrumb().
This PHPDoc description could use some rewording using less emotions.
The same applies to the inline comment in this function.
Since we have hook_drupal_goto_alter() now -- proposal: populate a drupal_static() in form_alter and check that in overlay_goto_alter() to omit/alter drupal_goto() and close the dialog on the same or next request's output.
This is hi-jacking the entire idea and concept of a library registry. Any alterations to those library files will not be taken into account here.
a) Just use drupal_add_library() or
b) open a very critical bug report to fix this limitation of the library registry - which was already mentioned in some other issues.
Additionally, all of this code, except of the $mode handling, should be moved into overlay_preprocess_page() - using the #attached property to properly load those things only on pages.
This should be registered and loaded as a library.
This needs to be moved into a hook implementation of Toolbar module.
This review is powered by Dreditor.
Comment #205
eigentor CreditAttribution: eigentor commentedPosted a follow-up: #574338: Overlay: put all admin screens into overlay optics
The screens must be unified. I see having some screens in seven theme only (all white) and some in overlay/overlay optics as a ux bug, just the same as mark expresses in #65.
Comment #206
seutje CreditAttribution: seutje commented@200 the overlay files seemed to have gotten dumped in the root instead of modules/overlay
attached patch should fix that and apply to HEAD
quick visual review:
Comment #207
seutje CreditAttribution: seutje commentedgo testbot!
Comment #208
seutje CreditAttribution: seutje commentedoh, I just overlooked the images... again, sry
can we have a fallback by clicking the black background or something?
Comment #209
heather CreditAttribution: heather commentedFYI: This patch is added to a list of patches which urgently need user testing. http://groups.drupal.org/node/26409
Closing broken?
This is the first time I tried this overlay. I clicked on the black area to try and close it. It didn't close. I am familiar with the use of lightbox/shadowboxes on various sites. These 'escape' on click of outside/black area. This could be why I am expecting this behaviour.
CSS
This spacing is affected in the site when overlay.patch is applied (enabled or not).
Spacing within the overlay:
FYI: In case it's not obvious to other noobs, you have to download head, apply the patch; dump your database and reinstall d7 to test this patch. That way, it should be enabled by default. I also tried enabling it, that works ok too.
Comment #210
Manuel Garcia CreditAttribution: Manuel Garcia commentedI've been playing around with patch #206, (btw, place the folder in images.zip inside /modules/overlay/).
I've clicked on every page and config i could get my hands on, couldn't find any bugs, so to me this is looking good, at least functionaly there are no problems.
There seems to be a weird effect when you have scrolled down on the overlay, and you scroll back up, the bar on top kinda sticks a bit before moving up, using ff 3.5... not sure its an overlay problem or what, nothing critical that could stop this patch from being commited imho =)
Can we get this in, then handle other small display bugs as separate issues perhaps? Any brave souls want to change the status to reviewed? :>
Comment #211
Gábor HojtsyTaking this on to implement hopefully most of the recent feedback. Will come back with an updated patch later today.
Comment #212
Gábor HojtsySo here is the patch based on your feedback from above. Notes:
Fixed:
- Removed page title from breadcrumb based on feedback from @sun
- Fixed preprocess hooks for html.tpl.php, the head title and body class should be preprocessed there
- Removed CSS border radius and shadow on close button since it was already in the image; extended the image size to show up
- Fixed spacing issues noted by @heather
- Based on feedback from @heather/@seutje: now that we have a notification tool to warn people when they try to close overlay with forms, I've added back Charlie's code to close the overlay on click; we might still want to extend the click area of the tabs as suggested by Mark to avoid people trying to click on them and closing the overlay
- rgba() spaces added as per @sun
- more jsdoc added per @sun (probably not as much as he would have liked though)
- fixed object notation whitespaces and "page page" duplication per @sun
- removed overlay mode constants in favor of strings but I'm not sure this was the best thing to do (suggested by @sun)
Not fixed/debated:
- @seutje: When opening a long page in the overlay (like Add content -> Page) and then going back to a short page (like Add content), I have a ton of leftover space at the bottom, causing an unnecessary scrollbar
- @sun: changing the theme in overlay_init(); not sure we should allow theme name passage in the URL, that would let all kinds of pranks to happen; we could use a different variable_get() and not provide a UI for that to have an "overlay theme", and just use the admin theme for that in core (let contribs provide a UI for that setting if they want);
- @sun: on user_access('access administration pages'), not sure what you suggest; should we have a separate permission for the overlay? how would you suggest identifying that the user is allowed to see the overlay
- @sun: on the regions allowed being hardwired, I don't think it does make any sense to have a UI there since this looks to be a theme vs. overlay specific issue; sounds more like themes would want to specify which regions to show in the overlay; since themes cannot have hooks, we can only work around this by new .info file items, which sounds bad (but I don't have a clearly better idea)
- @sun's "Since we have hook_drupal_goto_alter() now -- proposal: populate a drupal_static() in form_alter and check that in overlay_goto_alter() to omit/alter drupal_goto() and close the dialog on the same or next request's output." - not sure that is a less hackish solution then what we have now
- @sun's advice to use hook_library() fails badly on all the extra stuff that a library adds; we'd need to work hard to override all that just to make the overlay look sane again, adding twice as much useless styles to the page; sure we can go on a tangent to fix the library APIs or we can just commit this and let the library API cleanup come around and rework this based on libraries once it is capable enough (see below for screenshot of how it looks with libraries == the extra CSS added)
- @sun's advice to using #attached to add the JS in our preprocess seem to break down on the core preprocess being called before ours so the CSS/HTML string is already built once we get called to build that (from http://drupal.org/node/223430)
- @sun's advice on making libraries of the overlay files is not compatible with his advice to use preprocess (no way to add in libraries with #attach, right?), so let's clear up what is the real goal here; many ways to do the same perfectly applies
- on whether toolbar knows about the overlay or overlay knows about the toolbar to disable it in the overlay, I'm not entirely sure, we can do either way; others' opinions?
In pictures, here are the problems of the patch before my fixes in Safari:
Note that the tab spacing I did not fix (it is a long lingering issue and is not major). The others I did fix.
Here is how it looks if we add the whole jQuery UI library. We could add tons of overrides on the CSS, so we override the loaded useless CSS with our override CSS and we have twice as much useless CSS added. Instead, by not adding that useless CSS, we don't get this "nice" look:
Comment #214
cwgordon7 CreditAttribution: cwgordon7 commentedThe locale.test part from previous patches appears to have been removed from this one? Also, there is a way to add libraries using #attached_library (see #505084: Add #attached_library FAPI property for drupal_add_library()), but I don't believe we want to do that because we don't want to add the library-specific CSS files.
Note that the locale tests fail because this patch pushes the count of objects up to 10, which is not properly captured by the original regular expression (though single-digit integers are).
Comment #215
Gábor HojtsyUhm, ok, my bad. I did not notice the relevance of the locale patch. Can be added back in for sure.
Comment #216
seutje CreditAttribution: seutje commentedawesomeness!
tried Gábor's patch in #212 and I have a few additional notes:
Comment #217
webchickAt sun and ksenzee's request, I went ahead and committed a modules/overlay directory to core with an empty overlay.info file. This should hopefully make patching easier from here on out.
Comment #218
Gábor Hojtsy@seutje: on the blocks admin, I've alerted many people to this before; it is basically an issue with how our blocks admin works at the moment. We either accept this "misbehavior" of the overlay showing different themes, or institute a blocks UI which does not require theme switching. I'm not sure this theme switching blocks UI was user tested with a different admin theme, but I'd guess it is confusing. (It freaks me out to see the theme change for sure). While it makes sense to show the theme regions live in the theme, nowadays complex themes have all kinds of funky regions, which might not even appear on all pages, so it might not scale anymore either.
Comment #219
seutje CreditAttribution: seutje commented@Gábor: yea I kinda figured, the theme suddenly switching kinda freaks me out as well and it makes me wonder why I keep giving my regions proper descriptions. But it made me wonder about the whole "filtering certain regions" thing we're noticing when combining this and the dashboard
Comment #220
WorldFallz CreditAttribution: WorldFallz commentedJust want to comment that I see a lot of WTF in the forums regarding the theme changing on the block config page-- and frequently get asked if there's anyway to prevent it. I would think stopping that behavior, especially in light of the difficulty it seems to be causing this important d7 feature, is no great loss. Might be enough to provide the ability for a theme to display a graphic showing the block regions for those themes that want/need it.
Comment #221
xmacinfoDrupal now comes with two enabled themes, which causes confusion. However this is normal behavior since each theme can have it's own set of regions. When setting up blocks for various enabled theme, each theme having their own complete set of block settings is very important.
Drupal just display blocks in their proper context.
Removing the context for block settings would be a return to the past.
Comment #222
catchI'd agree with removing the theme switching - where you have regions which are never designed to be displayed on the same page, having them both pop up on this one can be a pain. In usability testing we had people trying to drag blocks into the regions, thinking they'd assigned blocks correctly because the regions were showing up and various other snags with displaying the regions there - I don't think it makes sense to display them until we can drag things into them, which is a separate issue with it's own complications, but our half-way version we have now doesn't really help.
Comment #223
seutje CreditAttribution: seutje commented@catch: which is a separate issue with it's own complications
is there already an issue opened for in-page block editing? cause I've been looking for it but can't seem to find it
Comment #224
Gábor HojtsyWhatever we do with blocks, it should be a separate issue IMHO. We are already trying to cover a lot of bases. Opened #581118: Blocks admin user interface should not do theme switching.
@sun: I've also opened #581120: Libraries can only be added in their entirety to discuss the library issue. I did not mark it "very critical" since I don't necessarily agree it is critical. Feel free to continue that discussion there.
Comment #225
Gábor HojtsyAdditionally to opening side issues, I've also worked on this issue more to fix some of the outstanding issues.
- @sun: on the block altering, I've now implemented block region removal in the overlay based on overlay_regions[] from the .info file; the two default (hard-wired) regions content and help are included with the overlay module, so no change to .info files required; these two special regions are treated special in other places of Drupal too; tested with Garland and it works fine there too; this also allows modules like dashboard to add more regions to the displayed ones, so the dashboard items are shown in the overlay
- @seutje: that white hovering issue was due to the recent patches having a malformed CSS; I've restored the active hover style that was at the end of the CSS but was not in the patches recently due to somehow broken files
- @webchick: I've rerolled the patch with the latest HEAD, so it is delta from the empty overlay.info
- @seutje: on your loading comment, Damien already opened a follow up issue at #574164: Fix transitions between pages inside the overlay
- I went back to as far as Damien's feedback in #168, because that looked like a critical issue, and tried to reproduce that without luck; I don't get a toolbar in the overlay; looks like your patched setup was somehow confused on which one is the parent and child in terms of the overlay setup; can you still reproduce that?
Comment #226
Damien Tournoud CreditAttribution: Damien Tournoud commented@Gabor: the issue was caused by links with fragments, and has been fixed by Charlie in #183.
Comment #227
Bojhan CreditAttribution: Bojhan commentedCan anyone explain to me why the last patch is so broken? It didn't seem so broken before.
1. You get a lot of whitespace, when you go from Configuration & modules, to say Content.
2. Are you sure you want to close this dialog? on /content? Do we even need an JS error? Its highly unlikely you accidentally click that tiny x.
3. The tabs are not connected to the overlay anymore?
4. Overlay scrolling seems broken, see http://screencast.com/t/pUmwioYJ
A note, I want to test this patch in next weeks usability tests.
Comment #228
Gábor Hojtsy@Bojhan:
1. Cannot reproduce this issue.
2. You seem to have missed several iterations of the patch. That JS alert for example was introduced above several iterations ago in discussion and seems to be a highly simplified version of the dirty forms module.
3. From the looks of your screencast.com video they look as intended from the mockups, even if a tiny bit farther from the overlay in the disabled tabs' case, but definitely on the overlay in the active tab's case. The exact padding inbetween the overlay and tabs and between the tabs is for some reason different depending on the browser you use since we added those tabs, but we did not manage to find out why or fix this issue. It was certainly not of highest priority.
4. Cannot reproduce this issue. Could be very well due to the slowness of the computer. Seen similar scrolling errors in all kinds of web apps before when my CPU was highly busy.
Comment #229
Bojhan CreditAttribution: Bojhan commented1. I did a clean install, I go to Configuration & Modules - then to Content. And I have a lot of whitespace at the bottom, below the overlay. I don't know what else I can do to help you reproduce it ( Vista, Firefox 3.5).
2. Sadly, I can't follow the design discussion that is happening in between all the code discussion. I completely disagree with this movement, we do not see this in our current administration - nor is it required by this interaction. Closing means closing, people know when you close, you do not save and you lose your data. But beside that debatable point, it should not be triggerd on Content (admin/content) administration.
3. Our current.
Mark's
Unless I am not seeing this correctly, but that is not representing mark his mockup. I am using firefox 3.5, it shouldn't exist in that browser the least? I understand its not a priority, but given the orientation issue with tabs already - I think its wise to make sure they are connected to the body visually.
4. Seems somewhat weird if this is due to a slow PC, I am running Intel Core 2 Quad Q9100. I didn't have much running at that time. Tried it out, with one browser and tab open - and the same effect.
Comment #230
xmacinfo@Bojhan "Closing means closing, people know when you close, you do not save and you lose your data. But beside that debatable point, it should not be triggerd on Content (admin/content) administration." --> This has been introduced for two reasons:
1. User can click on the dark part of the overlay to close the window.
2. User can press esc to dismiss the overlay.
And incidentally, the dark part of the overlay is more visible than before, the whole content narrower.
In proper UI design, we should not close a modal dialog, or here the overlay, by clicking beside it.
Also, we must make a difference about dismissing a dialog or closing a dialog.
Pressing esc should dismiss, which is in essence cancel. While closing the dialog or overlay should check if the form have been saved, even clicking on a tiny close button.
The only change I would love to see is to remove the clicking on the dark overlay to close the window.
That said, I have not tried the last version. :-)
Comment #231
Gábor Hojtsy1. Ok I can reproduce that. That is space below the overlay, not in. I was looking at the overlay itself.
2. It is currently displayed on all pages which had forms, so you don't loose your input. We did not see this in our current administration, but closing a window there was about closing a browser window/tab, and not merely clicking at the side of the screen. I'm not saying I support that this alert is in there, I personally don't like it either.
3. As said, there are "pixel issues" here. Mark's mockups also distance the inactive tabs with one pixel on each side, and as said, the distance between the tabs and the overlay is different depending on what browser you use. This is clearly a bug.
4. Uhm, not a machine issue then. Interesting I did not see this before though.
Comment #232
Bojhan CreditAttribution: Bojhan commented@xmacinfo 1. Does not exist in current patch, 2. seems somewhat as an edge case.
2. Oke, I am trying to revisit why it was introduced - I think charlie did in #141 , but there seems to be no clear reasoning as to why. We can probably remove this feature, and bring it back in a new issue - if its truly that desired.
4. I will try to see, what is causing it.
Comment #233
xmacinfo@Bojhan 2. Was introduced because there are now too many ways to close the overlay and that some users did click at the wrong place and thus lost their changes. So since this patch does not close when clicking on the dark overlay then I guess we can remove this warning.
Comment #234
markus_petrux CreditAttribution: markus_petrux commentedIn regards to the warning that appears when closing the dialog... I believe I missed if this comes from D7UX, but I did this in Modal Frame API. However, it only fires if a form in the child window has been changed (requires Dirty Forms module).
I think it's somehow annoying to see such a warning if you haven't changed anything. The warning might be there if things have changed because that means the user had a initial intention to change, but closing would loose that changes, so it makes sense. But not if things have not changed.
~~~~~
This is another issue: it seems the Overlay module could be used elsewhere by contrib modules. But there are features that cannot be configured, rather they have been hard coded with all this admin interface enhancements in mind, which is incredible, but maybe contrib modules wish to enable draggable feature, dialog size, etc.
Comment #235
Gábor HojtsyGuys, the current patch includes code to close the overlay when the background is clicked, as that was requested by many people above and outside the issue (again). Looks like we are in a big limbo on this one. The full code to achieve this is:
On the dirty forms integration, I've said before that I believe that would be a different issue, and I'd personally support a more appropriate dirty forms integration, instead of the current "minimal approximation". That is all up to the core committers to decide though, depending on the usability need I guess. So I think we could remove the popup from this issue and keep debating elsewhere.
On the reuse of the overlay, I'm seeing the overlay as a specific implementation of jQuery UI dialogs, so I'd expect people to reuse the jQuery UI dialogs, not the overlay. Any contributions to the patch to make it more reusable are welcome however obviously :) But I'm not sure how would people reuse it and what parts would they want to reuse anyway.
Comment #236
markus_petrux CreditAttribution: markus_petrux commentedOn the reuse of the overlay... for example, instead of:
It could be like this:
And now callers could enable the draggable option in jQuery UO Dialog, for example.
Another thing that could be done to give more power to callers would be to add optional onFoo() callbacks here and there. For example, onChildLoad(), onChildUnload(), etc.
This kind of things will be hard to achieve if not planned ahead, I think. Imagination is endless, and the whole overlay thing is so new (in core itself) that I'm sure there will be a lot of great things from here.
Comment #237
Gábor Hojtsy@markus_petrux: as I've said, any such improvements are welcome in a patch update :)
Comment #238
xmacinfoOMG, this code is still there, then. This is why it was requested to have a dirty form function.
Lets remove this code as well as the warning and lets discuss those in a new issue later.
Comment #239
markus_petrux CreditAttribution: markus_petrux commented@Gábor: Ok, I'll check out HEAD, apply the latest patch in #225, and see if I can post a patch asap.
Comment #240
markus_petrux CreditAttribution: markus_petrux commentedOk, here's a patch. Changes are:
1) Removed the onclick event attached to the background (comment #235).
2) Allow external scripts override jQuery UI Dialog options (comment #236). This is new customDialogOptions option for overlay.
3) Added new overlay option onOverlayOpen. This is a callback invoked when the dialog is opened.
4) Added new overlay option onOverlayCanClose. This is a callback that can be used to allow external scripts decide if the dialog can be closed (Dirty Forms could take advantge of this).
5) Added ability to pass arguments to onOverlayClose callback through overlay_close_dialog() invoked server side. This is a feature of Modal Frame API that was not present here, and this allows external scripts do something when the dialog is closed based on server side response.
I believe I'm not missing any other change I just made.
Aside... one bug I think there is here is in overlay_form_submit(). When this submit callback is invoked, if the dialog close has been enabled, then it will terminate execution, but this short circuits other FAPI submit handlers. In Modal Frame API I was just setting $form_state['redirect'] to FALSE, so that other submit handlers can still be processed, Drupal.settings will be sent when FAPI redraws the form, and child.js will close the dialog. I was afraid to fix this, however, because I'm not sure why it has been changed from what Modal Frame API has.
Another issue related to this change, is that overlay_close_dialog() is tied to form submit handlers, and that means a child window cannot be closed from a simple GET request with no form submit.
Comment #241
Gábor HojtsyThanks for the changes! Your patch included a default.install.orig file. Here is a quick reroll which now lacks that.
Comment #242
Gábor HojtsyAnother patch update now with actual bugfixes included. I worked in my fixes to markus' update, so this patch includes all latest changes. Fixed added for the two problems identified by Bojhan:
- Tinkered a lot more with the positioning of the tabs on the overlay; looks to be consistently working in Firefox 3.5 (Mac) and Safari 4 (Mac). Obviously tests in other browsers are welcome.
- Fixed the issue with resizing of the background dim. It did not take the window size into account and always kept the existing dim background size as a maximum (when the overlay shrank, that never "won"). Now takes into account the page body size and the window size when calculating, so it now shrinks the background.
This is how the tabs align on my machine in both browsers I've tested with:
I'd also like to note that Firefox 3.5 seems to exercise my CPU in an amazing way when the overlay is on. Even if I don't do anything just leave the overlay open. This does not happen in Safari. Since others did not note it above, I'd assume it might be some local issue (one of my Firefox extensions maybe interfering).
Comment #243
leisareichelt CreditAttribution: leisareichelt commentedGabor, just confirming our conversations elsewhere re: clicking on background to close overlay window.
The reason we advised against this (ref: #45 and Bojhan #51) is because it is very easy to make an error and to close your window accidentally and it is very difficult to recover from this error.
I think we should avoid having click on background to close and instead we should address the usability of the close button - if it is not being found then we need to ensure that it is designed in such a way that it is more noticeable/accessible as you scroll down.
Also, I don't think it is a safe assumption that people know when they close a window that their data is unsaved - in these days of autosave, I don't think that is entirely true at all, and certainly in the scenario where people have been creating article content etc. we really want to make sure that people are deliberately NOT saving, and assuming that they will want to (in most cases) preserve their content.
Comment #244
catchThe problem with the prompt on closing is pages like admin/content which are technically a form (you can delete / publish stuff from there), but you're never actually entering any data, and a lot of the time will be using it for search/filter and going to articles directly rather than using the bulk update options at all. On those pages, we definitely don't want a message when you close - a lot of people won't even realise it's a 'form' and could get very confusing.
Comment #245
Gábor HojtsyOk, warning and skipWarning code removed. We can revisit this in a follow up patch and hopefully add a more proper dirty forms type of check where we can exclude certain forms and only warn if there was something entered. That would basically be the integration of the above mentioned dirty forms module. Let's open that can of worms in a follow up issue then.
Attached patch removes the warning.
Comment #246
markus_petrux CreditAttribution: markus_petrux commentedLet quote myself from comment #240:
Maybe this could be addressed with this version of
overlay_form_submit()
:And then we don't need those implementation of hook_node_insert() and hook_node_update().
[EDIT] since we have the chance to touch core here, another possible way to resolve this issue would be to add a new #after_submit callback to FAPI so that it is invoked after all submit handlers have been invoked.
Comment #247
Gábor HojtsyMarkus: I don't know of a specific reason for not letting the rest of the submit handlers act, so it sounds like a bug indeed. Your suggestion code seems to be a better approach. Again, feel free to intervene ;)
Comment #248
markus_petrux CreditAttribution: markus_petrux commentedOk, so here's patch in #245 updated with a slight variation of the code in comment #246. Implementation of hook_node_insert() and hook_node_update() are now gone, and this is resolved inside overlay_form_submit( ) itself.
Also fixed wrong condition related to onOverlayCanClose:
Comment #249
markus_petrux CreditAttribution: markus_petrux commentedOops! I attached a wrong version of my changes. Sorry.
Comment #250
Gábor HojtsySuperb, let's get those test results flowing again!
Comment #251
sun#583400: jQuery UI libraries are loading too much CSS that needs to be reverted / Allow jQuery UI themes to be swappable should fix the jQuery UI theme issue.
Comment #252
sunI can't see why #583400: jQuery UI libraries are loading too much CSS that needs to be reverted / Allow jQuery UI themes to be swappable doesn't make it work. The idea is to use the APIs of the stuff you want to use. Just like in Drupal.
Revamped.
However, this is still very far from being ready.
Comment #253
Gábor HojtsyThe patch suggested there still adds unused/useless CSS files which we might need to override (I did not actually test it yet).
Comment #254
sunLike the friendly jQuery UI core developers explained over there, jQuery UI is supposed to be used that way. Removing the ui.theme.css removes the theme. The remaining styles are for base functionality only. This patch tries to re-invent the wheel by using own base styles.
That needs to be redone, but that's also minor compared to the real issues.
For starters:
1) Overlay is Overlay. No more and no less.
2) Everything with "toolbar" in it has to move into toolbar module. toolbar.js, to be more concrete.
3) Toolbar module triggers (resp. implements support for) Overlay. Only within the toolbar, to be precise.
4) What remains in Overlay is the dialog/parent handling as well as the child handling. All strings and descriptions referring to "administration" need to vanish. Overlay is not limited to administration.
Comment #255
Damien Tournoud CreditAttribution: Damien Tournoud commentedI don't get what this all ui.theme.css debate is all about. jQuery adds a "overlay" class to the dialog because we are using
dialogClass: 'overlay'
. It's simple enough to override the specific directives we need to make the overlay a "special dialog".Comment #256
RobLoachA little better, and you can see how drupal_add_library can help.....
This touch isn't needed, is it?
Do we have to add a class to the node module here? Not every Drupal site out there will be using the Overlay module, nor need this overlay-escape class.
we could probably once() this.... $('body').once('overlay', function() {
// Other code here.
});
With this, if you want to display any jQuery UI element, you have to manually add ui.theme.css, which will in turn break the Overlay anyway. We should be working with jQuery UI's CSS as opposed to against it.
Should this be in one of the overlay's CSS files?
............................... Here's another patch that uses ui.theme.css, but builds ontop of it. It's not perfect, but it doesn't require other jQuery UI elements to register their own theme each time.
Comment #257
RobLoachUhh, wrong screenshot. Sorry... Here it is!
Comment #259
markus_petrux CreditAttribution: markus_petrux commentedIn regards to the following:
This is executed when we do not know yet if we have a parent window with a Drupal.overlay object. Would it be possible to add the isObject() method to drupal.js? (it would have to be in jQuery itself, but it isn't).
Comment #260
Gábor Hojtsy@Rob Loach: Charlie added that locale fix. For some reason the locale module is broken with the overlay patch applied (I'll send over virtual hugs to people who find that out why). So your patch fails on that locale test now. I've tried to remove that above seeing it was superfluous, but see above.
@Damien Tournoud: the issue is that adding whole libraries adds CSS which will never get used and needs to be overwritten. While we do have the specific classes we need, jQuery UI's generic classes are also there and matched from their code.
@Damien Tournoud, @sun, @Rob Loach: So we include twice as much CSS and make debugging that harder in the name of reusing our APIs, right? Well, if that is the *correct* way, I'm not gonna go against you helping us fix this up! However, in that lite and given Rob's comment above, #583400: jQuery UI libraries are loading too much CSS that needs to be reverted / Allow jQuery UI themes to be swappable seems to be a no-go, since removing that will cause problems with others adding jQuery UI components to the page for other reasons.
Comment #261
Damien Tournoud CreditAttribution: Damien Tournoud commented@Gábor Hojtsy: you could argue that we need to design a Drupal-specific
ui.theme.css
, but simply not including the default jQuery UI css just for the sake of saving a few bytes makes very little sense to me.[Edit] example of what I mean: what if the base
ui.theme.css
is already included, because another UI element is used in the same page? Your partial CSS file will brake horribly.Comment #262
markus_petrux CreditAttribution: markus_petrux commentedIn response to sun at #254: I agree that overlay could be a module that implements an API, then other modules can make use of it. All the stuff related to integration of overlay in Drupal administration could be a separate module that depends on overlay.
This is also a method to ensure overlay api is well designed to cover different use-cases, at least those that are implemented for core would work.
It is now difficult to see if overlay api can be reused elsewhere.
Comment #263
cwgordon7 CreditAttribution: cwgordon7 commented@Gabor/Rob Loach: As I understand it, the locale test fails without that change because the overlay module adds a few additional Drupal.t() calls which adds strings to the "to be translated" string list which stores the strings in the database by lid. The additional Drupal.t() calls bumps the locale test's test string's lid into the double digits, which exposes a preexisting bug in the test's regular expressions, which only captures the first digit of the lid from the URL. Thus, with this patch, the test needs to be corrected in order to prevent the failure from occurring.
Comment #264
sunI'm not sure on what Rob Loach's last patch was based, but it had some strange changes in it.
This one is based in #252, cleaning up a lot of stuff.
Next step is to move away Toolbar-stuff into Toolbar module.
Comment #265
RobLoachLooking through my patch, it seems that I forgot to fully clean the development environment, whoops.
With the new patch, you need to add back
ui.theme.css
and then stick this into overlay-parent.css:We need to work with ui.theme.css as if we add any other jQuery UI element along with its CSS, it would break the overlay. We have to build ontop of it.
Comment #266
bensnyder CreditAttribution: bensnyder commentedfinally subscribing after 1 month of following.
I'm glad sun laid out some direction in #254... I was a little lost as to how far along this issue was.
Keep up the good work fellas! Looks incredible!
Comment #267
Gábor Hojtsy@sun: thanks for working on the patch. Here is my testing feedback:
- it does not work in Safari; the symptom is that the loading screen keeps being displayed; this did not happen with any previous patch I've tested
- it has minor style glitches in Firefox 3.5 (where it does work), maybe not target of immediate fixing while your clean up APIs, but noting just in case:
For the spacing designed for breadcrumbs / top of overlay, see the image above in #107: http://drupal.org/node/517688#comment-1972556.
Otherwise seems to work and look as before on Firefox.
Comment #268
RobLoachThis patch builds off of
ui.theme.css
so that the Overlay doesn't break when other jQuery UI elements are active.Comment #269
Gábor HojtsyI'd second that #574156: Overlay: submitting a node should close the overlay and friends will need a rapid resolution once this patch is perfected and committed. Watching test videos from http://vimeo.com/groups/drupalux/videos prove that the node showing up in the overlay can be very confusing. People go back and forth between viewing and editing the node in the overlay and on the website UI. We've had a solution for that above for quite some time but it was shot down in the name of API cleanliness without a better solution in sight, and moved to a side issue to fix later. People doing user testing with this patch will obviously test the somewhat broken user experience in very core things like creating a node. That does not make it easy to project the results to a place where we have these fixed workflows in place. Tossing the fixes for those even later will get us even less user testing of the actual user experience we aspire to achieve.
Also, multiple user tests proved perfectly that the overlay closure warning is a huge annoyance even after a few minute's use, so let's not argue about putting that back in :)
Comment #270
Dries CreditAttribution: Dries commentedI tested the patch and, based on limited testing, it seems to work well. Haven't reviewed the code yet.
Comment #271
Gábor HojtsyThe blocks page theme switching WTF which was said to be a major problem for the overlay now has a patch at http://drupal.org/node/581118#comment-2106588
Comment #272
Anonymous (not verified) CreditAttribution: Anonymous commentedSubscribe
Comment #273
catchPlease put this in an overlay_form_alter() instead of hard-coding markup into node.module which will be output regardless of whether overlay.module is enabled or not - this may or may not be helped by #595982: Use #tableselect instead of ugly theming on admin users and admin nodes.
This was mentioned in sun's review in #47 and Rob Loach's review n #256 but doesn't seem to have been changed by the patch.
Comment #274
Anonymous (not verified) CreditAttribution: Anonymous commentedSince the output of l() is being put into the form elements, it seems like it would be a much cleaner task to simply surround the code referenced in #273 with if (module_exists('overlay')) { }... Thus that's what I did. If that is not acceptable, I'll try to go about it another way.
Comment #275
ksenzeeIntroducing overlay code into node.module is pretty tight coupling. If it's too unwieldy to handle it with a form_alter as catch suggested, could we just add the overlay-escape class with Javascript?
Comment #276
Anonymous (not verified) CreditAttribution: Anonymous commentedI don't really think that would be a good approach.
Since the rendered link is being added to the form via l() I only see two approaches that form_alter could take:
1) Copy the entire section of code that generates the links, making the one small change. This goes to create code duplication.
2) Use a regex to add 'overlay-escape' to the links. That sounds like it would be serious overkill. Regexs aren't exactly notorious for a good developer experience nor for speed.
As far as the javascript aspect, I'm not sure if it would work. If it did, however, adding a class to only one set of links on one certain page... well, that seems to have the same downfalls as the regex. I can't see a reason to use lots of code and sacrifice speed to remove one single line of code. Not to mention there is always the possibility that the user would click on the link before the page had finished loading. (So the javascript wouldn't have run yet.)
Honestly, I think that #274 and #268 both work acceptably well. I just don't see any other way to remove that one bit of coupling without greatly increasing code complexity.
Comment #277
chx CreditAttribution: chx commentedThe patch is rolled without -p so I do not even know which function is being changed with that coupling.
Comment #278
chx CreditAttribution: chx commentedOK, so right now we are at the point where we want to add a class regardless of overlay is installed or not, maybe there will be an awesome_overlay module who knwows. Then what the class indicates? admin exit. Now, let's find a class name... give us a few minutes.
Comment #279
David_Rothstein CreditAttribution: David_Rothstein commentedAll these proposed solutions have a major problem: They are one-off solutions that only apply to this particular form. Whereas on a typical Drupal site, there might very well be other content listings displayed in the overlay as well. So we'd be introducing a weird inconsistency to only close the overlay for this one listing (or alternatively, a weird requirement to ask every contrib module to think about duplicating the solution)....
More important question, though: Why do we want to do this at all? I tried this patch, clicked on "Content" in the toolbar, and it opens the overlay and shows me the content listing. OK, so far, so good. But since it's a content listing, my first inclination is to browse through some of the things there. So I click on the first piece of content I see. What happens? Suddenly, I am thrown out of the overlay without warning and dumped onto a different page altogether. Very disorienting. OK, now I'm done looking at this piece of content and want to go back to my list and browse through some of the other items. So I hit my browser's back button, and it doesn't even take me back there. I'm a bit stuck.
I suggest we simply don't do that at all, and then the above implementation problems are neatly avoided :) Closing the overlay without the user hitting the close button is something that should happen only very very rarely, when there's a clear, deliberate reason to do so.
Comment #280
seutje CreditAttribution: seutje commentedlike viewing a node after it was created?
and would that also apply to editing a node?
imo, it makes a lot more sense to be thrown out of the overlay and onto the page the node lives at, when submitting the form. And I think that can be done trough the submit handler, no?
Comment #281
casey CreditAttribution: casey commented@#279
Another option: show a confirmation message in the overlay that the node is created and give the user a list of actions; view node (close overlay), create new node (stay in overlay), ...
Comment #282
marcvangend@#281: IMHO confirmation screens that don't do anything else (except redirecting) are not really user friendly. In that case I'd rather present those options on the bottom of the node form. For instance, you could have multiple buttons, like this:
|Save| |Save & close| |Save & add new|
Another option would be to have just one 'save' button and let the user choose the after-save-behavior with radio buttons:
Comment #283
markus_petrux CreditAttribution: markus_petrux commentedIt seems to me all these matters would be easier to approach if there was a module that provides the overlay as an API, and then... a) another module that uses the Overlay API to extend/enhance core modules, or b) these features are implemented in core modules themselves depending on whether the Overlay API module is enabled. I would say a) would be better because there would be no need to mess with overlay *inside* the other core module, and it would also easily show how can the Overlay API can be used in contrib/custom module directly, or to extend other modules.
Comment #284
Anonymous (not verified) CreditAttribution: Anonymous commented@#279: That actually sounds like a much better solution. I'll see if I can roll out a patch later today that fixes that.
@#282: I agree with your take on confirmation screens. It's probably safe to leave closing the overlay to the close button and assume that user wants to stay there unless they choose to close it.
@#283: If we want to actually get this in to D7 (and this thread not be a waste of time) then I think we'll have to skip on going with an API. I think that would require too much rewriting.
Comment #285
Gábor Hojtsy@JoshuaRogers: I'm not agreeing with you that we should only close the overlay on hitting the close button. When a user clicks on a node link or submits a node, we should not get to the actual rendering of the node *in the overlay*. The overlay is supposed to be used for administrative interactions, not viewing content. The admin theme will most probably not have provisions (theme functions, images, styles) to display the node as intended in the admin overlay. (The same causes previews being a failure in the overlay as noted in the issue starter).
Comment #286
David_Rothstein CreditAttribution: David_Rothstein commentedLet's separate the discussion of creating content vs viewing content. My comment in #279 was only about viewing content -- and the specific case of the node listing at admin/content that is causing so much trouble.
I agree that creating content is probably a special case where automatically closing the overlay does make sense. However, the code required for that would be completely different anyway.
I think that is going to be impossible to enforce without rewriting all of Drupal. There is no such clear separation, and there are cross-links all over the place.
Right, I was thinking about that as well. Can we show the main site theme in the overlay if a node is being viewed there (but only the main content part of the page - not the other blocks)? This does get very tricky.
Comment #287
Anonymous (not verified) CreditAttribution: Anonymous commented@#277: The attached patch has been rerolled to with -p
Comment #288
sunThis patch won't fly if Toolbar-stuff isn't separated away into Toolbar module and Overlay becomes a plain Overlay module, without any technical limitation for "administrative stuff".
Achieving this won't be hard at all. I'll try to get my hands dirty with that ASAP. Dries already asked, but I'm still overly busy with mission critical API clean-ups.
Comment #289
Gábor HojtsyRight. Modules need to be aware of a possible overlay and need to tell the overlay how to work with the content, if special handling is required. Just like modules are aware of a possible edit mode, and provide data which might not be displayed at all (ref: the d7ux attached links patch). The fact that overlay is optional might make it look like we are adding cruft to modules which will not get used when the overlay is not used, but this same "collaboration pattern" is used in contrib all the time. Optional support for certain modules is all over the place.
The defining principle of the overlay is that it separates administration from the main site. This will not work for all sites, and not all sites need to use the overlay obviously. But if we disregard this defining principle and start to display whatever we happen to have in the overlay, it is gonna hurt more then help. We don't need to make this work for every imaginable use case in the world. You know Drupal has this **modularity** which comes in very handy, when you need something which is not implemented in core the way you want it to be. Be it a drop-down admin menu you'd want to see, a different logging module, or a replacement for the (admin) theme or a different admin UI wrapper.
I'm getting that @sun et. al. take API-ification of the overlay as an important step, and we'll see what modules and sites will make use of that and do non-administrative stuff with the overlay. However, if that looks the same as the admin interactions, as explained above, it could confuse more then help the user. Let's at least try to get the core implementation of the overlay reflect our goals with it. If overlay treatment in node module or user.module is a no-go (as in "code for an optional core module should not be in there") in terms of responsibilities of modules, then the overlay needs to alter in these places, and alter node listings, user listing, etc. If that is not yet possible, it is a good exercise anyway to make those places alterable. That would be for the benefit of any contrib module as well.
Comment #290
David_Rothstein CreditAttribution: David_Rothstein commentedI wouldn't disagree with that so much as saying that "administration" is hard to define. Viewing content can definitely be an administrative activity - what I tried to do in #279 (basically, browse through content from an administrative screen) is I think pretty common.
I would suggest that the defining feature of the overlay is really that it focuses you on a task that you went there to do, and when you are done that task, you go back to where you were before. So if I get thrown out of the overlay at random times and the parent page gets redirected somewhere else, that seems to really interrupt that flow.
I think if other people tried out something similar to what I did in #279, that would be helpful. I am only one person :)
Comment #291
Noyz CreditAttribution: Noyz commentedComing in late to this, but my opinion is to close the overlay. I agree with Gabor in #285 - the reason for the overlay was to try to enforce the separation of administering a site. Sounds like this might not be impossible in all cases, but we should adhere to the pattern wherever possible. Additionally, I think we should assume that if content is clicked, it's clicked for a reason - albeit to view it, or edit it. If the content stays in the overlay - viewing it would be tainted by Seven (constrained by width), and editing it would be impossible.
Back button not working stinks, but getting back to your list of content is only one click away "find content"
Comment #292
Linea CreditAttribution: Linea commentedsubscribe
Comment #293
Anonymous (not verified) CreditAttribution: Anonymous commentedIn that case, how does #287 look?
Comment #294
David_Rothstein CreditAttribution: David_Rothstein commentedIt would? Why?
That page is not the only place where content is listed. Think of something like a View, which might be deep within the admin structure and therefore not possible to get back to via a single click.
Also, somewhat minor point: I believe it's the case (but can't confirm since this part of core seems to be broken right now) that if you had sorted your content on the "find content" page, drop out of the overlay, and then go back by clicking on that link - it wouldn't remember your sorting and you'd have to redo it... but that might just be a bug to fix.
Comment #295
Noyz CreditAttribution: Noyz commentedEditing would be impossible in that editing content is supposed to be done in place. That model kind of breaks down if you're in the overlay.
Sorry, I thought we're talking about looking at your content via /admin/content/node, and then clicking on a piece of content to view it? Looking at your listing of Views, clicking on the View, then not being able to get back to the view would take longer yes.
I still think closing the overlay is the correct thing to do. Comparing evils, it's much worse to pull the content into the overlay, possibly have a theme within a theme within a theme, and jeopardize the very thing the overlay is meant to do - separate admin from content.
Comment #296
ksenzeeI agree with Noyz -- it's horribly messy to start displaying content within the overlay. I also agree with David that after you view a piece of content, you need to be able to get back to your last overlay state. If we can find a solution that satisfies both concerns, perfect. If not, I think losing the overlay state is the lesser of two evils.
I can think of two solutions that preserve overlay state, while still letting us close the overlay when viewing content:
1) Keep track of the last page viewed in the overlay, and provide some kind of button or link that says "Reopen overlay." This could even look like a rolled-up or minimized version of the overlay.
2) Make it so that after you've viewed the content, the back button reopens the overlay at the last overlay state.
I ran these by Noyz offline, since he's an actual interaction designer, unlike me. He didn't care for #1. So I investigated solution #2, and I believe it's doable. We could do what Gmail does and change all the overlay-enabled links to fragments. So if you're on the homepage, and you have a link to /admin/content that's meant to open in the overlay, we'd stick a # in there, and your link would be to "http://example.com/#admin/content". Clicking on that link would not trigger a page reload, but it would create an entry in the browser history. We could then set it up so that any link with a fragment that matched an actual menu path would open the overlay for that path.
The downside here is that the URLs might be a bit ugly. An overlay of the Find content page, opened from a node view page, would look like "http://example.com/node/4#admin/content". Thoughts?
Meanwhile, I've rerolled the patch without .orig and .rej files, and made a couple of minor typo-level fixes.
Comment #297
JacobSingh CreditAttribution: JacobSingh commentedOkay, did a re-roll due to the massive API change to all theme functions and the move to tableselect.
Comment #298
Anonymous (not verified) CreditAttribution: Anonymous commentedI've been trying to bust this bug, but no success so far: If I attempt to open the overlay directly after installing D7, I get a "page not found" every time. Clicking on the dashboard button again brings up the actual dashboard. I can only get it to happen if I click the dashboard directly after installing. Granted, it's not a severe bug. Just kind of annoying though.
Comment #299
David_Rothstein CreditAttribution: David_Rothstein commented@JoshuaRogers: I think that might be a separate bug, likely related to this: http://drupal.org/node/557542#comment-2120098
Comment #300
Anonymous (not verified) CreditAttribution: Anonymous commented@#296: It makes sense to me. I don't think the links have to be pretty if they are meant to open in the overlay. It seems like a reasonable price to pay.
I'm not sure if that is reassuring or not. (I'll pretend that it is for now.) I'll try to get some work done on implementing #296 part 2 this weekend.
Comment #301
Anonymous (not verified) CreditAttribution: Anonymous commented@#296: Thinking about it a little bit more, I have some (probably obvious) questions. Still, I'd like to figure it out before trying to code.
1) Suppose it open "admin/content" in the overlay and that I go to "admin/comments" from there. Next, assume that I close the overlay. Would the browser history list http://example.com/node/4#admin/content as the most recent place in the history?
2) Assume that we're on a multipage form or that we are looking at a list with a particular filter and order: (Unpublished pages listed by author, for instance.) If we close the overlay and then choose back to reopen it, what would happen? (For that matter, any page that has POST content.)
3) On the node add form, if the user closed and then went back, would they be presented with a blank node add form?
Not trying to be stubborn. Just trying to find any issues before coding (in hopes to find a way around them in advance.)
Comment #302
ksenzee1) Suppose it open "admin/content" in the overlay and that I go to "admin/comments" from there. Next, assume that I close the overlay. Would the browser history list http://example.com/node/4#admin/content as the most recent place in the history?
Ideally the fragment -- and thus the URL -- would change with every click, so no.
2) Assume that we're on a multipage form or that we are looking at a list with a particular filter and order: (Unpublished pages listed by author, for instance.) If we close the overlay and then choose back to reopen it, what would happen? (For that matter, any page that has POST content.)
I think POST content is gone when you reopen the overlay. Ideally content listings would set a cookie. I think.
3) On the node add form, if the user closed and then went back, would they be presented with a blank node add form?
Yes, but they'd have gotten a dirty form warning before they closed, so caveat form-filler-outer.
If anyone objects to sticking fragments in the URL to fix the back button, please speak now before we spend too much time working on this....
Comment #303
Anonymous (not verified) CreditAttribution: Anonymous commentedIt sounds good to me.
Comment #304
David_Rothstein CreditAttribution: David_Rothstein commentedFragments in the URL make sense to me also.
If I'm not mistaken, with an extra tweak or two this would also make it possible to link to overlay pages or bookmark them in your browser? If so, that's a huge win.
Comment #305
sunIn case you didn't notice: We just crossed the #300 mark.
To get this done:
1) Remove all lines relating to Toolbar from modules/overlay/* and insert them appropriately into modules/toolbar/*
2) Forget about fragments, and whatever dream you have about awesome UX experience. Such features should have been discussed in the first 100 follow-ups. If at all, follow-up issue, or, welcome to Drupal 8.
3) Stop discussing. Let's do it now.
Comment #306
Anonymous (not verified) CreditAttribution: Anonymous commentedPlus, it should allow for any links in the overlay that are opened into new tabs to also appear in overlays (not just as the content that should appear in it.
Comment #307
markus_petrux CreditAttribution: markus_petrux commented@JoshuaRogers in #284: "@#283: If we want to actually get this in to D7 (and this thread not be a waste of time) then I think we'll have to skip on going with an API. I think that would require too much rewriting."
Well, if Overlay in D7 is not implemented as an API, it will be hard to reuse, and so I'll have to create a version of Modal Frame API for D7. If reusability it is not provided by Overlay in core, then Overlay in core is just void to a few modules that exist in contrib as well as in custom code.
Comment #308
marcvangendLet's not forget the reason why we started on the overlay module. The main purpose of Overlay is not to make other modules (Popups API, Modal Frame API, Lightbox2, CTools' modal dialog or whatever else) redundant. Even though it would be really nice if we get a flexible API out of this, our focus should be on UX. Quoting Leisa Reichelt on http://www.d7ux.org/d7ux-initial-concepts-direction/:
Comment #309
Anonymous (not verified) CreditAttribution: Anonymous commented@sun: "Forget about fragments, and whatever dream you have about awesome UX experience." I'd say that having a good UX is worth a little discussion. Yes, we're over #300 posts, but how many of those were "+1", "Subscribe" or TestBot crying itself to sleep? No reason to get testy. (Well, other than the pressure that comes with the impending doom of freeze.) ;)
Having said that, I agree that we need to get this completed. I'll be working on your #1 and seeing if I can't get something done with fragments this weekend. We are low on time, but there might still be enough to get good a UX and good design. I'll see what I can't get done this weekend.
Comment #310
Bojhan CreditAttribution: Bojhan commentedSo this is my fifth review or so, and I am surprised to still see the same bugs from review 1. I think most critical issues should be solvable, yet with only 4 days to get this sucker in I think almost none of them are truly crazy UX issues - that can't be fixed with follow ups. I think what I am mostly concerned with is that all the tiny details we still haven't fixed, add up to a "still work in progress" feeling.
Smaller issues
Comment #311
Anonymous (not verified) CreditAttribution: Anonymous commentedI plan on working on this solid up until the 15th. I'll see if I can't try to take out as much of #310 as possible while I'm at it.
Comment #312
Anonymous (not verified) CreditAttribution: Anonymous commentedThis patch should take care of #305-1 as much as reasonably possible.
Next I will work on the issues #310 and see if the fragment thing could end up working.
Comment #313
Gábor HojtsyOn fragments: simply appending the path as a fragment sounds limiting as to how other modules can use the fragment. That would lock down using fragments for overlay only. Also, fragments like #admin would not fly well, when you can actually have an id='admin' element on the page. This can happen to any single level path at least. If we consider we only display admin paths in the overlay ever (which I'm a big promoter of), then we might only have this issue strictly with the admin word. Also, Drupal paths (admin filters, etc) can usually also take arbitrary GET parameters. How would those represented in the fragment? I'd agree that this sounds a bit late to implement. The in-overlay back button experience works due to the iframe we deliberately choose (for this among other reasons).
@Bojhan: your rendering error on scrolling screenshot seems to point to the horizontal scrollbar on appearance; I was unable to reproduce the horizontal scrollbar or the scrolling bug, you noted earlier and could not find others to reproduce either
@Bojhan: closing overlay on node creation was in the patch for long, but was removed in the name of API purification; I do not agree with this and David and Noyz as well as ksenzee was discussing that lately; same treatment should happen to users when created
@Bojhan: blocks page issue already at #581118: Blocks admin user interface should not do theme switching
@Bojhan: alignment of items as well as spacing in the overlay was fixed multiple times but keeps getting broken due to CSS reorganization happening by multiple parties :|
Comment #314
Dries CreditAttribution: Dries commentedSo what is left to do on this issue? After 300 issues, it is hard not to loose track ... a quick summary would be really helpful.
As far as I can see, the one thing that is still up for discussion is the "behavior problem" with regard to breaking out of the overlays (eg. when user clicks on a content link from an overlay, should they be taken out of the overlay or not). Is that correct, or is that considered to be a follow-up patch?
Comment #315
Gábor Hojtsy@Dries:
- @sun discussed in #305, that toolbar related code should go to toolbar, not overlay, and he had no outstanding issues mentioned here; Joshua attempts to accomplish this in his latest patch
- @sun previously mentioned that all "administration" related ties should be removed from the overlay (see next point too, which is ver similar);
- @markus_petrux notes that we should make the overlay an API and a UI, decoupling the two, but many of us noted (including @marcvangend nicely explaining above), that the overlay is not supposed to replace other dialog tools (and we added jQuery UI, which has a nice dialog component for people to reuse, which we actually reuse for our own purposes), so its not like all popup/dialog modules will need to be written from scratch for D7, but that the overlay is a special UI; unlike other dialogs, it grows and scrolls with the page, it fits into most of the screen estate and is designed for multi-page administrative interactions
- @David_Rothstein, myself, @ksenzee, @Noyz and @Bojhan were arguing about closing the overlay when a non-admin endpoint is requested; David suggests not closing, everybody else suggested closing (which would be in line with the plan for the overlay)
- related to this is how node previews and other actions are handled, which was stated as a problem to solve in the initial issue starter, but due to the API and (to lesser extent) basic level interaction questions, we did not even reach to talk about such problems yet
- the back button and "open is new tabs" interaction was also discussed and using fragments was briefly suggested to be able to move about with the overlay opening on certain links even if bookmarked or opened in a new tabs; which would allow the seamless integration of the overlay into the browser UX, which was deemed too late to some extent above
- there are also other relatively smaller issues mentioned above by @Bojhan (and he also covers some of the above)
Looks like we need a definite direction in terms of breaking down the overlay to API and implementation (so we'd have another level of API on top of jQuery UI dialogs, instead of going directly to the overlay) as well as the basic interaction of "outer links" (non-admin page links) in the overlay.
Hopefully this summarized the issues well, let's add more items, if not complete.
Comment #316
David_Rothstein CreditAttribution: David_Rothstein commentedI'm fine with closing if that's what everyone else wants - however, it means the "back button is broken" issue is then critical to solve. The overlay experience is broken if you get taken out of it without warning and have no reasonable way to get back to where you were.
For the name-spacing of fragments, maybe it's ugly, but can't we preface all the fragments with "#overlay-" or something like that? And I could be missing something, but why are GET parameters a problem - can't they just remain GET parameters?
Comment #317
Gábor Hojtsy@David: ok, how would the URL (incl. GET parameters) look when I am on "search?q=Paris" and have an overlay open which displays "admin/people/events?region=Europe".
Comment #318
David_Rothstein CreditAttribution: David_Rothstein commentedWell, ideally something like "search?q=Paris®ion=Europe#admin/people/events" but OK, I see how there are edge cases here.
Comment #319
Gábor Hojtsy@David: then how would we know which query string part is for which page? You have probably anticipated, but we can also play the idea with search?q=Paris with an overlay displaying admin/peeople/events?q=Prague, which easily breaks down.
Comment #320
ksenzeeWe could encode the ?q= bit when it's in the fragment URL. I agree that it might break fragments for other modules though.
Comment #321
eigentor CreditAttribution: eigentor commentedHm the overlay looks strange recently.
No close button...
Also the "meta" tab and the second save button have vanished, and all meta settings are in vertical tabs at the bottom.
What have I missed?
Ack, saving a node opens Garland in the overlay, help!
Comment #322
mcrittenden CreditAttribution: mcrittenden commentedI for one wouldn't be the least bit upset if fragments didn't make it in. It's a pretty good chunk of work with very little payoff. I can't think of a single situation in which I'd miss them if they weren't there.
Comment #323
David_Rothstein CreditAttribution: David_Rothstein commented@mcrittenden: So do you have a different solution for the problem where you click on a link, get taken out of the overlay suddenly, and don't have any clear way to get back to where you were?.....
Comment #324
webchickNote: If you are are interested in testing this patch, make sure to download the images from http://drupal.org/files/issues/images_9.zip and stick them in modules/overlay/images.
I burned 5 minutes looking for these, so trying to save the next person some grief. ;)
Comment #325
Gábor Hojtsy@eigentor: The node UI changes you mention (meta tab, more submit buttons) were never part of this patch and did not get code freeze exception either, so will probably not happen. Issue at #472126: D7UX: Move buttons to right area, add content and meta selector. The close button will only appear if you also copy the image, see Angie's note in #324.
Comment #326
ksenzeeI discussed this with Noyz, David, Joshua and a few others this morning, and we decided the overlay approach is worth trying. We couldn't think of any other way to fix the problem of getting your overlay content back. I'll come back with a patch as soon as I can.
Comment #327
ksenzeeUm, duh, the *fragment* approach.
/me casts about for the nearest caffeine source...
Comment #328
mcrittenden CreditAttribution: mcrittenden commented@David_Rothstein: Yeah, my solution is to not worry about it. IMO an overlay should be kept as an overlay, and not as its own mini site. Yes, I know, I'm probably alone on this one, but every vote counts.
At most, let's make it a follow-up. It's time for this to get in there. (what sun said)
Comment #329
sunoh boy. If you keep on derailing this issue with more and more wishlists and feature requests, then I can assure you that it won't make it into D7.
If the Overlay would provide a proper API that is invoked by Toolbar module, then we'd have a clean and simple thing with sufficient possibilities to implement any special URL fragment or whatever handling in a follow-up patch.
So pretty please, please focus on the initial implementation and get that implementation right.
What we need *now*, is a proper implementation. Any issues and tweaks around CSS styling, images, borders, and whatever can be done after 10/15. Likewise, bugs in JS or CSS can be dealt with after. But all of that can only be done afterwards if there will be a solid implementation in a core-worthy and commit-worthy state on 10/15. The current implementation is not.
Comment #330
ksenzeeFixing the close overlay interaction is neither a wishlist nor a feature request. If it doesn't get fixed, the overlay is fundamentally broken. I have the time and JS ability to handle fragments. I do not have what it takes to reimplement the entire overlay as an API. Anyone who cares to work on it at the same time as I am is welcome to join me on github or ignore my patches entirely.
Comment #331
seutje CreditAttribution: seutje commentedeven if you were to implement fragment history with this initial patch, you might wanna considering implementing one of the perfectly decent existing jquery plug-ins instead of re-inventing the wheel, but I agree that "initial" should be initial and we should build off that
Comment #332
Dries CreditAttribution: Dries commented- I don't think the overlay modules should provide a generic reusable API for all overlay needs in Drupal, but we should have an easy to use API for other modules to use. I agree with @sun in #305 that we should focus on clean separation and APIs first (if we haven't already).
- We should close the overlay when a non-admin endpoint is requested. I agree with ksenzee that this makes for an odd user-experience, but we could classify this as a UX bug that needs fixing after 15/10. We can fix the 'back'-button stuff later.
So where are we from a separation / API point of view?
Comment #333
Gábor HojtsyQuestion is what is the spec for that API.
From the Drupal vs. overlay point of view, modules that wish to open links in the overlay can mark those links with a "to-overlay" class. Links and forms used in the overlay will keep opening in the overlay. If a link is not to be opened in the overlay, modules can use the "overlay-escape" class on the link. If a form submission is to close the overlay, overlay_close_dialog() is to be invoked in the form submission process to pass that information on. The underlying page title is moved over to above the overlay, the tabs are moved over to display on the overlay, these can be manipulated via normal Drupal means (with title and tabs for the page being displayed).
Other then that, the overlay implements a very specific use case of the generic jQuery UI dialogs plugin, in that it overshadows the whole page, is not possible to move around, is not scrolling but expands to scroll with the page, has special treatment for titles and tabs coming from the page, will still need treatment for workflows like node previews temporarily closing the overlay in-between multiple edits of the node, etc. While the opening/closing mechanisms provided also allow opening whatever page in the overlay, making it a generic page displayer on top of your Drupal site, from a UX perspective, it is a very specific user interaction, which was designed for administering your site.
I'm absolutely unsure as to what level of reuse would be advocated in terms of breaking this down to an API and an implementation. It is already an implementation of the generic jQuery UI Dialog plugin for the needs of this UX problem. It is so specific in how it looks and behaves that I don't get the point where it could be broken into an API and implementation above jQuery UI dialogs but below its current overlay functionality. Unfortunately use cases were not enumerated yet on the issue either to help figure out what kind of API would be needed for that at all.
I hope this extended explanation of the matter helps figuring out the answer finally.
Comment #334
sunThe use-case is fairly simple:
Instead of Toolbar module, Administration menu may want to trigger the Overlay. The current code always assumes Toolbar and performs some hard-coded tweaks to make it look right when displayed next to Toolbar.
That has to go, and requires some larger changes in the setup, positioning, and handling of the overlay.
Comment #335
ksenzee@sun: Thank you for the specific use case. I'll tweak the JS and CSS and see if I can't get rid of the remaining hardcoded toolbar stuff. Admin menu and modules like it will still have to have some way of specifying elements that are not to be hidden by the overlay. The best I can come up with is "overlay-displace-top" and "overlay-displace-bottom" classes, which admin menu etc. will have to add to their markup. Other ideas welcome. (Sure would be nice to have hooks in JS... hm...)
Re the question of how to maintain overlay state or fix the back button, Gábor pointed out that we have a blue "ribbon" bar UI element that was never implemented in the edit in place patch -- mostly because it's not actually edit in place. It would work well here as a place to remind the user they're still in an administrative action, and to provide a link for them to reopen the toolbar. It might be preferable to using fragments to fix the back button. Dries, my understanding is that it's okay to consider it a UX bug that we lose overlay state, and to fix it later in a separate issue. Note that we might want to use either URL fragments or this blue ribbon element to solve the problem. If I'm wrong, and we need to fix it today, correct me quickly. :)
Comment #336
xmacinfoPlease open a new issue for the "blue ribbon". :-)
Comment #337
David_Rothstein CreditAttribution: David_Rothstein commentedAnd we don't currently have a clear path forward for the solution.
If any modules don't do that, "non-administrative" content will wind up getting displayed in the overlay sometimes anyway, whether we want it to or not.
I'd suggest that if it's that fundamental, this class be added internally inside the l() function (just like that function currently adds the 'active' class to links now).
To really fix this, we'll probably have to wait until some followup work at #553944: Define hook_menu_get_item_alter() as a reliable hook that runs before the page is doomed is done and committed -- in the end, overlay_init() will probably have to be replaced with something like overlay_custom_theme(). Not something to really worry about now, but writing it down while I remember it.
Comment #338
ksenzeeThe attached takes care of the hardcoded toolbar positioning. There really wasn't much left to change there. @sun, let me know if it's acceptable to you. I'm also including a diff of the changes since #312 to make it easier to review.
I'll open a separate issue for the ribbon and post a patch there soonish.
Comment #339
Gábor HojtsyI'm on testing this patch and working to solve some of the outstanding issues.
Comment #340
Gábor HojtsyThis patch fixes many outstanding bugs:
- restoring the breadcrumbs (and its styling as per the mocks); this was using the new theme hook params badly
- restoring the node form submission close behavior; this was due to element hook changes
- fixed the overlay to work again in Safari; this was broken due to how the links were constructed
- changed the click handler for .to-overlay links to not modify the link href => allows to right-click open in new tab/window, without that opening with the overlay rendering treatment; also fixes the link handler to support having fragments on the links, etc.
- changed the link processor in the child to attach a click handler instead of modifying the links; reused the same code as in the parent, abstracted that out to a method on its own
- tried to fix tab styling (again) using pixel based font, line and margin/padding sizes; works good in Firefox again and only little broken in Safari (the inactive tab is 1px farther then should be on each side)
Also attempted to work on these bugs but could not figure them out:
- $custom_theme is not supported anymore (but code is left in the patch for now to work with it), so the node add form is showing up in Garland in the overlay (eh!); David talks about the follow up work required to make this function in the patch which removed $custom_theme at http://drupal.org/node/553944#comment-2139652 (it's an acknowledged limitation and it does not seem to be possible at the moment to make theme switching work with the overlay)
- the submission callback is called multiple times for some reason in the form handling process, so the node submission does not redirect to the right node path (it keeps being on the same page where you are); this is due to only the second invocation of the callback knowing the redirection properly, but being too late to store that for the JS
- attempted to make the admin user creation form also close the overlay and redirect; that redirect worked (which makes the node redirect suspicious for some local problem), BUT it redirects now to creating another user, not to the user URL (unlike node creation), so we need to think about this more instead of just applying this outright (I did remove this code from the patch, it was just checking $form['#id'] == 'user-register-form' by side of checking the node form)
Existing bugs which I reproduced again but did not look into them:
- in Safari, the overlay shrinks in width on successive page loads to width, where it does not shrink anymore; this does not happen in Firefox; I'd assume it takes the width setting at a wrong time, when animating
- when a tall page was requested and a less tall page follows in the overlay, the background keeps being tall; I've tried to look into this before by using callbacks on the resizing animator, but it did not work out for some reason
Bojhan, I still cannot reproduce your scrolling bug and/or your Appearance page bug.
Handing this off.
Comment #341
seutje CreditAttribution: seutje commented#340:
what the hell? I remember fixing this for FF during the D7UX spring in Utrecht, and it seems to be pretty much the same thing
it gets smaller by 17 pixels every time, which is exactly the width of the scrollbar
but that's obviously more of a follow-up thing
Comment #342
ksenzeeI think everything's now separated cleanly. This patch adds the following:
- All admin links now trigger the overlay, not just links inside the toolbar. I believe this is the correct behavior, but it does seem a little odd on local tasks (node view is in the parent, node edit is in the overlay...). At any rate, there's no longer a need for the "to-overlay" class.
- @David 337.2: Point taken. sun said something similar in IRC, I believe. Which links are "admin" is now defined within Drupal.overlay.isAdminLink(). Modules that need to override the default algorithm for determining an admin link may add a list of links or a regex to Drupal.overlay.links.admin[modulename] or Drupal.overlay.links.notAdmin[modulename]. It's not very performant, although we can add some static caching. Better solutions are welcome (for the next few hours...).
I'm now tracking down bugs (and there's debug code in here). One bug I'm aware of in addition to the ones Gábor pointed out:
- Dashboard's customize feature has a "Done" button (a simple input type=submit that submits to the same page, just as a way to refresh the page) and it reloads the overlay contents, toolbar and all. (yoroy pointed this out: http://skitch.com/yoroy/ndgqk/doubledouble). I'm guessing a JS link that simply refreshed the page would cause the same problem. Still thinking about how to fix this. You know some contrib module is going to do something similar, so I'm not sure changing Dashboard's behavior is a real solution.
Talked to Dries yesterday and he would like the back button to work after the overlay is closed, so it's looking like fragments are the way to go, as opposed to the blue ribbon. I'll work on that after the overlay is otherwise committable.
Comment #343
ksenzeeaargh. my patch numbering always seems to be off...
Comment #344
cweagansCould we possibly get the overlay in without the URL fragments and then get those in later as an 'accessibility bug'?
Comment #345
Gábor Hojtsy@cweagans: I think that is the plan if the fragment based code does not work out today.
Comment #346
Anonymous (not verified) CreditAttribution: Anonymous commented#342: "Better solutions are welcome (for the next few hours...)." Okay, this probably isn't better. It is an idea though. We could add one item to the hook_menu. 'admin' => TRUE or 'admin' => FALSE.
It would allow us to specify any path as admin or not. This could easily be reused by any module that cared whether or not something was considered an "admin item."
Okay. Horrible idea. Don't hurt me!
Comment #347
Dries CreditAttribution: Dries commentedYes, we can get the overlay in without the URL fragments. We can follow-up with the fragments later.
Comment #348
ksenzee@Joshua: I was pretty enthusiastic about this idea, and I mentioned it to Gábor, but he pointed out that every time somebody l()'s to a path we'd have to load its menu item. That's probably a no-go. :/
Yeah, fragments are not happening today. So far all I've had time for is separation of code and bugfixing.
Comment #349
ksenzeeThis includes a fix for the dashboard brokenness, but I fixed it in dashboard.module, not in overlay. :/ Without the ?render=overlay GET parameter, there simply is no way for a page to know it's being loaded in the overlay -- short of setting a session variable, which I wasn't ready to commit to. The fix in dashboard module will work for now.
/node/add links now trigger the overlay as well.
Comment #350
Gábor Hojtsy@ksenzee: Using sessions would come with a few problems, mainly that you could not browse in a non-overlay way parallel to your administration. Your action to use overlay should not have an effect on your parallel browsing experiences for the same site IMHO.
As for the done button, it can very well be a button styled link. I know I experimented with making the customize dashboard button a simple link originally, which would be styled like a button, but it became a link anyway. I know this is not a generic solution, but for these interactions, the modules will really need to be aware of a possible overlay. Just like modules are aware of other possible enhancements like drag table headers or AHAH updates on the page which affect how they write code to blend in with the rest of Drupal.
Comment #351
ksenzeeYeah, I made the button a link and restyled it to look button-y. You're right that other modules will have to be overlay-aware to some degree. And I agree on sessions. I think we could work it so that the session info was detailed enough that your other browsing window wouldn't be affected, but I don't like session in general at all. So I'm happy to take a pass on that one.
Working on the redirect bugs at the moment.
Comment #352
cweagansFound a bug: If enable the overlay module, go away from the modules page, then come back to the modules page and disable the overlay module, the resulting page is still loaded in the overlay.
Comment #353
Anonymous (not verified) CreditAttribution: Anonymous commentedThe patch works excellently for me. :)
Something to think about after initial commit: editing a node and then clicking save, should it really close the overlay, or should it return you to where you were? (In my case, the find content screen.)
Either way, that can definitely be worked on later.
Comment #354
Dries CreditAttribution: Dries commentedI get this error when trying to save something from within the overlay: Fatal error: Cannot use assign-op operators with overloaded objects nor string offsets in ./drupal-cvs/modules/overlay/overlay.module on line 95
Comment #355
Dries CreditAttribution: Dries commentedSome extra small feedback.
'thead' is a typo?
That wraps funny.
It would be good to have a description somewhere of the overlay-parent vs overlay-child separation.
Do we need to add a @todo here as well? $custom_theme no longer exists?
This could use some phpDoc. Just mention that adding this class pushes the overlay down below the toolbar.
... list of submit handler_s_ ?
I think we should better document why we add our own handler, and what that handler does.
Comment #356
xmacinfo"thead" is not a typo :-)
http://www.w3schools.com/tags/html5_thead.asp
Comment #357
ksenzeeAddressed all of Dries's feedback above, except docs on the parent-child distinction, which cweagans is working on. #354 is fixed (boy is it hard to chase HEAD tonight!)
Redirection is still wonky from the node save form. We're adding our submit handler as an after_build callback on both the submit button and the node form itself, which is why it gets called twice. I'm pretty sure we can tweak the conditions under which we call overlay_close_dialog() so that it doesn't get called until we have the right redirect path, but I haven't had time to look at it yet. quicksketch has kindly offered. :)
Comment #358
webchickYoink! Taking this for a spin. *cracks knuckles*
Comment #359
webchick(commit-blocker) I was able to break this right away with the following steps:
1. Click "Dashboard"
2. Click "Customize"
3. Click "Done"
Ta-da!
(note: ignore my screwed up shortcut bar; that's from the other patch. I'm speaking here of the recursion.)
However, it's gotten smarter since the last time I triggered this recursion bug since it only works one level deep instead of infinite. :)
(commit-blocker not this patch) When I clicked "log out" I got:
Fatal error: Call to undefined function menu_load_links() in /Users/webchick/Sites/core/modules/shortcut/shortcut.module on line 214
And now my Drupal install is completely hosed! :)
Going to jump back to shortcuts for awhile.
Comment #360
webchickOmg. IGNORE me. PEBCAK error.
Going back to shortcuts anyway though since David needs to get to bed.
Comment #361
Anonymous (not verified) CreditAttribution: Anonymous commentedThis patch should remove the bug where removing the overlay module while in the overlay causes a second toolbar to appear.
Comment #362
markus_petrux CreditAttribution: markus_petrux commentedThe following may generate a javascript error because when it loads, you're not yet sure a parent window exists. This could happen if someone loads a child on new window.
In regards to isObject(), I think a nice place to put this would be in Drupal object, so it can be reused elsewhere.
Comment #363
webchickSo I had every intention of reviewing this patch in detail tonight, but I have officially run out of steam. So just as with the shortcuts patch, I'd like to see how far this can get in ~24 hours. It feels pretty darn close, though it also seems a bit easy to break.
Brief list of weirdness, some of which might be purely fatigue-related. I'll post a more thorough review tomorrow:
- This patch makes node/add and node/X/edit not in the admin theme. Why?!? It's very jarring.
- Even more jarring is when Seven opens up in some overlay panels and Garland in others. Though ksenzee informed me that this is because of a follow-up bug from the hook_menu custom theme patch not being able to set the theme properly.
- At random intervals, I am able to get two copies of the toolbar going: one on the outside, one within. I think it has something to do with being on a page like admin/X/X already and then clicking one of the shortcut bar items.
- The interaction of the overlay when first enabling and disabling the module is a bit whack.
- In FF I was not seeing the close bar, in Safari I was.
Comment #364
Gábor Hojtsy@webchick:
Tried to reproduce this both in Firefox and Safari, but can't. Also, errors in shortcuts are definitely not related to this patch, since shortcut module is neither in code nor in this patch.
@markus_petrux: do you suggest just copying the code to both JS files or set up a common JS loaded before the parent and child.js?
Comment #365
markus_petrux CreditAttribution: markus_petrux commentedRe: "do you suggest just copying the code to both JS files or set up a common JS loaded before the parent and child.js?"
I would implement the isObject() method in drupal.js.
The other 2 methods in child.js that are assigned to the one in parent.js are not really necessary because we can use those defined in the parent window.
For example, we can do this:
instead of:
and the same could be applied to isAdminLink() in child.js
All we need is a version of isObject() method that can be easily reused, here and it would also be nice if it could be reused elsewhere, IMHO.
Comment #366
Gábor HojtsyOk, drupal.js got isObject added. Others are invoked via parent.Drupal.overlay.*.
Tested more based on @webchick's feedback:
David et al. worked hard to clean up theme swicting in Drupal 7, thereby removing any possibility for us to force these pages to be in the admin theme. These two issues are the same. This happened a few days ago, and discussed at http://drupal.org/node/553944#comment-2139652
As I've said above, I cannot reproduce this neither in Firefox, neither in Safari.
I don't see this with Joshua's latest update. The reloaded module page does not try to use the overlay right after being disabled, and when it is enabled, the overlay starts to be used from the page when the next link is clicked. What interaction do you expect here, which is not implemented?
Cannot reproduce. I can reproduce a 1px "distance" of the close button though in Safari. A rounded border is seen where it should not be. That is probably not what you see and is minor and fixable.
Let's continue the reviews! :)
Comment #367
marcvangend@webchick:
I don't really see what you mean with "The interaction of the overlay when first enabling and disabling the module is a bit whack".
However your remark made me think (or maybe you actually meant something like this): after enabling the module, wouldn't it be good for the UX to immediately show the effect? I'd say that redirecting to the home page and re-opening the module page in the overlay is a really convincing way to say "the overlay module has been enabled".
Comment #368
mcrittenden CreditAttribution: mcrittenden commentedA couple things from testing in FF3.5 (on Ubuntu Linux)
Whenever I submit a form inside of the Overlay (ex: save on Modules page or save on Appearance page or Submit on the add node form), I get the following error. Reproduced numerous times...the submit always works correctly but this error shows up inside of the overlay afterwards:
Fatal error: Cannot use assign-op operators with overloaded objects nor string offsets in /var/www/drupal/modules/overlay/overlay.module on line 57
Useless screenshot of that attached. Is that one just me?
Also, whenever I click on something in the toolbar and then in the overlay click to a different tab of that section (example: Configuration and Modules in the toolbar and then the Modules tab, or Appearance and then the Configure tab), the top few pixels gets cut off inside of the overlay. Screenshot attached
Looking really awesome, though. I'm really happy with how fast it loads. Scrolling is a bit painful because the overlay seems to fight with the toolbar (the toolbar goes over top of it for a split second until the toolbar pops up again), but that seems to be a browser thing that we can't really help much. Awesome work, guys.
P.S. How do I put images into these? Input filter strips <img>.
Comment #369
effulgentsia CreditAttribution: effulgentsia commentedsubscribing
Comment #370
ksenzeeI think Gábor is offline now, so I'll pick this back up. I'm pulling what I think is all the remaining feedback together, in increasing order of severity.
I was seeing this last night with earlier versions of the patch, but I think I fixed it. Can anyone reproduce it today?
As Gábor said, these are the same problem, and we really can't fix it in this patch. We can get rid of the variable_set('node_admin_theme', 1) since it's not really related to this module's functionality.
I can't reproduce this either.
This seems minor enough to be a followup.
Can someone with a Mac look into this? If not, I'm assuming we can fix it after commit.
I think seutje is on this.
cweagans pointed out in IRC that this separation is in fact so unclear that you can't just read the patch and write the docs. You have to have your head totally around the patch. So I'll work on this today.
I like marcvangend's idea in #367 about redirecting home and reopening the modules page in the overlay.
I'll install FF3.5 and try to reproduce. It's not happening for me on FF3.0/Ubuntu.
This is still a problem AFAIK, and is why we're getting weirdness on node creation. I'll keep working on it today. I believe I can fix it.
This is a serious problem. I am not even sure how to fix it. It's the same root problem as the dashboard problem I "fixed" in #349, and that I was stewing about in #342. With good reason, as it turns out. When a page (re)loads its content without going through an actual <a> link, a FAPI redirect, or drupal_goto(), Drupal has no way of knowing that it should render the new content without the toolbar. That rendering is controlled via a GET parameter ?render=overlay, which is added via Javascript to all the admin links on a page. The overlay module checks for that parameter on hook_init(). If it's not there, all we get is a regular Drupal page, not a specially formatted overlay child page with no toolbar.
We originally saw this problem in the dashboard, but Batch API causes it too, and so will anything else that redirects without going through drupal_goto() or a FAPI redirect. I'm racking my brain for ways to solve this globally. Solutions welcome.
Comment #371
marcvangend@ksenzee: as discussed in IRC, here's my patch against D7 HEAD + #366, which adds the behavior described in #367. I'm attaching it as a txt file so hopefully I will not confuse the testbot.
Comment #372
ksenzeemarcvangend, you rock! I'm attaching an updated patch with those changes included. Here's what it addresses:
#370/7: The best documentation we had for the parent/child separation was for overlay_mode(), so I clarified and expanded that, and added references to it throughout. Also expanded phpdoc throughout.
#370/8: marcvangend came up with a session-based approach that works great. Could probably stand some people banging on it though (session makes me nervous). Eventually, when we get the back button fixed with fragments, we can get rid of the session variable and simply redirect to example.com/#overlay=admin/config/modules.
#370/11: I couldn't find a way of handling this globally that wasn't a total embarrassing hack. So I resorted to whack-a-mole, and just fixed the redirection method for Batch API, which was the only situation in which I could reproduce the bug. This could also use some people banging on it, in case I missed a spot.
I couldn't reproduce #370/9 on the same browser and platform, so I'd say it's a followup if we run into it after commit.
Remaining todos:
#370/4-6: I think these are minor enough to qualify as followups.
#370/10: Node creation weirdness. I keep putting this off. :) It's the only commit-blocker left, though, as far as I can tell.
Comment #374
ksenzeeHm. There are changes to batch API in the patch, and I would normally assume that's why the testbot is choking on install, but as it happens I can't install vanilla HEAD without Javascript. So I'm at a loss to troubleshoot. I'll let testbot have another whack at it.
At any rate, node creation now redirects correctly. That's the last major issue I was aware of. Reviewers have at it.
Comment #376
marcvangend@#374:
Unfortunately I'm not very good with the testing framework (yet). Even if there is a good reason why the automated tests are failing, that needs to be fixed. I mean, some simpletests would have to be rewritten to match changes in batch API, right? I don't feel capable enough to take the next step, which worries me because we need to keep going and not let it slip out of our hands in the last minute.
Comment #377
Gyt CreditAttribution: Gyt commentedLast patch work well for me. Only problem is broken "Add content" link in shortcut bar within overlay (screenshot attached).
I also attempted to attach patch #374 for new HEAD.
Comment #378
drifter CreditAttribution: drifter commentedsetting to 'needs review' for the bot to have a go at #377
Comment #380
drifter CreditAttribution: drifter commented#377 is seriously broken - overlay.module gets installed in the drupal root instead of /modules/overlay.
Manually installing #374 works OK.
Comment #381
mfer CreditAttribution: mfer commentedAs Drifter points out, #377 is really broken. When I tried to apply it there was a rejected section in dashboard.css and fuzz factor is needed to apply the patch. Also, the overlay module is placed in root directory folder. Only overlay.info is in the overlay module folder.
I think I fixed these two issues. But, drupal head isn't installing for me so I stopped testing here. The attached patch is an untested attempt to fix the fuzz and to put the overlay module in it's folder.
Comment #382
Gyt CreditAttribution: Gyt commentedHm, new patch.
Comment #384
drifter CreditAttribution: drifter commentedSo I'm struggling to re-roll one of the above patches that will pass both manually installing and PIFR. #382 was missing a newline at the end, but even when I fixed it, I couldn't install manually (got some error about MergeQuery not loaded).
Gabor's #366 was the last patch that I remember PIFR passing without a "Failed to install HEAD". So I'm trying to reroll that one. I succeeded installing it, however, the admin overlay just doesn't show. I tried disabling and re-enabling it - is there some setting I need to change also to make it appear?
Anyhow, attaching a rerolled #366 and hoping for the best.
Comment #385
marcvangendDrifter, you re-rolled against #366, but you did not include my addition from #371?
That one is (as requested by ksenzee, who was also working on a patch on that moment) not a complete patch, but a patch against HEAD + #366 so it was easier to merge into ksenzee's patch that became #372.
I propose that we first see how your #384 performs with the test bot (although it bothers me that you say the overlay doesn't show - unfortunately I don't have time to test it right now) and then roll #371 back in.
[edit] Great, the test has already finished, thanks Drifter. Let's roll #371 back in now.[/edit]
Comment #386
drifter CreditAttribution: drifter commentedOK, here we go, one step at a time - Gábor's #366 and marcvangend's #371 rerolled together.
[edit] Overlays are now opening for me - I have a bit of an unusual setup, Apache with VirtualDocumentRoot, so clean URLs are disabled if I don't edit .htaccess. This apparently also breaks the overlay, though it should work with them off. Something to debug later.
Comment #387
drifter CreditAttribution: drifter commentedNow rolling ksenzee's #374 - the one with the Batch API changes.
Comment #389
drifter CreditAttribution: drifter commented#374 reroll failed to install HEAD (as with the original) - though a manual install works for me. Now trying #374 without the batch.inc and form.inc changes, just to see if it would pass.
Comment #390
Gábor HojtsyWe discussed this extensively in IRC with drifter, and I'm trying to roll in some of the improvements we discussed and fixes for issues we found.
Comment #391
Gábor HojtsySo worked on more of the issues @drifter and I identified. The installer overlay autoloading and the isAdminLink function was not in any way compatible with non-clean-URLs, so not surprising it did not work that much for @drifter. Attempted to fix that by looking at the q=param of the URL if a base path is otherwise not available beyond Drupal's root.
Also, fixed a minor issue with Safari close button rendering that I mentioned above.
Finally, worked on fixing some of the issues with "Add to shortcuts" links. They showed up in the page body at a very obscure place in the iframe, but were designed to show up by the title.
Now fixed to show up by the title (even if not yet perfectly aligned). This also gets them apply the shortcut link theming from the parent page, so the plus button and the description works as expected. Shortcut specific code here is not nice, but I had no better idea on how could we implement the specific spec here.
However, shortcutting actual pages in the overlay does not work due to an integration bug I've noted weeks ago on the shortcuts issue. We get "render=overlay" on the shortcut link due to the overlay having that query string argument. The plus button also got a target="overlay-element", so that results appear in the overlay. Anyway, basically, shortcutting anything in the overlay is going to be broken. Even more so, shortcutting anything, which has a query string is broken, because the whole path + query string is stored. So even if we'd special case the "render=overlay" item to be automatically removed, it would still stop you from being able to shortcut any page with query string arguments (pagers, filters, etc).
Worked off of #387 (including the form.inc and batch.inc changes), so let's see what is broken now.
Also note that I found an easy and perplexing way to get Garland and the toolbar in the overlay. Just go to "Add content" and try to shortcut that. You get a 403 page (amazingly) and Garland and the toolbar. You can shortcut many other pages (eg. Find content - with the above bug of that shortcut not being actually useful), but not this one.
Unassigning in the interest of hopefully getting others to look at this again, not that I do not intend to work on it more.
Comment #393
dmitrig01 CreditAttribution: dmitrig01 commentedsomething isn't a great variable name. how about item or object?
what's the onbeforeunload callback, and why does it have any business in core?
Other than that, looks good. I hvae no idea why the testbot is failing it.
Comment #395
dmitrig01 CreditAttribution: dmitrig01 commentedAlso, the add to shortcuts link still appeared in the body and it doesn't work.
[Edit: the link being generated is something liek: http://localhost/head/admin/config/system/shortcut/shortcut-set-1/add-li...
Comment #396
Gábor Hojtsy@dmitrig01: did you try my latest patch and read my comments on shortcuts there?
Comment #397
boombatower CreditAttribution: boombatower commentedPer request from Gábor Hojtsy I attempted to look into the bot not being able to install the patch...
Applied patch locally and was not able to install with javascript disabled which is most likely why the bot cannot. It only install 36 tables (38 with js) and then after the profile screen I get a white screen of death with (also with js on so maybe head just broke):
Comment #398
boombatower CreditAttribution: boombatower commentedOk, last comment is outdated. Confirmed the bot sees the config page unlike the rest of us.
This is due to the URLs they end up on.
Firefox ends up with batch API parameters on the end of the url...the bot does not. If you install manually and then remove them...wala.
A look at what the bot sees: http://imagebin.ca/view/C_k5UnM.html.
I am thus assuming:
1) simpletest browser does not interpret the redirect right and FF does, 2) the batch API is broken
Comment #399
ksenzeeSo we have a couple of major issues left.
The first is that our fixes to Batch API seem to break something in the installation process. That's been really hard to debug, since installation works fine with Javascript on (either with or without this patch), and without Javascript, installing HEAD is completely broken (with or without this patch). In #398, boombatower narrowed in on why testbot can install HEAD with JS disabled but other browsers can't. I've opened an issue for that at #608826: Install breaks without Javascript enabled if you have has_js cookie set.
The other issue is that shortcuts and overlays do not play nicely together. I'll reproduce Gabor's summary from #391:
I'm working on the shortcut issue, but if someone else were to tackle Batch API it would be lovely. I *think* it's committable without the Batch API fix, but it sure would be nice to get it all wrapped up today.
Comment #400
David_Rothstein CreditAttribution: David_Rothstein commentedOK, I believe this fixes the batch API testbot problem, but we won't know for sure until we let the testbot run it, so let's see what happens...
As can be seen from the interdiff, this was a one line fix - but it took an embarrassing amount of time to actually find :)
Comment #401
ksenzeeYay! Nothing like having people around to fix your stupid mistakes. :/ Thanks a bunch David. Here's an updated version that fixes the menu_get_item weirdness, so you can add shortcuts correctly.
Comment #402
sun1) Overlay is displayed ("replacing" the original page) when installing it manually.
2) (bogus)
3) Fieldsets, JS, and scrolling in general is horribly slow within the Overlay.
4) Firefox currently eats 50% of my computers resources.
5) The browser freeze is reproducable. Firefox bumps to 100% (one processor, or 50% multi-threaded) as soon as an Overlay is opened. CPU decreases to normal after closing. (FF 3.5 on Windows 2003 (XP)).
6) (bogus)
7) When disabling Toolbar module from within the Overlay, it is still there. WTF? ;)
Comment #403
webchick4) and 5) seem to be a Windows-only situation, since only Bojhan and sun so far have reported it. Bojhan's on FF 3.5 (also tried 3.1) and Vista.
Comment #404
sunThis needs more docs, because it makes no sense at all without.
So please elaborate on "Why?" (in-code, not on-issue).
I'm on crack. Are you, too?
Comment #405
David_Rothstein CreditAttribution: David_Rothstein commentedI've something similar with Firefox on Linux too (there, it's "Xorg" that jumps to 100% of CPU). Around 200 comments ago, I remember it being discussed that this was due to browser caching and that clearing the cache fixes it.... Still, seems like a pretty weird and disturbing problem.
Doesn't this prevent any shortcut from containing any query string at all? As a temporary fix to get this in, it makes sense though :) It seems to me like the real bug is in menu_get_item() and possibly in shortcut_valid_link() also, though.
Comment #406
ksenzee#402/1: I'm not entirely sure what you're seeing here, but I'm getting the expected behavior on a fresh install, on both FF3.0/Ubuntu and FF3.0/XP. I tried it on both the default profile (after disabling the overlay module) and on the minimal profile (after setting Seven as the admin theme). In both situations, when I enable the overlay module, the modules page reopens in the overlay and redirects the parent window to the homepage.
#402/3-5: I'm using FF3.0/XP on a 2.5-year-old Dell (Intel Core2 Duo 2.4 GHz, 4GB RAM) and it's positively snappy. As in a joy to use. I can't reproduce any of these performance problems. I could try serving the site on the XP box as well to see if that breaks, but FF on XP as a client has no performance issues that I can detect. IE7 seems fine too.
#404: This is not new code. Perhaps we could open a new issue for better documentation of Batch API. As it is, the only inline docs that are there are the ones I added.
#402/7: Good point. This is the same root problem as the fact that when you add a new shortcut (or otherwise edit the toolbar) the changes don't show up until the next full refresh of the parent window. We need an AJAX refresh of just the toolbar under certain conditions. I'd like to do this as a followup; I'm not sure whether or not the new AJAX framework supports something like this.
Comment #407
ksenzeeDavid_Rothstein: I talked to chx in IRC and according to him, menu_get_link() is not meant to be passed a query string ever. So yeah, that would be a separate issue.
Comment #408
webchickI spent about two hours with this patch tonight. Here's a list of the things I found. This is a UI review only. I'll be looking at the code shortly.
Note that no decisions will be made on this patch tonight. Dries is en route to DC tomorrow morning at the crack of dawn, and he wants time to take a thorough look at this before we make any sort of calls. So for now, here's my review. Skip down to the summary section if you don't care about the nitty-gritty details (and I wouldn't blame you!)
First thing I tried was applying this patch to an existing installation of Drupal 7 and turning on the module. That did not end well. ;)
http://img.skitch.com/20091020-fxiuynsx17pcu97waikm2x1hxh.png
However, things look fine on a fresh installation, and Katherine confirmed that she did make sure this patch worked on expert profile, so there is nothing that inherently ties it to Default profile, unlike Shortcuts. Probably some caching bug.
The first thing I tried was seeing if I could break shortcuts with this patch. There were two issues:
1. I get a shortcut add link on pages where the link is already in my current shortcut bar. This bug exists in HEAD as well. See #609100: Should not be able to re-add same shortcut to same set twice.
2. I managed to accidentally delete all of my shortcuts. ;) This bug exists in HEAD as well. See #609122: shortcut_set_save() no longer deletes existing links when a new set of links is passed in.
3. After adding a shortcut to the shortcut bar, it doesn't immediately appear. This is going to require some kind of AJAX fanciness, and may involve bug fixes to the new AJAX library in Drupal 7. I therefore think I agree with pushing this off to a follow-up issue.
I then tried to break the dashboard. Dragging/dropping blocks worked fine. The overlay even deals with me clicking "Configure block," changing the title of the block, and redirecting back to the Dashboard page properly. Very nice! :D
I found a couple of places where core fails to give feedback upon changing configuration, but these also exist in HEAD: #609108: Menu admin page should say something when you save #609128: Dashboard should provide feedback when clicking "Done"
At one point I also received "The block Powered by Drupal was assigned to the invalid region -1 and has been disabled." However, I have no idea what steps I went through to get that error, and so I wasn't able to verify whether or not it exists in HEAD. :( This wouldn't be a commit-blocker in any case, though, since on the whole the dashboard seems to work surprisingly well!
I *think* I might've reproduced the horizontal scrollbar issue. Try setting your window to 1024x768 or whatever. Load up the overlay. Now, shrink your window size down a bit. Even though the content area scales down appropriately (yay!), I'm still getting a horizontal scrollbar as though the full-size content area was still there.
This is quite minor, and fixing this can be a follow-up patch.
Now, to break edit in place. This I actually was able to do. Main page, click "Configure block" on "Powered by Drupal" block. Change some settings or whatever. Click save. You're taken to the front page (garland) loaded in the overlay:
This one is definitely less than ideal, since it's something that people will probably hit a lot since those links are so prevalent. I take it we need another instance here of whatever fixed it for nodes.
On this topic, I do notice that in general, the overlay contents seems to load sllowwwlllyyy. I'm not sure what that's about. It could be that I'm running xdebug profiling, perhaps. :P But it does make certain interactions, such as creating a node, pretty awkward:
1. I see the flash of the "Add to shortcuts" link on the node form when I first go there.
2. After submitting the form, I see the form re-presented to me in the overlay, then I see the default front page with no content (where I started from), and finally I see the contents of node/1.
I also noticed that neither of the user menus work while in the overlay:
I haven't read all 400+ replies :P but I could actually see the argument for this being "by design" at one point or another, so that certain links on the toolbar wouldn't take you back to the front-end theme. I still found it pretty jarring, however. I wonder if there's a way to either fix this, or somehow indicate visually that these links are temporarily inactive.
Then this gave me all sorts of other mean ideas, like clicking a node title from the node listing at admin/content and running cron. Yet, these all seemed to work fine. Darn! ;) I did find #609140: Rewrite outdated references to taxonomy_term_node and #609146: Tons-O-Notices running cron by playing around with this, but those are bugs in HEAD as well. I even tried changing the admin theme, but that also worked.
Also confirmed that the latest version plays nicely with Batch API. Yay! No double-toolbars in SimpleTest anymore!
I definitely see what Bojhan and others were talking about ~200 replies ago with the fact that you can't link to a page in the admin panel anymore, and that the behaviour of the overlay is inconsistent when you do:
As you can imagine, this also has "interesting" implications for the back button, in that you can get 4-5 levels deep in your administrative interface, hit back, and be taken to whatever page you were at *before* the page where you originally clicked the overlays. Very disorienting.
This is the main part of the patch that, frankly, is worrying... as in its current state it results in a net loss in usability in the admin panel. I realize we have contingency plans around using URL fragments, but I still am not sure how this will work in practice to both ensure a consistent user experience, fix the back button, and allow URLs to be bookmarkable. As to whether this is a commit-blocker, I'm afraid I don't know. But it is definitely a huge WTF (in fact, I'd even upgrade it to a full-on WTFBBQ, to use a JohnAlbin phrase ;)).
UI REVIEW SUMMARY
That's where things sit right now with me. I'll dive into the API next.
Comment #409
RobLoachRan into an overlay inside an overlay when I was playing around with the dashboard.
Comment #410
marcvangend@ Sun in #402-1:
I think you missed the discussion in #367, #370, #371 & #372.
Comment #411
webchickMeh. Sorry, but I totally ran out of steam for tonight. :( Here's a partial review as far as I got before my eyeballs started burning.
Summary: Major red flags around the Drupal.overlay.isAdminLink() stuff. Probably a commit blocker. :\ There's also some lengthy bit of logic going on in overlay_init() which looks a bit suspect, though I'm not sure what else to do there. On the whole, though the code in this patch is really well-documented (particularly the JS stuff, which I appreciate :D).
Let's document this please. I don't understand why you need to go through all of these hoops.
Wow, really? jQuery doesn't have one of these? jQuery sucks! :P
Minor, but I could commit this fix in about 30 seconds if it were a separate issue.
I don't really parse JS, so am pretty useless for a review here... but I wanted to say the comments in this file are very good, and helped me understand what was happening. :)
Ok, now this function? This function is pretty ghetto. :\ (To be fair, ksenzee admitted as much in #drupal earlier. :))
The first thing that makes me nervous here is that module developers (who are PHP wizards) need to learn *JavaScript* in order to add/exempt their links from the overlay. Eek. :\ But additionally, we're basing this off some crazy regex pattern stuff.
It'd be much better if there was a hook (or /maybe/ a generic "admin" => TRUE type of extension to hook_menu()...) that allowed module developers to declare *in PHP* which links should be overlay-enabled.
I realize that this must get tricky though when we talk about keeping the overlay code logically separated from the rest of core. So I can see why this approach was taken. But this code seems very hackish, and is probably a commit blocker. :\
(minor) We don't need @return None if there's nothing to return.
overlay.install is not listed here.
Is this still true? You seem to have used it elsewhere without this todo.
I had to read this quite a few times and didn't quite get it. One problem is the name of $set for that variable. Maybe $active or something would work better?
Don't we need to check_plain() this?
A: Oops. No. This is handled down below thanks to t().
PHPDoc please. Also this and the corresponding bit of code in toolbar.tpl.php could be committed very quickly separately, to make this patch a bit easier to review.
(minor) Wrap comments at 80 chars.
I'm on crack. Are you, too?
Comment #412
webchick@Rob Loach: Could you post your steps to reproduce? I tried several times to get overlays to do that again, and failed. :(
Comment #413
seutje CreditAttribution: seutje commentedit was on the roadmap for 1.4 -> http://docs.jquery.com/JQuery_1.4_Roadmap#Core
but it looks like it's pretty broken -> http://dev.jquery.com/ticket/4946
Comment #414
marcvangend@ Webchick #411:
If you're referring to the two-stepped $_SESSION['overlay_just_enabled'] variable over there: ksenzee and myself weren't too happy about the use of sessions there either, but I couldn't find a better way to do it. The thing is: we want to redirect after a form has been processed, but you can't implement that behavior with a form alter because the module would have to alter the form before it is enabled. By the way, this code would get a little cleaner once the use of url fragments is implemented, because we could just redirect to [basepath]#overlay-admin/config/modules.
Comment #415
Gábor Hojtsy@webchick/@sun/@David_Rothstein: I could reproduce the slowness on Firefox 3.5 on Mac. I think the peformance of the overlay depends on the exact browser version as much as the browser brand. Overlay works snappy in Safari but is noticably slow in Firefox 3.5. (Even if I just leave Firefox 3.5 open with the overlay and do something else, it uses huge amounts of CPU resources).
@webchick: "The block Powered by Drupal was assigned to the invalid region -1 and has been disabled." bug can be reproduced without the overlay and was caused by region support added to the block configure forms. Posted your reproduction note on http://drupal.org/node/503782#comment-2127414
@webchick: on shrinking overlay width, I can reproduce that in Safari, but not in Firefox 3.5. It does shrink multiple times but stops after a while. I bet it is due to how the width is calculated in relation to the animation and other effects.
@webchick/@ksenzee: I've had the impression that the in-overlay back button behavior works, but it indeed does not. That is simply solved by replacing doc.location.replace(url) with doc.location.href = url; in Drupal.overlay.load(). replace() does brake back button behavior. This does not solve back-ing into an overlay state from a node submission or from clicking a node in a node listing though.
Looking at the other feedback too.
Comment #416
Gábor HojtsyHere is an updated patch with the following fixes based on feedback from @webchick:
- documented the batch API changes (new 'url_options' batch setting now documented, its use to retain existing query string values is documented as well)
- @return none removed,
- overlay.install added to files list
- changed docs and variable name on overlay_close_dialog(), the trick is that this is used to set the overlay mode AND possibly pass on a value to the close callback
- added doc to template_preprocess_toolbar()
- wrapped garland CSS
Also worked in the fix to the back button problem when inside the overlay.
Fixed a reappearance of a bug I fixed before. The "to-overlay" class was used in some places in the overlay JS, but it is not a trigger anymore. So I fixed those places. This makes the active toolbar buttons to turn off their down-state when you close the overlay again. As expected.
I've also took considerable time trying to figure out why the user menu is not clickable when the overlay is on. I don't think it is intended, but could not figure out what is blocking that. Grm.
On isAdminLink(), yeah, we could go right into the menu API and have an admin => TRUE key on each menu item, but while it would be mostly overlay specific code in the menu, as documented above it would also require loading the menu item for every possible link displayed via url() or l() (and sometimes, when links are generated via JS from url() values passed in JS settings, this would not even be possible at all). Now loading menu items for all links on a page just to decide whether they are admin or not seems crazy. Also, trying to figure out each case what kind of menu item node/3245/subscriptions/manage belongs to. Would require the whole path -> menu key parsing logic to run before loading the menu item even. Is that an acceptable penalty? Any better ideas?
Comment #417
seutje CreditAttribution: seutje commentedI see the input was replaced with a regular link, but it seems like the things looking for an input weren't adapted to reflect these changes...
perhaps this would be better:
or even
or would that make it less readable?
Comment #418
David_Rothstein CreditAttribution: David_Rothstein commentedRegarding the isAdminLink() function, I have to say that as a PHP developer who is a bit shaky on JavaScript, looking at that code did not scare me nearly as much as I would have thought based on reading the above :)
If we decide to keep it in JavaScript (which seems to make some sense in theory, since the overlay is a JS feature), here are three things I can think of to improve it:
Any thoughts on whether those changes would make it committable?
Comment #419
webchickYou know, I didn't think about:
And also:
I was also thinking about this in the shower today, and for the most part it's far more common to define menu paths that /are/ admin paths than are not. So 'admin' => TRUE on every single thing would get really annoying.
I really like David's suggestions in #418. That would help this code indeed feel a lot less like a hack and more like an API of sorts. Is there someone who can work on that today by chance?
Incidentally:
This isn't a commit-blocker for me, since it's just a bug, but it is definitely a release blocker. We just want to be careful of how many of these release blockers we're accumulating, since that number has gone up significantly in the past week with all the exception patches. ;)
Comment #420
sunOK. The progress on this issue simply can't be followed anymore. There seem to be quite a couple of outstanding implementation issues and bugs that need to be fleshed out properly. Comment paging at #300+ adds to the nightmare, and we are even slowly getting to a 3rd page. :(
Here is what I propose we do now:
This will be the main issue we will work on to get the implementation right. We will try to get as many menu system, form API, JavaScript, and AJAX experts into that issue to find a working solution ASAP.
Due to the split from 1), this won't touch or change any APIs at all, because it's mainly JavaScript only. Note that everything committed after this one must be considered as API change.
All of those issues we tag with "Overlay".
Especially the initial summary for 2) will need some serious love and work to get it right. Only if the summary is right from the start, we are able to point people there and let them directly make any sense of the implementation issues to think about proper solutions.
Comment #421
effulgentsia CreditAttribution: effulgentsia commentedFollowing up on #420: I created an issue for the API changes: #610204: Overlay API changes. I'll start preparing the patch for #2 and #3, but the sooner the API changes can be committed, the sooner those patches can pass testbot, so please review #610204: Overlay API changes.
Comment #422
effulgentsia CreditAttribution: effulgentsia commentedLinks to the other two issues for after #610204: Overlay API changes lands:
- #610234: Overlay implementation (someone, please summarize on that issue what still needs to be resolved). Even though that patch can't be testbot'd until the API issue lands, developers can still try to troubleshoot outstanding problems if they can understand what those problems are without reading 400+ comments.
- #610238: Overlay styling issues
Comment #423
Gábor Hojtsy@webchick: your horizontal scroll bar in #408 is an artifact of the background webpage(!) being wider then the viewport, not related to the overlay being wide. Not at all sure how to make this not result in a scrollbar. Including this in my summary for #610234: Overlay implementation.
Comment #424
mgiffordOh my gosh this is a long issue queue! Is the patch in #416 the best one to review? How close is this to getting into core? Seems like a pretty powerful function if we're close.
I"m confused about if this issue is now a duplicate of:
#610204: Overlay API changes
or
#610234: Overlay implementation
I'm assuming the latter. I want to look at the accessibility issues here.
Comment #425
ksenzee@mgifford: The latest patch is at #610234: Overlay implementation. I suggest testing the patch in #48. In terms of functionality, all that's left to do is get the toolbar buttons to work in situations where you've just edited the toolbar.
Comment #426
mgiffordThanks. I've done some testing of #48. This is too long a thread for me to read it all. Gabor's video helped - http://www.vimeo.com/7451703
Can we mark this issue as a duplicate or closed since discussions have moved and this thread is no longer active (I hope - 426 comments is way too many).
Comment #427
Bojhan CreditAttribution: Bojhan commentedClosing this issue, we will continue discussion at #610234: Overlay implementation
Comment #428
xqbzzr CreditAttribution: xqbzzr commentedI havent read all 400 threads and i dont know, if this has been reported before, but right now i am testing d7 dev of 12/9 on an older pc and the dashboard-overlay is a huge performance killer! The fan speeds up to max and i am barely able to work with d7. I am looking for a way to deactivate the dashboard so i can continue testing.
Please keep in mind that eye-candy should never hinder performance.
Thank you for reading!
Comment #429
David_Rothstein CreditAttribution: David_Rothstein commented@xqbzzr: This issue is no longer active. Please see the other overlay issues, and in particular #615130: Overlay locks up the browser and consumes 100% of CPU for certain browsers/graphics cards/operating systems
In the meantime -- or any time you want, really :) - you can install Drupal using the "minimal" profile which does not enable the overlay module.