Closed (fixed)
Project:
Drupal core
Version:
7.x-dev
Component:
overlay.module
Priority:
Critical
Category:
Bug report
Assigned:
Reporter:
Created:
3 Dec 2009 at 00:35 UTC
Updated:
22 Jun 2010 at 06:00 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
triversedesigns commentedUpdate: this also does not work on the Permissions page as well. It seems to be all table headers.
Comment #2
Ryan Palmer commentedSticky table headers uses the css position: fixed; to place table headers when scrolling beyond the table header.
http://www.w3schools.com/Css/pr_class_position.asp
Perhaps the solution here is to use "absolute" instead of "fixed" for the tableheader. I haven't tried this, but I know could never get sticky table headers to work in overlays on Drupal 6.
Comment #3
casey commentedSubscribing.
Comment #4
casey commentedHmm this is a difficult one. Because of the autoFit functionality for the overlay there never are scrollbars in overlay's iframe window.
Comment #5
kiphaas7 commentedYet another consequence of using an iframe. As casey pointed out, the overlay makes sure the iframe never gets scrollbars, and since the iframe contains an entire separate document, the stickyheader never get's activated, because the document in the iframe never shows scrollbars. So either we make sure the iframe has scrollbars, or we make sure that the sticky header code works with our overlay. (I prefer the first approach).
Comment #6
kiphaas7 commentedThird option would be adding extra JavaScript behavior to our overlay code so that tableheaders work again, by binding the scroll event of the mainwindow, then calculate the scrollOfset of the main window, the offset of the iframe against that main window, and set the difference of those 2 as an offset for the absolutely positioned stickyheader.
That option would make more sense than adding extra scrollbars. Thoughts?
Comment #7
casey commentedYea that should do. But using JS for position of stickyheader doesn't have the smooth effect that position:fixed in CSS has.
We could also try to copy those stickyheaders to the parent window inside the #overlay-container (this one needs position:relative then) just above overlay-iframe.
Comment #8
kiphaas7 commentedPosition: relative won't work as you imagine it would, position fixed is relative to the browser window, so position relative wouldn't help. http://www.quirksmode.org/css/position.html
Taking it outside the iframe opens a whole new can of worms: the offset needs to be calculated, the width needs to resized with every window resize.... I think it's less costly to keep it in the child frame and manipulate it there.
Comment #9
kiphaas7 commentedFirst attempt at restoring sticky tableheaders.
Comment #11
casey commentedDidn't test it yet, but should it be based on height of elements with the "overlay-displaced-top" class instead of just the toolbar?
Comment #12
kiphaas7 commentedIt get's the offset from the wrapper element, and the wrapper indeed calculates for overlay-displaced-top. I'm only doing an explicit calculation for the shadow of the toolbar, so that the shadow falls over the table header.
----
That said, performance wise it would be better to move some code around
This:
To:
My dev pc is currently undergoing some maintenance, so can't reroll it myself for now :).
Comment #13
reglogge commentedThe patch in #9 only partially works for me:
- no effect in Safari4/Mac
- table headers only partially visible behind shortcuts bar in FF3.5/Mac (see screenshot sticky_headers.png)
- table headers only partially visible without shortcuts bar in FF3.5/Mac (see screenshot sticky_headers_wo_shortcuts.png)
Haven't tested in other browsers yet.
Comment #14
aspilicious commentedThis need some love!
Comment #15
BarisW commentedIt works great on the Permissions page but not on other tables. Could it have something to do with the height of the THEAD (because the table header is higher on the permissions page due to a word-wrap)?
Tested on Firefox 3.6.3 on MacOSX.
Comment #16
webchickAs much as I hate to do this, this is really a crappy bug that we need to fix.
Comment #17
tstoecklerRetitling.
Comment #18
kiphaas7 commentedLet me start off by saying that I don't really like my own patch, and that it's not finished (see @todo in patch and comments below). Allthough it brings back the auto-follow in the browsers I tested (IE 8, Opera 10.5, FF3.6, Chrome, Safari 4), it does that with some downsides.
Quirks in the patch:
self.$iframe.load(function() {does not fire at all in opera 10.5. Big wtf for me, and a quick google didn't resolve anything. Could be a jquery bug, could be an opera bug.self.$iframeDocument.ready(function() {does fire in all tested browsers, but needs a rather raunchy timeout function (at least I think so...)......
But it does work! :)
Comment #20
kiphaas7 commentedOne other way would be to clone the original table outside the iframe, and apply the behavior there. Would stop the jerkiness while scrolling (since the scoll event is no longer abused)
I tried and failed in that approach, need someone who is more capable for this issue...
Comment #21
cwgordon7 commentedI'll probably fix this over the next few days.
Comment #22
kiphaas7 commentedNice, looking forward to your solution!
Comment #23
cwgordon7 commentedOk, so here are our options as I see them:
1. Manually adjust the css "top" property to keep the tableheader where it should be on the iframe. Problem: this is clunky, and shows up poorly in the browser (e.g. patchy/laggy), particularly in slower browsers.
2. Take the tableheader outside of the iframe, and then keep it on top at a constant point on the page, like we currently do with the toolbar. Problem: this would change the theming of the tableheader in the case that there is a different theme within the overlay. Solution: Put it in its own iframe with the correct theme. Problem: this is ugly and complicated. Secondary problem: there's no easy way to solve alignment issues if we go down this path.
3. (my favorite) Change how the overlay works. Currently we make the overlay as big as it needs to be and let the page scroll. I think this approach is problematic, not only for the tableheader but for some other situations too, such as when you open the overlay when you're already on a long page and you're left with a large shaded-out region at the bottom of the page. The solution would be to disable page scrolling when the overlay opens (body { overflow-y: hidden }), and instead have the overlay iframe itself scroll. I personally think that this would be the most intuitive scrolling mechanism anyway, and would allow us to solve the tableheader issue without much trouble.
Discussion welcome. I'd particularly like ksenzee to look at this.
Comment #24
cwgordon7 commentedAlternatively, for #2, I think we don't have to use an iframe if we use the jQuery calculated CSS plugin (see http://github.com/peol/jquery-computed-style), which I think would allow us to apply the calculated CSS properties of the tableheader within the iframe directly onto the tableheader outside the iframe.
Comment #25
kiphaas7 commented#23:
1 Is what I tried. Clunky it is.
2 Is IMHO the best option for now. Together with something like you mentioned in #24, it would be usable. The clunkiness of 1 would be solved by using position:fixed. Only problem left is calculating for what distance the header needs to be shown.
3 is not really an option. You can't hide the main vertical scrollbar in all browsers, and that would mean 2 scrollbars, which sucks. I already tried binding the scroll event of the main window to the iframe, but that is also clunky.
Comment #26
casey commented@Kiphaas7 "You can't hide the main vertical scrollbar in all browsers, and that would mean 2 scrollbars, which sucks"
Could you give me an example?
Comment #27
cwgordon7 commented@Kiphaas7 Why can't you get rid of the vertical scrollbar in all browsers just by reducing the size of the content so no scrollbar is necessary? If we need a browser-specific trick, we can do it (e.g. hide main page content when the overlay opens in IE6 / etc.).
Comment #28
kiphaas7 commentedErr, I was sure it couldn't be done cross browser. I just checked, and it seems possible. Sorry :/
But: if the scrollbar of the main window gets hidden, and the iframe scrollbar is used, doesn't that mean the iframe scrollbar can't be stretched over the entire main viewport (as one would expect), because of the toolbar overlaying the iframe?
Comment #29
kiphaas7 commentedI'm pretty close to a fix while taking path #2. Needs some more love though, will post when done.
Path 3 would be more ideal, but I thought I saw a patch by casey somewhere doing something like #3, I tried approach #2.
Comment #30
kiphaas7 commentedInitial try at restoring sticky tables according to method #2 suggested in #23.
Works OK in safari and Firefox, apart from graphical errors also good in IE8, but extremely buggy in opera 10.53 for reasons beyond me.
Needs a thorough review and thoughts whether this is the way to go.
Personally, thinking more and more that method #3 in #23 might be better, if the scrollbar issue can get worked around.
Anyone up for making a patch using method #3?
EDIT: didn't use the plugin suggested in #24 because it somehow didn't produce reliable results crossbrowser for me...
Comment #31
cwgordon7 commentedShouldn't that be:
Should Drupal.overlay.getCSS be Drupal.overlay._getCSS to signify that it's an internal function? Or furthermore, should that be moved to drupal.js? Also that array needs a bit of formatting work to comply with coding standards. Are those all the possible CSS properties there? Any reason for the ordering of those properties, or should they be alphabetized?
Lastly, statements like
while(i--)andif(i = self.stickyTables.length)need spaces between the while/if and the opening parentheses.Other than that, looks like a solid proof of concept. I'll be happy to update this with the required complex resizing code and/or track down bugs in particular browsers.
Comment #32
MichaelCole commentedMarking "needs work" based on comments in #31.
How would you reproduce/test this?
Is this really a critical bug? No crash, or data corruption. Workaround would be to scroll back up right? Am I missing something?
Comment #33
webchickI feel it's critical because it effectively breaks a major user-facing feature that worked just fine in D6.
Comment #34
kiphaas7 commentedjust thought of this: A very simple workaround would be instead of using auto following table headers, to just insert the tableheaders every n rows in the table itself....
@31: thanks for the comments, will work those in a new patch.
That array of css values certainly does not contain all possible values. I got it somewhere from stackoverflow I think, and at least for the array part, was pretty much copy paste.
Still nobody wanting to try method 3? I'd hate not having explored that path before comitting to this approach.
Comment #35
casey commentedmethod 3 needs a lot of things done differently. #668640: Overlay shouldn't be based on jQuery UI Dialog uses method 3.
Comment #36
kiphaas7 commentedI just tried #668640, and IMHO it's way more robust, and needs less code (and sticky tables are already functional there!).
Yes it needs more changes, but would be better in the long run.
I'd like to hear some consensus from people higher up in the chain what's the status on these 2 issues: if #668640 has a good chance of being committed for Drupal 7, then it's not worth pursuing this (IMHO technical inferior) issue. I'd rather try and help finish #668640.
I tried in my patches in this issue to emulate the original conditions for sticky table headers, in #668640 the original conditions are restored. #668640: Overlay shouldn't be based on jQuery UI Dialog is the high road :).
Comment #37
cwgordon7 commentedThe usability people don't like approach #3 because of the "scrollbar within a scrollbar" problem. Since the entire point of the overlay is supposed to be to improve usability, I'm inclined to listen to the usability people on this one. We could just make the tableheader not scroll down, of course, but that's also a degraded experience.
I think the best idea is to go with the solution in #30 - not terribly elegant, but workable. I'll be playing around with this a bit tonight.
Comment #38
cwgordon7 commentedHere's an updated version of the solution in #30. It now works with resizing and toolbars and stuff, plus a little bit of code cleanup. Possibly ready for a thorough review.
Comment #40
cwgordon7 commentedLet's try that again...
Comment #41
cwgordon7 commentedbump? :)
Comment #42
aspilicious commentedLet's wait a little more. If #668640: Overlay shouldn't be based on jQuery UI Dialog gets committed this one is useless.
Comment #43
yesct commentedI just marked #668640: Overlay shouldn't be based on jQuery UI Dialog rtbc. Should it be critical if it fixes this critical issue?
Comment #44
yesct commented#40: sticky-tables-overlay-alternative-2-02.patch queued for re-testing.
Comment #45
yoroy commentedWorks in FF, Chrome and Safari under OS X
Comment #46
yesct commented#40: sticky-tables-overlay-alternative-2-02.patch queued for re-testing.
Comment #47
casey commentedPlease review #668640: Overlay shouldn't be based on jQuery UI Dialog... This issue is fixed when that one lands.
Comment #48
webchickSweet! Confirmed! Had to clear cache first.