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?

CommentFileSizeAuthor
#170 overlay-followup-668640-170.patch4.3 KBDavid_Rothstein
#161 overlay-47-followup.patch3.65 KBcasey
#158 overlay-rtl-totalPatch.png12.41 KBaspilicious
#155 overlay-47-followup.patch5.57 KBcasey
#154 668640_followup.patch4.68 KBaspilicious
#154 close.png368 bytesaspilicious
#154 close-rtl.png368 bytesaspilicious
#154 overlay-rtl.png5.76 KBaspilicious
#144 overlay-47.patch76.37 KBcasey
#138 overlay-46.patch76.35 KBcasey
#133 no-overlay.jpg104.91 KBGeorg
#133 overlay.jpg105.73 KBGeorg
#130 overlay-45.patch75.79 KBcasey
#117 overlay-44.patch75.85 KBcasey
#85 overlay41.patch65.77 KBcasey
#81 overlay.jpg99.37 KBdhthwy
#74 overlay40.patch64.69 KBaspilicious
#72 overlay39.patch64.74 KBcasey
#71 overlay39+tableheader.patch73.78 KBcasey
#67 overlay-38+displace.patch83.69 KBcasey
#64 blocks-styling.png41.36 KBGeorg
#64 demonstrate-blocks.jpg65.37 KBGeorg
#64 no-toolbar.png58.16 KBGeorg
#63 ie7_broken_close_overlay.png2.2 KBaspilicious
#61 overlay-36+displace.patch83.21 KBcasey
#57 overlay-35+displace.patch82.99 KBcasey
#54 overlay-34+displace.patch82.57 KBcasey
#53 overlay-33+displace.patch81.64 KBcasey
#48 overlay-31+displace.patch80.07 KBcasey
#45 stickyTableHeaders.png25.71 KBaspilicious
#40 overlay-29+genericdisplace.patch78.75 KBcasey
#37 overlay-27.patch69.34 KBcasey
#36 overlay-26.patch69.13 KBcasey
#35 closing-x_half-hidden.png96.69 KBGeorg
#35 closing-x_half-hidden0.png97.46 KBGeorg
#30 overlay-24.patch68.7 KBcasey
#29 overlay-23.patch68.5 KBcasey
#27 overlay-22.patch67.72 KBcasey
#26 overlay-21.patch66.69 KBcasey
#25 overlay-20.patch65.7 KBcasey
#24 overlay-19.patch65.74 KBcasey
#23 overlay-18.patch59.64 KBcasey
#22 overlay-17.patch59.13 KBcasey
#21 overlay-16.patch59.12 KBcasey
#20 overlay-15.patch58.87 KBcasey
#19 overlay-13.patch55.52 KBcasey
#10 overlay-11.patch53.94 KBcasey
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

- It'll use less code.

At this point, is only an assumption. Write the patch, we will see :)

mcrittenden’s picture

Sub.

shunting’s picture

Makes 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).

Kiphaas7’s picture

Yes, 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.

kaakuu’s picture

Keeping subscribed. (@shunting this one is awesome, wish that can be done for Drupal blocks by an easy module /drools/)

aaron’s picture

As 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).

aspilicious’s picture

I'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?

David_Rothstein’s picture

Category: bug » task

This 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...

aspilicious’s picture

Yeah 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

casey’s picture

FileSize
53.94 KB

Initial 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...

JacobSingh’s picture

David_Rothstein’s picture

Well, as of now, at least, it does look like the "it'll use less code" prediction was right :)

diffstat overlay-11.patch 
 overlay/overlay-child.css  |  128 +++++++
 overlay/overlay-child.js   |   62 +++
 overlay/overlay-parent.css |  166 +--------
 overlay/overlay-parent.js  |  798 ++++++++++++---------------------------------
 overlay/overlay.module     |   76 +++-
 overlay/overlay.tpl.php    |   32 +
 toolbar/toolbar.js         |    2 
 toolbar/toolbar.module     |   10 
 8 files changed, 523 insertions(+), 751 deletions(-)
aspilicious’s picture

Nice 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

