Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
I don't think overlay should be based on jQuery UI Dialog.
- You hardly can name the overlay a dialog.
- We override/disable a lot that is done by jQuery UI Dialog.
- It'll use less code.
What do you think? Should I write a patch for this?
Comment | File | Size | Author |
---|---|---|---|
#170 | overlay-followup-668640-170.patch | 4.3 KB | David_Rothstein |
#161 | overlay-47-followup.patch | 3.65 KB | casey |
#158 | overlay-rtl-totalPatch.png | 12.41 KB | aspilicious |
#155 | overlay-47-followup.patch | 5.57 KB | casey |
#154 | 668640_followup.patch | 4.68 KB | aspilicious |
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedAt this point, is only an assumption. Write the patch, we will see :)
Comment #2
mcrittenden CreditAttribution: mcrittenden commentedSub.
Comment #3
shunting CreditAttribution: shunting commentedMakes sense, since overlay is not a modal dialog*. There are plenty of other design issues with this "page on top of a page," but it seems unlikely we'll resolve them by thinking of the overlay as something it isn't. (See on modal dialogs here, pro and here, con.)
* I'd argue that a lot of the issues that are framed as bug fixes are in fact a result of this category error: For example, both the (patched?) ability of the overlay to recurse and the (patched?) failure of the overlay to display error messages are not typical dialog box use cases, and result from other modes "tunnelling through" to the overlay. Further, the overlay's modality is quite porous: (1) users may restore context with the browser's back button, instead of by closing the overlay (as the video shows), and (2) users must scroll the overlay by going outside it to the browser's scrollbar. Right now, like the Holy Roman Empire, which was neither holy, nor roman, nor an empire, this modal dialog is neither a dialog nor modal. Making a modal dialog from a page that is fundamentally dynamic cannot be an easy thing!
OT: If I were king, I'd think about a way to make the overlay radically unobtrusive, maybe like a Photoshop floating palette that is ever present but expands or contracts based on a click -- or perhaps better, like a floating tab at the right or left margin that works like a tray. Having overlay come up when some links are clicked but not others is extremely confusing. I'm not sure this involves a huge level of effort in coding, but more being clear on what value overlay really adds and why. Heck,I can see the contributed module to sell advertising space on the floating tab right now...
UPDATE Here are some less obtrusive approaches using jQuery. (I find hijacking some admin links for overlay, and not others, both confusing and obtrusive. A lot of times, I want to change context for administration, so the primary use case for overlay, preserving context, does not apply. If sidebar tabs were used to call up overlay, then the hijacking would go away. Demos here, here, and here (with docking functionality).
Comment #4
Kiphaas7 CreditAttribution: Kiphaas7 commentedYes, a lot of code is disabled and overwritten, but jquery ui is implemented by default, so it would be crazy not to use some of it's code, unless we find critical bugs in that code. So I don't think it would use less code, since jquery ui is still included, whether we use the dialog function or not.
Comment #5
kaakuu CreditAttribution: kaakuu commentedKeeping subscribed. (@shunting this one is awesome, wish that can be done for Drupal blocks by an easy module /drools/)
Comment #6
aaron CreditAttribution: aaron commentedAs I pointed out at http://drupal.org/node/659488#comment-2438006, the current implementation of Overlays blocks other UI.Dialog modal popups. I'm all in favor of ditching that in favor of another that would allow modal dialogs within the overlay (or whatever it takes to make it play more nicely).
Comment #7
aspilicious CreditAttribution: aspilicious commentedI'm more into pushing this to D8...
Cause there isn't much time...
I know the overlay needs a rewrite in D7 and if not in D7 in D8.
But we need a stable patch for this before the end of march, then we can decide if we wonna replace it.
Casey: deal?
Comment #8
David_Rothstein CreditAttribution: David_Rothstein commentedThis is a task, not a bug, right? (As far as I know, the comment in #6 is out of date.)
Though from #716612: Overlay is not accessible to screen reader users it does sound like the ARIA stuff currently used by jQuery UI Dialog is a bit annoying from an accessibility standpoint...
Comment #9
aspilicious CreditAttribution: aspilicious commentedYeah I know it needs a rewrite.
Best would be a rewrite in team offline for this. So they can communicate directly.
Then post a testing patch that can be reviewed.
1) we have to get rid of the iframe and the jquery dialog thingie
2) we need more seperate functions, now it's a pile of code, which can't be reused
Comment #10
casey CreditAttribution: casey commentedInitial patch. This approach allows a lot of issues to be fixed much easier. e.g. #649662: Sticky tableheaders don't work in Overlay)
Problem is that it is becoming a massive patch...
Comment #11
JacobSingh CreditAttribution: JacobSingh commentedHah,
@see http://drupal.org/node/668640#comment-2411028 :)
Comment #12
David_Rothstein CreditAttribution: David_Rothstein commentedWell, as of now, at least, it does look like the "it'll use less code" prediction was right :)
Comment #13
aspilicious CreditAttribution: aspilicious commentedNice work, I discussed this with Ksenzee and webchick in irc. Chanches to get this in into D7 are kinda small (cause of the size of this).
BUT I don't give up. First I wonna get rid of the obvious bugs. AFTERWARDS I wonn focus on testing every aspect of drupal. This post will be a dump of everything I tested and others tested.
The first issues I found:
WHAT DOESNT'T WORK YET
Code tags are needed, else drupal eat my spaces
These ones need to be fixed rly quickly if we wonna get this in!
PS: If others find Issues I'll add them to this list
WHAT DOES THIS PATCH FIX
WHAT HAVE I TESTED (in various browsers, all my test happen at least in IE7, IE8, firefox and chrome)
Comment #14
David_Rothstein CreditAttribution: David_Rothstein commentedConsidering that this (or something like it) might be required for #716612: Overlay is not accessible to screen reader users, I'm thinking that makes it a more realistic possibility for a "whatever code freeze we're in now exception", no? :)
Comment #15
aspilicious CreditAttribution: aspilicious commentedAnd now some minor style issues
Trailing white space
trailing white space
Powered by Dreditor.
Comment #16
Kiphaas7 CreditAttribution: Kiphaas7 commented#9: Prepare to try and fix ID clashes, and clashes with existing external js/css files and new js/css files to be loaded...
Comment #17
JacobSingh CreditAttribution: JacobSingh commentedPerhaps I'm coming in too ignorant about our needs to re-write what jQuery UI dialog is doing, but has anyone reached out to the jQuery UI Dialog developers to see what can be done?
Scott González
http://blog.nemikor.com/
Is the UI development lead. I'm sure he'd be a great help here. At the very least, Javascript can generally be extended runtime by simply replace methods. Is there no way we can reduce all the code being developed here? We use jQuery UI for everything, so developing a paralel system may introduce later complexities on top of simply being less peer reviewed and more work to maintain.
Maybe there is no way to meet all our needs, but raising those concerns in the appropriate place (jQuery UI) at least assures that it is on their roadmap for further inclusion.
Best,
Jacob
Comment #18
Everett Zufelt CreditAttribution: Everett Zufelt commented@JacobSingh
Screen-readers and UI modal dialog - jQuery Accessibility | Google Groups
http://groups.google.com/group/jquery-a11y/browse_thread/thread/3b59aa35...
I don't recall if Scott was part of the discussion or not, but the accessibility of jQuery UI modal dialogs was posted on the accessibility list, to which I am quite certain Scott is a member.
Comment #19
casey CreditAttribution: casey commentedComment #20
casey CreditAttribution: casey commented- TAB-navigation
- sticky tableheaders
Comment #21
casey CreditAttribution: casey commented- calculation of scrollbar width wasnt working in webkit browsers (still need to improve outerResize).
Comment #22
casey CreditAttribution: casey commented- With help of Kiphaas7 improved outerResize (turns out we don't need to calculate width of scrollbars)
Comment #23
casey CreditAttribution: casey commented- fix tableheaderoffset
Comment #24
casey CreditAttribution: casey commentedComment #25
casey CreditAttribution: casey commentedComment #26
casey CreditAttribution: casey commentedComment #27
casey CreditAttribution: casey commentedComment #28
Kiphaas7 CreditAttribution: Kiphaas7 commentedJust some quick things I didn't mention in IRC:
opera 10.53
firefox 3.6.3
Comment #29
casey CreditAttribution: casey commentedFix for #28.2
#28.1 is because we only update max-width/clip if displaced element's is wider than document; still looking for a clean solution.
Comment #30
casey CreditAttribution: casey commentedFix for #28.3
Comment #31
dmitrig01 CreditAttribution: dmitrig01 commentedComment #32
aspilicious CreditAttribution: aspilicious commentedGoing to edit #13 tomorow! Nice work! Keep it going!
Comment #33
David_Rothstein CreditAttribution: David_Rothstein commentedAwesome. I tried this out briefly; still some obvious bugs of course, but overall it works pretty well. It seems a lot faster too - for example, loading the "list links" page for the management menu was previously very slow, but now is tolerable!
I only looked quickly at the code, but one thing did jump out as being a little scary:
What is going on here - why does the overlay module have to add the toolbar?
My other comment is that this patch seems to contain some unrelated changes (code style, etc), for example things like:
This is already going to be a gigantic patch either way, so I would recommend moving anything like that to a separate issue if possible (and does Drupal even have any JavaScript code style rules about the use of quotes?) - it will make this patch easier for reviewers to figure out if it is as focused as possible on the main goal. I have personally made the mistake of trying to fix lots of unrelated things in one patch before, and it rarely goes well :)
Great work!
Comment #34
Georg CreditAttribution: Georg commentedI applied patch from # 30 to just see, what would happen. I only took a glance at it.
Maybe it's just me and my imagination, or the overlay feels totally different. It just seems to be somewhat better, without being able to clarify why or how. I did not look into the patch, it's just my feeling, after applying it.
I'll test further, when the patch is close to being done.
Firefox, Chrome, IE8, Safari
(each in its latest version)
Comment #35
Georg CreditAttribution: Georg commentedI forgot to test it in Opera.
#773396: After creating a node in Opera the new node isn't displayed and overlay breaks. a duplicate of #761606: Inconsistent behaviour after overlay close is solved in Opera, after applying #30.
In IE8 the button to close the overlay is half "hidden" under the toolbar on some pages. (screenshot)
Comment #36
casey CreditAttribution: casey commentedPrevious patch was crashing in IE8 on some pages...
overlay_page_build() wasn't supposed to be there (leftover)
I also suggest to have a look at #787940: Generic approach for position:fixed elements like Toolbar, tableHeader. That patch could simplify overlay even more.
Comment #37
casey CreditAttribution: casey commentedFixed another bug in IE. Looks like #761606: Inconsistent behaviour after overlay close is fully solved now.
Comment #38
casey CreditAttribution: casey commented#35 "In IE8 the button to close the overlay is half "hidden" under the toolbar on some pages."
Please review #787940: Generic approach for position:fixed elements like Toolbar, tableHeader. with that patch this issue is fixed.
Comment #39
casey CreditAttribution: casey commentedSo you can test it, I combined it with #787940: Generic approach for position:fixed elements like Toolbar, tableHeader and #787670: Clean up tableheader.js
Comment #40
casey CreditAttribution: casey commentedforgot patch.
Comment #42
Georg CreditAttribution: Georg commentedcasey, if I understood you correctly, you had already included in you patch in #40 the patches of the two in #39 mentioned issues.
So I only applied #40.
Looks good, what I found:
1) edit any content (Contend -> edit on any node)
2) try to delete. (takes two tries in IE8)
In ALL Browsers the toolbar is displaced down, thus overlapping the overlay again.
(Chrome, Firefox, IE8, Safari, Opera)
Comment #43
webchickWoah. WOAH! Hold the phone here, please.
Can someone be very specific about what critical, release-blocking bugs this entire re-write of this module is intended solve? We are trying to get D7 out the door here, folks. This is not the time for wild refactoring of this nature, unless you have an insanely good reason.
Comment #44
Kiphaas7 CreditAttribution: Kiphaas7 commentedFrom what I see, this approach solves a few issues. It brings back scrolling inside the iframe, solving the need for workarounds for sticky tableheaders (since the iframe get's scrolled, not the parent window), and the need for checking the height of the iframe content every 150ms. Furthermore, it slightly increases performance. Casey probably has some more arguments.
(parent scrollbars are removed, so double scrollbars are not an issue)
Also, there is an actual code reduction compared with the jquery UI dialog option.
Critical bugs? Err, probably atm only tableheaders. If you don't want the refactoring in D7 (and move it to D8?), but one of the other workarounds, you'll be duplicating code from tableheader.js and adding it to overlay (at least 10kb worth of code).
The refactoring is IMHO needed, whether or not it makes D7 is up to you and Dries. :)
Comment #45
aspilicious CreditAttribution: aspilicious commentedHere is a screenshot ==> the sticky table headers in overlay
Comment #46
casey CreditAttribution: casey commentedYes there are several other issues that hardly can be solved with the current approach.
For example try to drag blocks (on admin/structure/block) from the top to the bottom. If the blocks table is bigger than your screen the screen won't scroll down when dragging. Same for dashboard. Problem is that the iframe is resized to fit its content and thus won't fire scroll events on which jQuery UI draggable depends on. #692726: Drag & Drop Not Fully Functional While Using Overlay
Of course we can try to dispatch scroll events to the iframe (like is being tried in #649662: Sticky tableheaders don't work in Overlay) but this will just result in one big hack.
Other issues this patch very likely fixes/might fix:
If we would give overlay any change, I really suggest to consider this patch.
Comment #47
casey CreditAttribution: casey commentedSize of this patch can be reduced if patches like the following are committed first:
Comment #48
casey CreditAttribution: casey commentedFixes #42
Comment #49
Georg CreditAttribution: Georg commentedThis new approach also feels better than the "old" overlay.
#48 works very smooth on everything I tried so far.
Comment #50
aspilicious CreditAttribution: aspilicious commentedSrry not much time to review, found some issues:
1) While loading the page, hide the add to shortcut button!
2) customise dashboard link is totally broken ;)
Will come back to this later ;)
Comment #51
aspilicious CreditAttribution: aspilicious commentedI screencast "quickreviewed" it.
Srry if you people can't understand me, just watch the video then ;).
http://www.screencast.com/t/NTFkYzk5
After filming I found that closing/opening the overlay in IE is still a little wonky.
I'll try to find a reproduce path.
Comment #52
David_Rothstein CreditAttribution: David_Rothstein commentedIn addition to ones mentioned above, I think this would be part of the solution for #716612: Overlay is not accessible to screen reader users (although not the whole solution). I don't think anyone has come up with a way to deal with that issue that doesn't involve getting rid of jQuery UI Dialog's ARIA properties. There are simpler ways to get rid of them than this patch, but at some point, there is a question of why use a library that is apparently built for a different purpose than the overlay is trying to use it for...
Comment #53
casey CreditAttribution: casey commentedComment #54
casey CreditAttribution: casey commentedFixed issues found by aspilicious + fixed IE6 style.
Changed status to needs review, so this patch gets some more attention.
Comment #55
aspilicious CreditAttribution: aspilicious commentedAnother screencast (again srry if you can't understand me in some parts of it). ==> http://www.screencast.com/t/MmE0ZDBkMW
I'm saying something bout the loading animation.
In the patch it goes like this:
1) Loading
2) Loading .
3) Loading ..
4) Loading ...
5) Loading ....
6) Loading .....
I prefer if it's like this
1) Loading
2) Loading .
3) Loading ..
4) Loading ...
5) Loading
6) Loading .
7) Loading ..
Why?
1. It looks better ;)
2. If something is happening and the overlay need to think very long, the dots will run out of the window
Comment #56
casey CreditAttribution: casey commented- Fixed dots limit on 10
- Fixed dashboard clearfix
Increasing the document height by dragging blocks to the bottom of the document (as shown in video) is also possible without overlay.
Comment #57
casey CreditAttribution: casey commentedforgot patch
Comment #59
casey CreditAttribution: casey commented#57: overlay-35+displace.patch queued for re-testing.
Comment #61
casey CreditAttribution: casey commentedUpdated #787940: Generic approach for position:fixed elements like Toolbar, tableHeader so it doesn't need the IE6 CSS hack any more.
Comment #62
casey CreditAttribution: casey commentedComment #63
aspilicious CreditAttribution: aspilicious commentedOne small esthetic issue found....
Space between close overlay png and overlay in IE7 while loading. (screenshot)
Tested overlay with bartik and seven theme.
In different browsers (IE7, IE8, chrome, firefox, opera, safari).
I tested everything I mentioned above and in my screencasts.
ps: is it possible to diasble scrolling when blocks are dragged outside the overlay?
Comment #64
Georg CreditAttribution: Georg commentedWhow! At first I thought WTF. The toolbar at the top is now in the background, when I work in the overlay. I was already so used to quickly jump around settings using the toolbar, that I missed it badly. Without the toolbar, you need to either close the overlay, or jump back to the dashboard. Either way it results in more clicks as before. I think, this is a regression. see: no-toolbar.png
On the other hand. The Overlay works so much more smooth than before. I'm inclined to accept the regression of having more clicks for this benefit. Drag and droping Blocks and other things now also scrolls the page appropriately. On small screens there is so much more space now. It's just annoying when setting up a new site and you need to edit so many settings. But later on, when you perform one task and be done with it, this is probably even better. - What do others think about this?
Just two minor things I noticed.
On the Blocks page there is a link to
Demonstrate block regions (Garland)
. On that page something's wrong (in all Browsers). see demonstrate-blocks.jpgIf there is a long Block Title... see for your self: blocks-styling.png
I did not find anything else to complain about. All in all this looks good.
Comment #65
casey CreditAttribution: casey commentedDid you clear Drupal cache? The toolbar isn't supposed to be below the modal background. If you did clear the cache, in what browser do you experience this? Edit never mind. Something has changed that makes this patch not work any more. I'll post a new patch in a few minutes.
I am not sure what you mean with demonstrate-blocks.jpg. Is it different without this patch?
About long block titles, good catch. I'll have a look.
Comment #66
Georg CreditAttribution: Georg commentedThe demonstrate blocks thing itself doesn't work, thats in issue #655416: "Demonstrate block regions" bugs with regions, navigation, overlay.
My hint concerned the overlay only - thats this issue :-) : The closing X is gone and the title is missing. Without the patch this is correct.
Comment #67
casey CreditAttribution: casey commentedFixed #64
Comment #68
Georg CreditAttribution: Georg commentedOk, looks good now. Anyone else wants do have a look at this and mark it RTBC?
Comment #69
casey CreditAttribution: casey commentedAdded #615204: Overlay (and other iframe) scrolling overlaps the toolbar to my list in #46.
Comment #70
aspilicious CreditAttribution: aspilicious commented#787940: Generic approach for position:fixed elements like Toolbar, tableHeader is commited. Can you reroll?
Comment #71
casey CreditAttribution: casey commentedReroll
Note that patch still contains patch from #787670: Clean up tableheader.js. Please review that issue first, so we can downsize this patch.
Comment #72
casey CreditAttribution: casey commentedOh well, its no longer dependant on that patch, so I removed it here (it's a nice cleanup though, so reviews of #787670: Clean up tableheader.js are still welcome).
Comment #73
aspilicious CreditAttribution: aspilicious commentedCan't find any issues anymore. Webchick/Dries install this one on your next test install. You'll love it (or not).
Green light for me.
Needs more reviews
Comment #74
aspilicious CreditAttribution: aspilicious commentedNew patch nothing changed, only a css cleanup.
Comment #75
dhthwy CreditAttribution: dhthwy commentedHrm the X button on the overlay is almost entirely hidden except for a tiny sliver. This is on Firefox 3.6.3. I can provide a screenshot if necessary. Should be happening to others testing this?
Also if I enter the /admin path directly into the address bar it doesn't trigger the overlay, not sure if this is intended behavior.
Furthermore, whenever I click a button it disappears and is replaced by a black rectangle for a second.
Other than that this overlay does feel much smoother and it uses atleast half as much JS as the other overlay. Pages load faster and my FF lags a little less due to the lower memory/CPU footprint. I hope this gets in, otherwise I probably won't use overlay at all due to the clunkiness feeling with the current one.
My stance is this:
A better performing overlay will enhance usability and overall experience.
A poor performing overlay will add friction and frustration. If many people feel it is clunky and overlay is provided as default, then it is possible Drupal 7 will be associated with a clumsy new UI and usability and its reputation will suffer.
Comment #76
aspilicious CreditAttribution: aspilicious commentedHidden? did you clear your cache? Used a fresh install?
Comment #77
tstoecklerYes, that's what happens with or without this patch and is expected behaviour AFAICT.
Comment #78
dhthwy CreditAttribution: dhthwy commentedYes I cleared the cache.
No this is an upgrade install. But if I add "top: 10px" to #overlay-close:hover in overlay-child.css it moves the button down so I can see it. I doubt it'll make any difference with a fresh install in light of the CSS change I made.
Thanks tstoeckler for pointing that out.
Comment #79
dhthwy CreditAttribution: dhthwy commentedOk the rendering issue (black rectangle) doesn't happen with IE 8 or Opera. It's probably just because I have a ton of tabs open in FF and it's rendering slow. I also tested with the "top: 10px" on IE+Opera+Firefox and the close button all show up fine now. Not sure if that should be there, I'm not a CSS expert. So I think the main issue now is just with the close button being hidden.
Oh yea, the overlay is crazy fast!
Comment #80
casey CreditAttribution: casey commentedCould you provide a screenshot of the close button being invisible?
Comment #81
dhthwy CreditAttribution: dhthwy commentedhere it is.
Comment #82
aspilicious CreditAttribution: aspilicious commenteddhtwy thnx for the screenshot, can you reinstall again and tell us if you still see the same problem?
Don't forget to clear cache anyway.
Comment #83
dhthwy CreditAttribution: dhthwy commentedA new install worked and I think it is because it uses the Seven theme by default. As soon as I switched to Garland it screwed up again. And it isn't just the close button its the whole top part, doh!.
Edit: Yea it seems to work OK on Seven, and Zen and Genesis. It's just having issues with Garland? I guess...
Comment #84
dhthwy CreditAttribution: dhthwy commentedIt seems like the offender is Garland. In style.css in Garland's theme directory it contains this CSS:
If I comment that out, the overlay's header appears as expected. Garland needs a quick slap.
Comment #85
casey CreditAttribution: casey commentedAdded some Garland styling.
Comment #86
dhthwy CreditAttribution: dhthwy commentedNice casey, works like a charm now. -- Should this be RTBC after the test passes?
Comment #88
dhthwy CreditAttribution: dhthwy commented#85: overlay41.patch queued for re-testing.
Comment #90
dhthwy CreditAttribution: dhthwy commented#85: overlay41.patch queued for re-testing.
Comment #91
YesCT CreditAttribution: YesCT commentedbased on passing the testbot, the works like a charm in #86 and my visual inspection of the code for coding standards, marking this rtbc. Anything else specifically needed to test it in more detail?
Comment #92
Kiphaas7 CreditAttribution: Kiphaas7 commentedTestbot doesn't do anything for javascript code.
Comment #93
Dries CreditAttribution: Dries commentedHaving read all the comments and having watched the screencast.com video, it feels like this patch would be a significant improvement that allows us to fix numerous issues. I'd be in favor of getting this in, but it would be great to get some more people to test it first.
Comment #94
tstoecklerThis looks perfect. Without having measured it I would bet money that the rendering time is faster with the patch. Scrolling works perfectly,
tabledrag.js, and tableheader.js work (in contrast to HEAD). Color module works, works in all themes, and clicking around for ~10min didn't do anything unexpected.
While I'm not certified to judge the actual JS code, I can say that:
1. all those red lines make me happy.
2. the code adheres to coding standards and is very well documented.
3. I love the fact that we get an overlay.tpl.php.
+1 on RTBC
Comment #95
aspilicious CreditAttribution: aspilicious commentedI agree that we need a code review from at least one of our js specialists. (sun, ksenzee, ...)
Comment #96
casey CreditAttribution: casey commentedThis patch is dependent upon #787940: Generic approach for position:fixed elements like Toolbar, tableHeader. There is however discussion over there that patch went in to quickly without sufficient reviewing.
Setting this one back to needs review as it still can use more reviews.
Comment #97
David_Rothstein CreditAttribution: David_Rothstein commentedI haven't looked closely at the code, only clicked around, but (besides the obvious, which is "this is great!"), the following issues came up:
Basically, the only way in which this feels different from a normal browser page load is that my mouse icon doesn't switch to "busy" while the page is loading... right? If we could fix that, we wouldn't even need a special progress indicator in the overlay at all, IMO. But if we do need a dynamic progress indicator, I'm not sure the titlebar is right for that.
Comment #98
aspilicious CreditAttribution: aspilicious commented1. by design, was originally designed by mark, that way you always can close the overlay without scrolling back.
2. hmm maybe you're right, I can't realy judge as the page loads very fast here.
The downside of that approach is that the page will recalculate height when it's loaded.
3. the current behavior is the behavior you describe, I like the old and the new behavior. We probably need bohjan or some other UI expert for this.
Comment #99
David_Rothstein CreditAttribution: David_Rothstein commentedPersonally I find it distracting, but more to the point - it will never really work with any overlay theme that has different colors, unless we can make the close button color change dynamically as you scroll :) One of the advantages of this patch is that it might help allow #790732: The overlay tabs and close button look like a minor train wreck in themes without a white background to be solved, but a moving close button would make that impossible again.
Also, how is this change related to the move away from jQuery UI Dialog, which was the main goal of the patch?
Can you clarify what you mean by that? :)
Comment #100
aspilicious CreditAttribution: aspilicious commented1. Hmm good catch about the colours....
Can't answer this immediately.
This isn't related to moving away from jQuery UI Dialog. So it can be dropped if we all agree about this
3. Without this patch your mouse icon is switched to busy while loading a page. So the current behavior without the page is the desired approach for you.
Comment #101
casey CreditAttribution: casey commentedBesides the reasons I mentioned in the opening post, the approach being used (the whole overlay inside the iframe) cannot be combined with jQuery UI dialog. This new approach however fixes some critical issues that currently can't be solved very easily.
I included the scrolling close button since I saw an issue for that (#655514: Usability issues with overlay close button) and to show it can be done easily, but maybe its better to discuss that in that issue.
Switching the mouse icon can be done.
Comment #102
Georg CreditAttribution: Georg commentedI already grew to like the always present closing button. I'd leave it.
Comment #103
dhthwy CreditAttribution: dhthwy commentedSo far David_Rothstein has provided the only good justification for taking out the fixed scroll close button. So it should probably be taken out. If there is a way you could configure that behavior it would make both camps happy. If not, I'd just take it out to keep this from stalling. Someone go get reviewers.
Casey, funny because that's David_Rothstein's issue. I think #99 trumps that though.
Comment #104
NaheemSays CreditAttribution: NaheemSays commentedI would like to add an opposing voice and support the moving close button - I remember watching some of the earlier overlay windows and noticing how some people had trouble getting out of the overlay mode in some situations.
This should help fix that and allow people to leave easily no matter how far down a page they are.
Comment #105
tstoecklerEither way, David_Rothstein is right in that the moving/not-moving close-button is totally unrelated to this patch. In order to get this thing in as soon as possible, I suggest moving this discussion to a new issue and leaving the close button where it is for now. Same probably goes for the Title "..." loading thing.
Comment #106
dhthwy CreditAttribution: dhthwy commentedThe moving close button really doesn't make a difference to me, only if it's gonna cause this to stall.
Have you both reviewed/tested the patch? If yes please say so, so we can RTBC this again. Dries wants more reviewers.
Comment #107
casey CreditAttribution: casey commentedSee #96.
Comment #108
Georg CreditAttribution: Georg commentedTo get more reviews, I just applied the patch from #85 again to the current head.
I played with it. I tried it with different themes (bartik, corolla). I installed new modules (ubercart).
Everything I did, worked nicely in overlay. I did not find any issue.
I'd leave the closing X in view all the time. It' makes live easier. It just feels right. It makes you feel in control. But if the new overlay get's into core sooner without: so be it. I'll test that follow-up issue for sure.
On the other hand: the patch from #85 works wonderfully, just get it committed and be done with it. We could skip all the process of taking features out again (like the stay-atop closing X; or the loading..... dots) and having to test the new patch all over again. I see no objections against this patch as is.
Comment #109
ksenzeeI hadn't had a chance to review this patch since about #10. I finally took it for a spin and looked over the code. Huge improvements since then - a ton of work has gone into this. A few high-level thoughts:
- The basic approach is sound, and does indeed fix a lot of problems we've been working around.
- It feels quite zippy!
- The two-iframe approach is quite clever.
- David's point in #33 about signal-to-noise ratio is valid. It makes it much harder to review such a huge patch when a bunch of it is moving code chunks, or changing double quotes to single quotes, or renaming "self" to "this" (since we're not actually instantiating objects), or changing jQuery bind namespaces. See http://webchick.net/please-stop-eating-baby-kittens. I would really like to see a reroll with just the bare minimum of changes needed to make this patch work.
A few more detailed questions:
- Is the overlay protocol thing ('overlay:close') necessary, or is it meowing?
- Why the change from jQuery.fn.trigger() to jQuery.fn.triggerHandler()?
- How much of the keydown event handler code was preexisting, and how much is brand new? I can't easily tell from the patch.
- Any chance of getting the original loading animation back?
I really hate the idea of rewriting the whole front end of a module just before release, but this fixes enough problems that I'm cautiously in favor of getting a version of it in. I'd still like to review the new Drupal.overlayChild.behaviors more carefully, and we could sure use a heap of cross-browser testing.
Comment #110
David_Rothstein CreditAttribution: David_Rothstein commentedWhich also means moving the close button code/discussion to #655514: Usability issues with overlay close button.... because leaving it in here (even if it is a good idea) just sidetracks the main issue. There's plenty of room to discuss it over there :)
The "Loading..." discussion probably does belong here, though, at least in the sense that if this patch gets rid of the "white box appears before the content loads into it" (I am not sure to what extent that is a required change or not) then it does need some other visual indicator to tell the user the page is loading. However, if we can make the mouse icon look busy after a link is clicked, then as far as I can tell an overlay page load looks just like a normal browser page load at that point, so the extra "Loading" dots animation (and maybe even the "Loading" wording itself) might be unnecessary.
In general it loads very fast for me too, but some pages are slow enough for it to be a problem. I think if you use the contextual links to click "List links" on the Management menu, that might be the best way to see it. That is probably the slowest page in Drupal (with or without the overlay).
Comment #111
Dries CreditAttribution: Dries commentedPersonally, I'd like to see this patch proceed. I would not take out stuff at this point, and get some additional reviews in.
Comment #112
Dries CreditAttribution: Dries commentedGoing to mark this RTBC. It looks good to me, and it allows us to proceed with various other bugfixes.
Comment #113
ksenzeeI hesitate to undo Dries's RTBC, and obviously he's welcome to commit it anyway, but I'd still like answers to my questions in #109. I'm especially concerned that we're introducing a whole new link "protocol" for closing the overlay. We're going to have links like
instead of something like
I'd very much like to know why that's necessary.
Comment #114
casey CreditAttribution: casey commentedIt is dependent upon #787940: Generic approach for position:fixed elements like Toolbar, tableHeader, but that one is rolled back and needs work.
On the new protocol, I agree a class would do too. I found some bugs too. I'll post a new patch tomorrow.
Comment #115
ksenzeeSuper - looking forward to the new patch!
Comment #116
Dries CreditAttribution: Dries commentedThanks for reminding me of #109 ksenzee. And thanks for your continued efforts casey. :)
Comment #117
casey CreditAttribution: casey commentedRemoved dependency on #787940: Generic approach for position:fixed elements like Toolbar, tableHeader (Although the problems that issue is trying to fix do still exist) so we don't need to wait for that one.
For clarity I renamed all functions that are event handlers; I gave them names that indicate the action being performed.
Bug I mentioned in #114 had to do with URL fragment not updating when using Drupal.overlay.open|close|load() directly.
Also removed the scrolling close button, we can discus that in the appropriate issue.
Comment #118
aspilicious CreditAttribution: aspilicious commentedCasey can you remove the loading animation also, that way this patch doesn't get off-topic every 3 posts.
Comment #119
casey CreditAttribution: casey commentedIn response to questions of #109
- Is the overlay protocol thing ('overlay:close') necessary, or is it meowing?
Replaced this by a class "overlay-close".
- Why the change from jQuery.fn.trigger() to jQuery.fn.triggerHandler()?
We don't need to actually trigger the events as we don't need any default handling of those events. triggerHandler() calls just the handlers bound using jQuery which is just what we want.
- How much of the keydown event handler code was preexisting, and how much is brand new? I can't easily tell from the patch.
Its all new. previous handlers don't work no more since the whole overlay is inside the iframe now.
- Any chance of getting the original loading animation back?
The whole overlay is inside the iframe now. So there are no container/wrapper divs (the divs that make the overlay look like a dialog) in the DOM yet. When loading a new page into the overlay the current stays visible until the new one is loaded. When opening the overlay however there is no current page. We could add an (cachable) empty page to overlay_menu() and load that one first when opening the overlay. This might be a solution for slow loading pages.
I am not sure about the suggestion of @David_Rothstein in #110 making the mouse icon look busy; In winXP I don't see no busy mouse icon when browsing in any browser.
@aspilicious
I would keep the loading animation and discuss any other approach in another issue.
Comment #120
YesCT CreditAttribution: YesCT commentedthe following critical issues might be fixed by this (non-critical??) patch
#649662: Sticky tableheaders don't work in Overlay
#725734: Overlay doesn't escape any page titles (residual cleanup)
#679190: Overlay breaks the Update manager workflow
maybe:
#716612: Overlay is not accessible to screen reader users
http://drupal.org/community-initiatives/drupal-core
says
this issue (668640) is critical (but the priority is actually normal)
I didnt list all the "normal" issues that this patch would fix. See comment #46 and #47
Comment #121
YesCT CreditAttribution: YesCT commentedsurely this is major, even if it is not critical?
In preparation for the new "major" issue priority, I'm tagging this (not actually critical) issue as priority-major.
See: http://drupal.org/node/669048#comment-3019824 (#669048: We should have four priorities not three (introduce a new 'major' priority)) for more information about the coming major issue priority.
Comment #122
Everett Zufelt CreditAttribution: Everett Zufelt commentedSo that I can provide a preliminary accessibility assessment, can someone please let me know what the functionality of this patch is?
I.e. what happens when a user with proper permissions attempts to open an admin page from the site?
Comment #123
casey CreditAttribution: casey commentedThere haven't changed very much accessibility-wise. The overlay is still wrapped inside two aria roles; a "dialog" role and inside that a "document" role. Screenreaders now should also mention overlay's document title on opening (I tested this with FF/NVDA).
But I'd like you to assess the patch; I might have overseen things.
Comment #124
casey CreditAttribution: casey commentedRTBC?
Comment #125
bleen CreditAttribution: bleen commentedI just applied the path in #117 and the overlay will not open at all... The screen gets dark, but no overlay.
I'm using MAMP 1.8.4
I tested in chrome5.0.3 & in FF3.6
I have a suspicion that this patch will also fix #692726: Drag & Drop Not Fully Functional While Using Overlay but so far I havent been able to test
Comment #126
casey CreditAttribution: casey commented@bleen18, have you cleared Drupal/browser cache?
Comment #127
bleen CreditAttribution: bleen commentedI take it back... a more thorough clearing of the cache fixed the weird behavior I was seeing in #125.
Incidentally I have now checked, and this patch does fix #692726: Drag & Drop Not Fully Functional While Using Overlay ... thats one more to add to the list in #46
Comment #128
bleen CreditAttribution: bleen commentedoops ... resetting status
Comment #129
bleen CreditAttribution: bleen commentedI found a small error with this patch. The toolbar is not making the correct item "active" when the overlay is open...
Comment #130
casey CreditAttribution: casey commentedOw good catch. For #117 I removed dependency on #787940: Generic approach for position:fixed elements like Toolbar, tableHeader but forgot a Drupal.displace check in Drupal.overlay.resetActiveClass.
Comment #131
Georg CreditAttribution: Georg commentedIn Chrome it's broken.
You can open the Overlay correctly, but when you go to another site in the overlay, the overlay quits on you. You are still on the correct page, but not using the overlay anymore.
e.g.:
1) goto Configuration
2) goto Performance (now you are not using the overlay anymore.)
Comment #132
casey CreditAttribution: casey commentedDid you clear Drupal/browser cache? I can't reproduce.
Comment #133
Georg CreditAttribution: Georg commentedWithout the overlay the category where you are working in is still highlighted.
With the overlay (old version, or after applying the patch) the highlighting is only for the landing page, not a subpage.
e.g.
Permissions is a subpage of People.
Without the overlay, People is highlighted on the toolbar. (see no-overlay.jpg)
When using the overlay, nothing is highlighted on the toolbar. (see overlay.jpg)
This is the same behavior before, so I don't know, whether this can be fixed easily in this issue.
Comment #134
Georg CreditAttribution: Georg commented@ #132: Yes, I had. But now, when I tried again (without flushing cache anew) it suddenly worked. It always worked in all the other browsers... I don't know what trick my computer played on me.
setting back to needs review.
Comment #135
casey CreditAttribution: casey commented@Georg (#133) Please open a separate issue; I think we can fix that, but lets not delay this issue even more.
Comment #136
casey CreditAttribution: casey commentedComment #137
aspilicious CreditAttribution: aspilicious commentedOk I'm going to push this because there is a feeling of bashing towards overlay BUT those people DON'T review this patch...
An dit does fix most of the concerns of those people!!!
So I reviewed this (again)...
I made a google doc review: http://spreadsheets.google.com/ccc?key=0AnMmC4VjMWMXdHh5WGxRLS1qR0dvT3hM...
Everyone can edit this if they feel they have to.
1) 9! issues will be fixed without doubts (testes them myself in all the browsers you can find in the google doc document (all the green blocks)
2) 2 will probably be fixed (according to casey) couldn't test them myself, so feel free to test them and edit the document
3) 1 issue will partially be fixed in some browsers, but that is still more than what we have at the moment and the current solution won't fix this.
GET THIS IN!
Support of the community, got code review of ksenzee, ....
Comment #138
casey CreditAttribution: casey commentedaspilicious asked me to update documentation of theme preprocess functions conforming http://drupal.org/node/1354#hookimpl.
Comment #140
aspilicious CreditAttribution: aspilicious commented#138: overlay-46.patch queued for re-testing.
Comment #142
aspilicious CreditAttribution: aspilicious commented#138: overlay-46.patch queued for re-testing.
Comment #143
aspilicious CreditAttribution: aspilicious commentedBack to rtbc
Comment #144
casey CreditAttribution: casey commentedYike an undefined variable in Drupal.overlay.eventhandlerSyncURLFragment made that function not working.
Still RTBC on green I guess/hope. @ksenzee, do you approve?
Comment #145
casey CreditAttribution: casey commentedback to RTBC
Comment #146
webchickI'm as eager to get this in as anyone else, but please don't RTBC your own patch. :P
Comment #147
dhthwy CreditAttribution: dhthwy commentedWell I was going to but how does one RTBC an RTBC? NR then RTBC? Oh, I suppose just say I RTBC?
not that mine counts. :D
Comment #148
ksenzeeOkay, given that Dries has said he wants to go this direction, this is RTBC. There are some doc typos and such but we can fix those in a minor followup.
I would really appreciate if everyone who has supported committing this patch this late in the cycle would test the heck out of the overlay in the next little while to make sure we don't have any regressions to go along with the great improvements here. Thanks casey for working like crazy on this and chasing HEAD for so long.
Comment #149
webchickWell, I just tried this again tonight, since ksenzee gave it a final thumbs-up, and I must say that this sucker is LIGHTNING fast now. Holy smokes! GREAT job, casey, and aspilicious and others for your very detailed reviews! It's amazing how this came together like it did.
Committed to HEAD!! :D
PS: In the future, though, it'd be much easier to get patches like this in if they didn't completely slaughter kittens... it makes reviewing a terribly daunting task. :(
PS 2: To address Everett's question, there's no visual change in this patch from before. Just the under-the-hood implementation has changed. A new accessibility review would be appreciated, though, to see if we by any chance addressed previous issues, though I doubt it...
PS 3: Warning: You must clear the cache or you'll just get blank overlays with no admin screens when you cvs up if you have Overlay module enabled. Since you can't clear the cache from the UI anymore in this situation, you'll have to either use
drush cc all -y
(which didn't work for me due to a fatal error unrelated to this patch) or truncate all of your cache_* tables (which did, but not sure which one(s) triggered it).It sounds like we need to set this back to "needs work" for some follow-up tasks. In particular, I notice the font issue from #129 still, and Katherine mentioned some minor style clean-up. Probably best to make separate issues and cross-link them here, and mark this 'fixed' once we get most of them documented.
Comment #150
David_Rothstein CreditAttribution: David_Rothstein commentedI looked at this for a while, read through all the code, and agreed it's more or less there. I wish I had the time to do a complete, in-depth review, but honestly I don't - this would be a monster patch either way, and the extra changes stuck in there really don't help with making it reviewable, sorry. But I don't see any big red flags, and overall this is a vast improvement!
Some things I noticed along the way, mostly minor things:
It depends on the browser (and my choice of "busy icon" to describe it may not have been the best words). Safari does this on Windows, Firefox does it on Linux only (or actually might depend on browser version), but IE doesn't seem to do it all. It's a bit less widespread than I thought, so overall when you take into account all browsers, this patch doesn't affect the loading experience quite as much as I thought it did. So as per @aspilicious in #118 it's even less clear why playing around with the "Loading" animation is necessary here or at all related to the patch; though I admit it's pretty cute :) But we could equally well consider removing "Loading" altogether later on, rather than adding to it here, since for many browsers the overlay already now feels like a normal page load.
Yikes :) Right, as discussed a few places above, the initial overlay opening is where the animation would be most useful now, because the window doesn't pop up as fast anymore. I think that probably qualifies as "polish" and can be deferred until later, though.
Should be "rigorous".
Should be "were".
This change should be reverted.
I think the original code did this (or at least some of this) also, but is there some reason I'm not understanding why these are using self-closing divs and self-closing iframes? AFAIK that is not valid HTML - those are not elements which are supposed to be allowed to self-close.
This should refer to "page" rather than "node", I think.
Comment #151
David_Rothstein CreditAttribution: David_Rothstein commentedWoohoo - crosspost! Well, I guess I got a little start at making notes for potential followups - but that's probably not a complete list either :)
Re the font issue in #129, I think that is not really a problem caused by this patch, but rather almost the other way around. Before this patch, the fonts may have matched but that was mostly by coincidence. The overlay title was actually inheriting fonts from the theme underneath it, so that e.g. if the page underneath it was using Zapf Dingbats, then the overlay title would have started using that too, whereas the rest of the overlay would stick with the fonts provided by Seven ! (There are some issues in the queue about that which we can probably close or at least modify now.)
So I haven't looked in detail, but I think what's happening now is that the overlay is properly using its own fonts, but the toolbar is still not insulating itself from the rest of the page, so it actually might be a toolbar bug more than an overlay one.
Comment #152
David_Rothstein CreditAttribution: David_Rothstein commentedOh, huh, just realized that the screenshot in #129 wasn't actually intending to show the font issue, but rather something else.... however, you can see the font issue pretty prominently in it also :)
Comment #153
webchickLOL. That's funny. And ok, cool, I can buy that rationale of this new font stuff being 'by design'.
Thanks a lot for your review. I'm sorry for cross-posting. :( casey/aspilicious, could you get a follow-up patch tomorrow that addresses David's feedback so we can close this sucker out?
Comment #154
aspilicious CreditAttribution: aspilicious commentedOk...
Fixed 1, 4, 5, 6 and 8 of #150
Added rtl style for overlay! whoehoew!
Shortcuts aren't working yet in rtl but that is another issue (seven-rtl + shortcut-rtl)
Webchick:
- Please replace the close image with the new ones attached to this post (they are smaller in size and include a rtl close button).
- And remove the loading image, we don't use that anymore
Casey:
can you look at 7 of #150
Can we close this and file separate issues if this follow-up is in?
Comment #155
casey CreditAttribution: casey commentedExtended patch from #154
- #150.7 Replaced the self-closing div and iframe. Doesn't really matter for jQuery but I agree it is more clear.
- Removed some cruft
Note that rtl needs to be tested in conjunction with patches of
- #740182: Toolbar and shortcuts lack RTL styling
- #766458: Seven theme lacks rtl styling
Comment #157
aspilicious CreditAttribution: aspilicious commented#155: overlay-47-followup.patch queued for re-testing.
Comment #158
aspilicious CreditAttribution: aspilicious commentedSo for people that can't/don't want to test overlay rtl here is a screenshot with all those patches applied.
Webchick would say: AWESOME!
Comment #160
aspilicious CreditAttribution: aspilicious commented#155: overlay-47-followup.patch queued for re-testing.
Comment #161
casey CreditAttribution: casey commentedMoved RTL styling into #766170: Overlay lacks rtl styling
Comment #163
casey CreditAttribution: casey commented#161: overlay-47-followup.patch queued for re-testing.
Comment #164
David_Rothstein CreditAttribution: David_Rothstein commentedAll looks good - thanks! I think everything else can be handled in followup issues.
Comment #165
webchickGiven that RTL stuff got moved to a separate issue, I only committed close.png (and removed loading.gif). Sound right?
Committed #161 to HEAD, and those changes. Thanks! Marking fixed now.
Comment #166
YesCT CreditAttribution: YesCT commentedanyone more familiar with the issues...
which issues from #120, #46 and #47 does/did this actually fix?
Comment #167
casey CreditAttribution: casey commentedYesCT, have a look at the issue queue :) I set a lot of issues to fixed/closed.
Comment #168
YesCT CreditAttribution: YesCT commentedwent through the critical ones from #120. (still would be nice to go through #46 and #47.)
http://drupal.org/node/649662#comment-3062016 confirms this fixed #649662: Sticky tableheaders don't work in Overlay
http://drupal.org/node/725734#comment-3066630 says this helped with #725734: Overlay doesn't escape any page titles (residual cleanup) but some follow up is needed there
http://drupal.org/node/679190#comment-3061982 and http://drupal.org/node/725734#comment-3066630 confirm this fixed (mostly) #679190: Overlay breaks the Update manager workflow and the the rest of that is being followed up in #821248: Installing a module or running updates from the overlay makes you leave the overlay but doesn't bring you back.
I dont know about #716612: Overlay is not accessible to screen reader users.
Comment #169
casey CreditAttribution: casey commented@webchick, looks like you only deleted/added the images but forgot the patch of #161.
Comment #170
David_Rothstein CreditAttribution: David_Rothstein commentedWhile we're at it, then, let's slip one more easy typo fix in there that I just noticed:
an displaced
=>a displaced
:)And also
+/* $Id$ */
in the CSS file in the previous patch was missing the closing$
, so I added that.Comment #171
casey CreditAttribution: casey commented#170 is RTBC.
Comment #172
casey CreditAttribution: casey commentedhttp://drupal.org/cvs?commit=378632
Comment #174
MustangGB CreditAttribution: MustangGB commentedComment #175
Bevan CreditAttribution: Bevan commentedA bug was introduced through this issue and/or the the code that it refactored; #1159586: Long titles make the overlay too wide.