Created attachment 658639 [details] linux.png The design for bug 754472 means the click-to-play plugin popup notification may change size (for instance, if the user plays all flash content, that entry in the popup is removed, and it gets a bit smaller). On OS X and Windows, this seems to work fine, with the overall popup window shrinking as needed. On Linux, it will frequently not resize.
Created attachment 660261 [details] [diff] [review] patch So, in widget/gtk2/nsWindow.cpp::Resize(int, int, bool), it looks like nsView::WindowResized is called, which eventually results in nsXULPopupManager::PopupResized being called. This can set the width and height of the popup. Later, when the size of the popup is being calculated, since the height has been set, it is assumed that this is the minimum size, which may no longer be accurate. As far as I can tell, OS X and Windows don't call WindowResized in their respective Resize implementations. So, I tried doing what it looked like they were doing (calling Invalidate), and this seems to work (although it doesn't work if I replace the other call to WindowResized in the other Resize function).
Created attachment 660628 [details] [diff] [review] patch So, disregard the previous comment and patch. What I'm seeing now is when nsMenuPopupFrame::LayoutPopup is called on OS X, the popup is open when its size has changed, so SetSizeConstraints gets called and the minimum size is updated as expected. On Linux, it looks like the size changes while the popup is not yet open, and then it gets opened, meaning SetSizeConstraints never gets called. This patch makes SetSizeConstraints always get called if the size has changed, but I'm not sure if that's what we want. Here's a try run anyway: https://tbpl.mozilla.org/?tree=Try&rev=250a45672133 (the failed tests are because of other changes I've made to the click-to-play plugin popup code, and shouldn't be because of this change).
David, did you want someone to review this?
Neil - I guess I was waiting to talk to someone (e.g. you) about this, but if you think this is the right way to fix this bug, feel free to review it.
Comment on attachment 660628 [details] [diff] [review] patch Marking for review based on the above comment.
Comment on attachment 660628 [details] [diff] [review] patch The patch looks ok, but I'm suprised it is needed. Is it just this popup that this applies to? We could check this in for now, and file a bug to investigate and make a reduced testcase.
Try run: https://tbpl.mozilla.org/?tree=Try&rev=956fcf03f94c Marking checkin-needed.
Comment on attachment 660628 [details] [diff] [review] patch [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 357725 User impact if declined: panels layout broken, see also bug 793487 Testing completed (on m-c, etc.): m-c, m-a Risk to taking this patch (and alternatives if risky): small String or UUID changes made by this patch: none
Comment on attachment 660628 [details] [diff] [review] patch CTP is a new feature, so we can take a polish fix like this - let's get this landed asap.
Should this be fixed in 17.0b4? I'm still seeing this in some cases. Possibly only when the popup has overflowed the screen height and then the contents have shrunk again? Symptoms are a big grey (in my theme) border filling the bottom of the popup where there are no longer children. This is happening with a tooltip, I didn't test other popup types.
(In reply to Ian Nartowicz from comment #16) > Should this be fixed in 17.0b4? I'm still seeing this in some cases. > Possibly only when the popup has overflowed the screen height and then the > contents have shrunk again? Symptoms are a big grey (in my theme) border > filling the bottom of the popup where there are no longer children. This is > happening with a tooltip, I didn't test other popup types. Well, it should be fixed, but maybe the patch didn't cover all possible manifestations of this bug or maybe you've found another bug. Do you have steps to reproduce? Also, what OS are you on?
I'm on Linux and was having trouble reproducing this, but being specific to FF17 and understanding the resizing issue described here I do this: 1. Create a tooltip which is taller than the screen. 2. Open it, preferably in a way that does not overlap the anchor and switch itself off) 3. Reconfigure the tooltip to be less high than the screen. 4. Open it again. Works fine when it is too tall, then opens with the big blank bottom border when there are fewer children. The children in my addon are actually three trees with a variable number of treerows. When the tooltip is too tall for the screen, it seems to be given a height attribute (and a width attribute!) by Firefox, and this then remains so that the tooltip keeps opening too tall.
Given that it looks like the tooltip has to be larger than the screen, I think this is a separate bug - would you mind filing a new one? (you could probably just clone this bug and go from there)
OK, I spun off bug 809388.