1) the primary tabs are gone
2) "add to shortcut" need to be repositioned (I know we wonna get rid of this issue in overlay, but today this is reality)
3) After installing a module, you don't get a message that your module is succesfully installed
4) Playing with collapsable fields: the behaviour is different than before (the decollapse brings you to an other place in the fieldset)
5) Overlay container is made smaller, make it the original width again (I can be wrong on this one)
6) in IE browsers: click configuration, close overlay, click configuration ==> overlay will not open. Other browsers are fine
7) again in IE: disable overlay, normal you need to get redirected to the overlay doesn't happen, related to #6 I guess
8) CRITICAL ISSUE: 
    1. Open your website TWICE
    2. Go to the modules page on both pages
    3) Disable the overlay in one of them
    4) Go to the other one and click on any toolbar item
    5) That page wants to open the link in the overlay, but the overlay is disabled so it goes nuts (burning my cpu ;) )
9) more a question: what happens if themes use a higher z value than overlay? can you prevent those possible problems?
10) secondary tabs are gone on dashboard load, after edditing set (pressing done) they appear. When clicking on one of those secondary tabs they are gone again.

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

1) if you go to the dashboard, take a new block from the top of the sreen and drag it down (outside the overlay), you see some weird effects, with this patch this is gone
2) Less code
3) Point 7 in the list above is fixed for opera ;)

WHAT HAVE I TESTED (in various browsers, all my test happen at least in IE7, IE8, firefox and chrome)

1) creating/editing article/basic page ==> No known issues yet
2) dragging with blocks ==> No known issues yet
3) installing module (see issue #3)
4) playing with fieldsets (see issue #4)
5) tested ctrl-t, ctrl-w ==> No known issues yet
6) closing and reopening overlay (see issue #6 and #7)
7) managing fields, I also played with the field selector and the widget gets updated accordingly (this was an issue in the old overlay few months ago) ==> No known issues yet
David_Rothstein’s picture

Considering 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? :)

aspilicious’s picture

And now some minor style issues

+++ modules/overlay/overlay-parent.js	28 Apr 2010 16:46:11 -0000
@@ -15,17 +15,28 @@
+    Drupal.overlay.scrollbarWidth = w1 - w2; ¶

Trailing white space

+++ modules/overlay/overlay-child.css	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,128 @@
+#overlay-content {
+  position: relative;
+  padding: .5em 1em;
+  color: #000;
+  background: #fff;
+  ¶

trailing white space

Powered by Dreditor.

Kiphaas7’s picture

#9: Prepare to try and fix ID clashes, and clashes with existing external js/css files and new js/css files to be loaded...

JacobSingh’s picture

Perhaps 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

Everett Zufelt’s picture

@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.

casey’s picture

FileSize
55.52 KB
casey’s picture

FileSize
58.87 KB

- TAB-navigation
- sticky tableheaders

casey’s picture

FileSize
59.12 KB

- calculation of scrollbar width wasnt working in webkit browsers (still need to improve outerResize).

casey’s picture

FileSize
59.13 KB

- With help of Kiphaas7 improved outerResize (turns out we don't need to calculate width of scrollbars)

casey’s picture

FileSize
59.64 KB

- fix tableheaderoffset

casey’s picture

FileSize
65.74 KB
casey’s picture

FileSize
65.7 KB
casey’s picture

FileSize
66.69 KB
casey’s picture

FileSize
67.72 KB
Kiphaas7’s picture

Just some quick things I didn't mention in IRC:

  1. Firefox resizes toolbar perfectly when making the window smaller, doesn't resize toolbar correctly when making the window larger. Both max-width and clip are not updated.
  2. Opera does not resize the toolbar correctly, the scrollbar is slightly overlapping parts of the toolbar that aren't overlapped in firefox. Have no idea why.
  3. Opera shows a bizarre white box when scrolled all the way down(with the scroll wheel). Tableheaders also stop working, so a wild guess would be that iframe #2 is weirding up. I also noticed that the scrollbars get moved up, as if I was scrolling inside the main document, instead of inside the iframe. Also have no idea why.

opera 10.53
firefox 3.6.3

casey’s picture

FileSize
68.5 KB

Fix 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.

casey’s picture

FileSize
68.7 KB

Fix for #28.3

dmitrig01’s picture

Status: Active » Needs review
aspilicious’s picture

Going to edit #13 tomorow! Nice work! Keep it going!

David_Rothstein’s picture

Awesome. 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:

 /**
+ * Implements hook_page_build().
+ *
+ * Add admin toolbar to the page_top region automatically.
+ */
+function overlay_page_build(&$page) {
+  if (overlay_get_mode() == 'child') {
+    $page['overlay'] = array(
+      '#pre_render' => array('toolbar_pre_render'),
+      '#access' => user_access('access toolbar'),
+      'toolbar_drawer' => array(),
+    );
+    $page['overlay']['titlebar'] = array();
+  }
+}

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:

-      if (!$target.attr('target')) {
-        $target.attr('target', '_new');
+      if (!$target.attr("target")) {
+        $target.attr("target", "_new");

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!

Georg’s picture

I 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)

Georg’s picture

I 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)

casey’s picture

FileSize
69.13 KB

Previous 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.

casey’s picture

FileSize
69.34 KB

Fixed another bug in IE. Looks like #761606: Inconsistent behaviour after overlay close is fully solved now.

casey’s picture

#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.

casey’s picture

casey’s picture

forgot patch.

Status: Needs review » Needs work

The last submitted patch, overlay-29+genericdisplace.patch, failed testing.

Georg’s picture

casey, 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)

