Last Comment Bug 788829 - popup not shrinking when its children are removed (linux)
: popup not shrinking when its children are removed (linux)
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: XUL Widgets (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla18
Assigned To: David Keeler [:keeler] (use needinfo?)
:
: Neil Deakin
Mentors:
: 793487 (view as bug list)
Depends on:
Blocks: 357725 754472 793338
  Show dependency treegraph
 
Reported: 2012-09-05 15:01 PDT by David Keeler [:keeler] (use needinfo?)
Modified: 2012-11-08 03:57 PST (History)
10 users (show)
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
fixed


Attachments
linux.png (115.01 KB, image/png)
2012-09-05 15:01 PDT, David Keeler [:keeler] (use needinfo?)
no flags Details
osx.png (170.58 KB, image/png)
2012-09-05 15:01 PDT, David Keeler [:keeler] (use needinfo?)
no flags Details
patch (891 bytes, patch)
2012-09-11 16:54 PDT, David Keeler [:keeler] (use needinfo?)
no flags Details | Diff | Splinter Review
patch (1.59 KB, patch)
2012-09-12 17:18 PDT, David Keeler [:keeler] (use needinfo?)
enndeakin: review+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description David Keeler [:keeler] (use needinfo?) 2012-09-05 15:01:21 PDT
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.
Comment 1 David Keeler [:keeler] (use needinfo?) 2012-09-05 15:01:48 PDT
Created attachment 658640 [details]
osx.png
Comment 2 David Keeler [:keeler] (use needinfo?) 2012-09-11 16:54:47 PDT
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).
Comment 3 David Keeler [:keeler] (use needinfo?) 2012-09-12 17:18:09 PDT
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).
Comment 4 Neil Deakin 2012-09-26 12:45:51 PDT
David, did you want someone to review this?
Comment 5 David Keeler [:keeler] (use needinfo?) 2012-09-26 12:48:52 PDT
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 6 Jared Wein [:jaws] (please needinfo? me) 2012-09-28 17:03:47 PDT
Comment on attachment 660628 [details] [diff] [review]
patch

Marking for review based on the above comment.
Comment 7 Neil Deakin 2012-09-29 14:11:41 PDT
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.
Comment 8 David Keeler [:keeler] (use needinfo?) 2012-10-07 11:03:47 PDT
Try run: https://tbpl.mozilla.org/?tree=Try&rev=956fcf03f94c
Marking checkin-needed.
Comment 9 Ryan VanderMeulen [:RyanVM] 2012-10-07 17:56:39 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/54079ce3e836
Comment 10 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-08 01:32:55 PDT
https://hg.mozilla.org/mozilla-central/rev/54079ce3e836
Comment 11 Karl Tomlinson (back Dec 13 :karlt) 2012-10-09 18:03:44 PDT
*** Bug 793487 has been marked as a duplicate of this bug. ***
Comment 12 Marco Bonardo [::mak] 2012-10-11 02:54:19 PDT
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 13 Marco Bonardo [::mak] 2012-10-11 02:56:17 PDT
*** Bug 793487 has been marked as a duplicate of this bug. ***
Comment 14 Alex Keybl [:akeybl] 2012-10-12 14:05:59 PDT
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.
Comment 15 Marco Bonardo [::mak] 2012-10-15 05:47:44 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/983db048d2f2
Comment 16 Ian Nartowicz 2012-11-06 04:28:06 PST
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.
Comment 17 David Keeler [:keeler] (use needinfo?) 2012-11-06 14:25:37 PST
(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?
Comment 18 Ian Nartowicz 2012-11-06 14:32:56 PST
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.
Comment 19 David Keeler [:keeler] (use needinfo?) 2012-11-06 14:38:21 PST
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)
Comment 20 Ian Nartowicz 2012-11-08 03:57:18 PST
OK, I spun off bug 809388.

Note You need to log in before you can comment on or make changes to this bug.