popup not shrinking when its children are removed (linux)

RESOLVED FIXED in Firefox 17

Status

()

Toolkit
XUL Widgets
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: keeler, Assigned: keeler)

Tracking

Trunk
mozilla18
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox16 unaffected, firefox17+ fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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 658640 [details]
osx.png
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).
Attachment #660261 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Blocks: 793338
Blocks: 357725
Blocks: 793487
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.
Attachment #660628 - Flags: review?(enndeakin)
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED
Version: unspecified → Trunk
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.
Attachment #660628 - Flags: review?(enndeakin) → review+
Try run: https://tbpl.mozilla.org/?tree=Try&rev=956fcf03f94c
Marking checkin-needed.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/54079ce3e836
Flags: in-testsuite?
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/54079ce3e836
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
No longer blocks: 793487
Duplicate of this bug: 793487
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
Attachment #660628 - Flags: approval-mozilla-beta?

Updated

5 years ago
status-firefox16: --- → unaffected
status-firefox17: --- → affected
tracking-firefox17: --- → ?

Updated

5 years ago
Duplicate of this bug: 793487

Updated

5 years ago
tracking-firefox17: ? → +
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.
Attachment #660628 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/983db048d2f2
status-firefox17: affected → fixed

Comment 16

5 years ago
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?

Comment 18

5 years ago
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)

Comment 20

5 years ago
OK, I spun off bug 809388.
You need to log in before you can comment on or make changes to this bug.