webchick’s picture

Woah. 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.

Kiphaas7’s picture

From 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. :)

aspilicious’s picture

FileSize
25.71 KB

Here is a screenshot ==> the sticky table headers in overlay

casey’s picture

Status: Needs review » Needs work

Yes 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.

casey’s picture

casey’s picture

FileSize
80.07 KB

Fixes #42

Georg’s picture

This new approach also feels better than the "old" overlay.

#48 works very smooth on everything I tried so far.

aspilicious’s picture

Srry 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 ;)

aspilicious’s picture

I 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.

David_Rothstein’s picture

Can someone be very specific about what critical, release-blocking bugs this entire re-write of this module is intended solve?

In 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...

casey’s picture

FileSize
81.64 KB
casey’s picture

Status: Needs work » Needs review
FileSize
82.57 KB

Fixed issues found by aspilicious + fixed IE6 style.

Changed status to needs review, so this patch gets some more attention.

aspilicious’s picture

Another 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

casey’s picture

- 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.

casey’s picture

FileSize
82.99 KB

forgot patch

Status: Needs review » Needs work

The last submitted patch, overlay-35+displace.patch, failed testing.

casey’s picture

Status: Needs work » Needs review

#57: overlay-35+displace.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, overlay-35+displace.patch, failed testing.

casey’s picture

FileSize
83.21 KB

Updated #787940: Generic approach for position:fixed elements like Toolbar, tableHeader so it doesn't need the IE6 CSS hack any more.

casey’s picture

Status: Needs work » Needs review
aspilicious’s picture

One 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?

Georg’s picture

FileSize
58.16 KB
65.37 KB
41.36 KB

Whow! 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.jpg
If 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.

casey’s picture

Did 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.

Georg’s picture

The 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.

casey’s picture

FileSize
83.69 KB

Fixed #64

Georg’s picture

Ok, looks good now. Anyone else wants do have a look at this and mark it RTBC?

casey’s picture

Status: Needs work » Needs review
aspilicious’s picture

casey’s picture

FileSize
73.78 KB

Reroll

Note that patch still contains patch from #787670: Clean up tableheader.js. Please review that issue first, so we can downsize this patch.

casey’s picture

FileSize
64.74 KB

Oh 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).

aspilicious’s picture

Can'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

aspilicious’s picture

FileSize
64.69 KB

New patch nothing changed, only a css cleanup.

dhthwy’s picture

Status: Needs review » Needs work

Hrm 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.

aspilicious’s picture

Hidden? did you clear your cache? Used a fresh install?

tstoeckler’s picture

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.

Yes, that's what happens with or without this patch and is expected behaviour AFAICT.

dhthwy’s picture

Yes 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.

dhthwy’s picture

Ok 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!

casey’s picture

Could you provide a screenshot of the close button being invisible?

dhthwy’s picture

FileSize
99.37 KB

here it is.

aspilicious’s picture

dhtwy 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.

dhthwy’s picture

A 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...

dhthwy’s picture

It seems like the offender is Garland. In style.css in Garland's theme directory it contains this CSS:

 /* Don't display any header elements when within the overlay, and adjust the
   page height accordingly. */

body.overlay #header * {
  display: none;
}

body.overlay {
  margin-top: -80px;
}

If I comment that out, the overlay's header appears as expected. Garland needs a quick slap.

casey’s picture

Status: Needs work » Needs review
FileSize
65.77 KB

Added some Garland styling.

dhthwy’s picture

