788829 - popup not shrinking when its children are removed (linux)

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Dana Keeler (she/her) [:keeler]]

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

[Dana Keeler (she/her) [:keeler]]

Attachment: osx.png

Comment 2

[Dana Keeler (she/her) [:keeler]]

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

[Dana Keeler (she/her) [:keeler]]

Attachment: 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]

David, did you want someone to review this?

Comment 5

[Dana Keeler (she/her) [:keeler]]

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

Comment on attachment 660628 [details] [diff] [review]
patch

Marking for review based on the above comment.

Comment 7

[Neil Deakin]

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

[Dana Keeler (she/her) [:keeler]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=956fcf03f94c
Marking checkin-needed.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/54079ce3e836

Comment 10

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/54079ce3e836

Comment 12

[Marco Bonardo [:mak]]

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 14

[Alex Keybl [:akeybl]]

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

https://hg.mozilla.org/releases/mozilla-beta/rev/983db048d2f2

Comment 16

[Ian Nartowicz]

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

[Dana Keeler (she/her) [:keeler]]

(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]

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

[Dana Keeler (she/her) [:keeler]]

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]

OK, I spun off bug 809388.