Nice casey, works like a charm now. -- Should this be RTBC after the test passes?

Status: Needs review » Needs work

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

dhthwy’s picture

Status: Needs work » Needs review

#85: overlay41.patch queued for re-testing.

Status: Needs review » Needs work

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

dhthwy’s picture

Status: Needs work » Needs review

#85: overlay41.patch queued for re-testing.

YesCT’s picture

Status: Needs review » Reviewed & tested by the community

based 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?

Kiphaas7’s picture

Testbot doesn't do anything for javascript code.

Dries’s picture

Having 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.

tstoeckler’s picture

This 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

aspilicious’s picture

I agree that we need a code review from at least one of our js specialists. (sun, ksenzee, ...)

casey’s picture

Status: Reviewed & tested by the community » Needs review

This 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.

David_Rothstein’s picture

I haven't looked closely at the code, only clicked around, but (besides the obvious, which is "this is great!"), the following issues came up:

  1. Why is the close button following me down the page when I scroll? :)
  2. When you initially open the overlay, the screen darkens for a little while but no window pops up. It looks like nothing is going to happen. The previous overlay behavior (where a blank white window opens immediately) is better, I think.
  3. When already in the overlay and clicking around, this patch makes it so the old content stays on the screen until the new content is ready, which I think most people will consider a definite improvement (closer to what a browser normally does) compared to the previous "window goes blank" behavior. However, it also seems to add a feature where the "Loading" title gets "..." continually appended to it to indicate progress? That seems really nonstandard to me - I don't think the titlebar should become a dynamic progress bar.

    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.

aspilicious’s picture

1. 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.

David_Rothstein’s picture

1. by design, was originally designed by mark, that way you always can close the overlay without scrolling back.

Personally 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?

3. the current behavior is the behavior you describe, I like the old and the new behavior.

Can you clarify what you mean by that? :)

aspilicious’s picture

1. 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.

casey’s picture

Besides 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.

Georg’s picture

I already grew to like the always present closing button. I'd leave it.

dhthwy’s picture

So 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.

NaheemSays’s picture

I 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.

tstoeckler’s picture

Either 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.

dhthwy’s picture

The 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.

casey’s picture

See #96.

Georg’s picture

To 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.

ksenzee’s picture

I 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.

David_Rothstein’s picture

I would really like to see a reroll with just the bare minimum of changes needed to make this patch work.

Which 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.

[me] # When you initially open the overlay, the screen darkens for a little while but no window pops up. It looks like nothing is going to happen. The previous overlay behavior (where a blank white window opens immediately) is better, I think.

[aspilicious] hmm maybe you're right, I can't realy judge as the page loads very fast here.

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).

Dries’s picture

Personally, I'd like to see this patch proceed. I would not take out stuff at this point, and get some additional reviews in.

Dries’s picture

Status: Needs review » Reviewed & tested by the community

Going to mark this RTBC. It looks good to me, and it allows us to proceed with various other bugfixes.

ksenzee’s picture

Status: Reviewed & tested by the community » Needs review

I 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

<a href="overlay:close">Close</a>

instead of something like

<a class="overlay-close" href="#">Close</a>

I'd very much like to know why that's necessary.

casey’s picture

Status: Needs review » Needs work

It 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.

ksenzee’s picture

Super - looking forward to the new patch!

Dries’s picture

Thanks for reminding me of #109 ksenzee. And thanks for your continued efforts casey. :)

casey’s picture

Status: Needs work » Needs review
FileSize
75.85 KB

Removed 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.

aspilicious’s picture

Casey can you remove the loading animation also, that way this patch doesn't get off-topic every 3 posts.

casey’s picture

In 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.

YesCT’s picture

the 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

YesCT’s picture

surely 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.

Everett Zufelt’s picture

So 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?

casey’s picture

There 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.

casey’s picture

RTBC?

bleen’s picture

Status: Needs review » Needs work

I 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

casey’s picture

@bleen18, have you cleared Drupal/browser cache?

bleen’s picture

I 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

bleen’s picture

Status: Needs work » Needs review

oops ... resetting status

bleen’s picture

Status: Needs review » Needs work

I found a small error with this patch. The toolbar is not making the correct item "active" when the overlay is open...
before
after

casey’s picture

Status: Needs work » Needs review
FileSize
75.79 KB

Ow 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.

Georg’s picture

Status: Needs review » Needs work

In 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.)

casey’s picture

Did you clear Drupal/browser cache? I can't reproduce.

Georg’s picture

FileSize
105.73 KB
104.91 KB

Without 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.

Georg’s picture

Status: Needs work » Needs review

@ #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.

casey’s picture

Status: Needs review » Needs work

@Georg (#133) Please open a separate issue; I think we can fix that, but lets not delay this issue even more.

casey’s picture

aspilicious’s picture

Status: Needs work » Reviewed & tested by the community

Ok 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, ....

casey’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
76.35 KB

aspilicious asked me to update documentation of theme preprocess functions conforming http://drupal.org/node/1354#hookimpl.

Status: Needs review » Needs work

The last submitted patch, overlay-46.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review

#138: overlay-46.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, overlay-46.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review

#138: overlay-46.patch queued for re-testing.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Back to rtbc

casey’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
76.37 KB

Yike an undefined variable in Drupal.overlay.eventhandlerSyncURLFragment made that function not working.

Still RTBC on green I guess/hope. @ksenzee, do you approve?

casey’s picture

Status: Needs review » Reviewed & tested by the community

back to RTBC

webchick’s picture

Status: Reviewed & tested by the community » Needs review

I'm as eager to get this in as anyone else, but please don't RTBC your own patch. :P

dhthwy’s picture

Status: Needs review » Reviewed & tested by the community

Well 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

ksenzee’s picture

Okay, 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.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Well, 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.

David_Rothstein’s picture

Status: Needs work » Reviewed & tested by the community

I 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:

  1. Is it my imagination (or my browser), or did the width of the overlay get a LOT smaller as a result of this patch? (It looks like that was mentioned as an issue way up in #13 as well... not addressed yet?)
  2. 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.

    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.

  3. - 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.

    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.

  4. +    // Use a more rigerous approach if the displaced element still overlaps
    

    Should be "rigorous".

  5. + * Event handler: restores size of displaced elements as they where before
    

    Should be "were".

  6. - *   A URL that will trigger the overlay (in the form
    + *   An URL that will trigger the overlay (in the form
    

    This change should be reverted.

  7. +  return '<div id="overlay-container" role="dialog"><div class="overlay-modal-background"/></div>';
    ...
    +  return '<iframe class="overlay-element" frameborder="0" scrolling="auto" allowtransparency="true" role="document"/>';
    

    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.

  8. + * - $title: the (sanitized) title of the node.
    

    This should refer to "page" rather than "node", I think.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work

Woohoo - 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.

David_Rothstein’s picture

Oh, 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 :)

webchick’s picture

LOL. 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?

aspilicious’s picture

Status: Needs work » Needs review
FileSize
5.76 KB
368 bytes
368 bytes
4.68 KB

Ok...

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?

casey’s picture

FileSize
5.57 KB

Extended 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

@@ -419,8 +419,6 @@
   }
 };
 
-Drupal.overlay.linksToRestore = [];
-

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

Status: Needs review » Needs work

The last submitted patch, overlay-47-followup.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review

#155: overlay-47-followup.patch queued for re-testing.

aspilicious’s picture

FileSize
12.41 KB

So 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!

Status: Needs review » Needs work

The last submitted patch, overlay-47-followup.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review

#155: overlay-47-followup.patch queued for re-testing.

casey’s picture

FileSize
3.65 KB

Moved RTL styling into #766170: Overlay lacks rtl styling

Status: Needs review » Needs work

The last submitted patch, overlay-47-followup.patch, failed testing.

casey’s picture

Status: Needs work » Needs review

#161: overlay-47-followup.patch queued for re-testing.

David_Rothstein’s picture

Status: Needs review » Reviewed & tested by the community

All looks good - thanks! I think everything else can be handled in followup issues.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Given 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.

YesCT’s picture

anyone more familiar with the issues...
which issues from #120, #46 and #47 does/did this actually fix?

casey’s picture

YesCT, have a look at the issue queue :) I set a lot of issues to fixed/closed.

casey’s picture

Status: Fixed » Reviewed & tested by the community

@webchick, looks like you only deleted/added the images but forgot the patch of #161.

David_Rothstein’s picture

While 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.

casey’s picture

#170 is RTBC.

casey’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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

MustangGB’s picture

Priority: Normal » Major
Bevan’s picture

A bug was introduced through this issue and/or the the code that it refactored; #1159586: Long titles make the overlay too wide.