701760 - Panel created with Jetpack "panel" API won't close on loss of focus if drop-down menu has focus

Status: RESOLVED FIXED

(Core :: Widget, defect, P1)

Description

[John Nagle]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0.1) Gecko/20100101 Firefox/7.0.1
Build ID: 20110928134238

Steps to reproduce:

1.  Run example add-on  "resize_panel_2", from "https://builder.addons.mozilla.org/user/jetpackexamples/"

2.  That brings up a Reddit page in a panel. By clicking on various links, you can eventually get to a page with ads, which provides a path off Reddit. Find a page with a form containing a drop-down menu. I reached the GameStop shopping cart checkout page, which has drop-down menus for countries and states. 

3.  Click on a drop-down menu.  Then click outside the panel.  


Actual results:

The panel remains visible and on top.

If one of the drop-down menus has focus, the panel will not close on a click outside the window.  It's possible to get two panels from different add-ons open at the same time in this way. That's not supposed to happen.


Expected results:

The panel should have been hidden by any out-of-panel click. 

See "https://forums.mozilla.org/addons/viewtopic.php?f=27&t=4006&p=13076#p13076" for a discussion by the user who discovered this bug.

Until this is fixed, all Jetpack add-ons with panels with forms with drop-down menus should have a "Cancel" button, as a workaround. There probably aren't many of those.

Comment 1

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Gabor, can you see what's going on here, and if there's a fix for it?

Comment 2

[Dietrich Ayala (:dietrich)]

reporter demonstrated this in real life.

when an html select element in web content inside a panel has focus, clicking *outside* the panel does not close the panel.

cc'ing Neil, who implemented <xul:panel>.

Comment 3

[Hernán Rodriguez Colmeiro (:peregrino)]

(In reply to Dietrich Ayala (:dietrich) from comment #2)
> when an html select element in web content inside a panel has focus,
> clicking *outside* the panel does not close the panel.

Is not just having focus, is just clicking it, as calling .blur() on the focused element isn't a valid workaround. Once you click on a select, the only way to hide the panel is to hit the "Esc" key or call panel.hide() from the main code.

Comment 4

[Neil Deakin]

Attachment: patch

This is because the popup widgets for <select> elements aren't tracked by the popup manager, so when one opens, it cancels out being able to close any other panels.

This patch simplifies the rollup handling, and also determines which popup to rollup when it is time to close it, rather than when it opens, allowing the right popup to close to be selected.

Comment 5

[Gabor Krizsanits (INACTIVE)]

I figured out the reason but it would have take me a while to fix this... So thanks for the help. Who is going to review this patch by the way?

Comment 6

[Neil Deakin]

I'd want to test the original testcase first. I couldn't get the test described in the first comment to work, so I don't know if the patch fixes the specific bug, but I assume it does. I get a 'wrong hashtag' error when running the steps above. Is there an updated test?

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Neil Deakin from comment #6)
> I get a 'wrong hashtag' error when
> running the steps above. Is there an updated test?

I think "wrong hashtag" means "this addon is using a version of the SDK that no longer exists on Builder, so I can't create the xpi to send to your computer". The example addon in comment 0 was built using SDK 1.2.1, and was never updated.

I copied the addon and repacked it with SDK 1.9 and it's available here: https://builder.addons.mozilla.org/package/153252/latest/


I can get a smaller testcase in a few minutes, though.

Comment 8

[Gabor Krizsanits (INACTIVE)]

(In reply to Neil Deakin from comment #6)
> I'd want to test the original testcase first. I couldn't get the test
> described in the first comment to work, so I don't know if the patch fixes
> the specific bug, but I assume it does. I get a 'wrong hashtag' error when
> running the steps above. Is there an updated test?

You can change the url in the addon to contentURL: data.url("mypage.html"), and add a mypage.html to the data folder of the addon that has some dropdown widget like:
<select>
    <option></option>
</select>
And then you should be able to reproduce it.

I have not looked into your patch yet, I think the problem was that both nsComboboxControlFrame and nsXULPopupManager using the same nsWindow::sRollupWidget without knowing about each other. So when the combo box rolls down the panel is cleared from sRollupWidget and never set back.

Comment 9

[Dave Townsend [:mossop]]

I copied the original example and simplified it a little, also pointed it at this bug report so there are select boxes immediately available: https://builder.addons.mozilla.org/package/153254/latest/

Comment 10

[Neil Deakin]

Nope, neither of those tests work for me. But the description of the problem given in comment 8 is correct.

Comment 11

[Gabor Krizsanits (INACTIVE)]

(In reply to Neil Deakin from comment #10)
> Nope, neither of those tests work for me. But the description of the problem
> given in comment 8 is correct.

You mean you cann't get them to work, or that the tests are failing? Anyway, I'll give it a try and test your patch.

Comment 12

[Gabor Krizsanits (INACTIVE)]

Yes, it did fix the bug! Yaay! By the way, next time please let me know if you want to take a bug over from me, so I don't waste time on it in the meanwhile. But I kind of assume that you worked on this issue before knew about this bug report, or just didn't see it is assigned to me, in which case nevermind :) And thanks for fixing it anyway.

Comment 13

[Neil Deakin]

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

This patch fixes the bug by determining the rollup widget on demand from nsIRollupListener instead of caching it. It also does the same from the consume outside clicks option.

Comment 14

[Mats Palmgren (inactive)]

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

Did you forget to update widget/os2/ ?

> layout/forms/nsComboboxControlFrame.cpp
>+  NS_ASSERTION(view, "nsComboboxControlFrame view is null");
>+  return view ? view->GetWidget() : nullptr;

I don't see why we need to both assert and handle it.  I prefer this:

  MOZ_ASSERT(view);
  return view->GetWidget();

>widget/cocoa/nsAppShell.mm
>-    if (gRollupListener && gRollupWidget)
>-      gRollupListener->Rollup(0);
>+    nsIRollupListener* rollupListener = nsBaseWidget::GetActiveRollupListener();
>+    nsIWidget* rollupWidget = rollupListener->GetRollupWidget();
>+    if (rollupWidget)
>+      rollupListener->Rollup(0, nullptr);

There's a subtle ownership difference here - the old code holds strong
ref on the widget (gRollupWidget) while the Rollup call executes, the
new code doesn't.  I'd like all GetRollupWidget consumers to use
nsCOMPtr<nsIWidget> for the result.

>widget/gtk2/nsWindow.cpp
> check_for_rollup(gdouble aMouseX, gdouble aMouseY,
>                  bool aIsWheel, bool aAlwaysRollup)
> ...
>-    } else {
>-        gRollupWindow = nullptr;
>-        gRollupListener = nullptr;
>     }

Shouldn't you keep this else-branch and just remove the gRollupWindow
line?  (same question for widget/qt/nsWindow.cpp)

>widget/nsIWidget.h
>+    NS_IMETHOD CaptureRollupEvents(nsIRollupListener * aListener, ...

Nit: make that "nsIRollupListener* aListener" while you're here.

>widget/qt/nsWindow.cpp
> nsWindow::CaptureRollupEvents(nsIRollupListener *aListener,
>...
>-    if (aDoCapture) {
>-        gConsumeRollupEvent = aConsumeRollupEvent;
>-        gRollupListener = aListener;
>-        gRollupWindow = do_GetWeakReference(static_cast<nsIWidget*>(this));
>-    }
>-    else {
>-        gRollupListener = nullptr;
>-        gRollupWindow = nullptr;
>-    }
>-
>+    gRollupListener = aListener;

Shouldn't that be "gRollupListener = aDoCapture ? aListener : nullptr;" ?

Comment 15

[Neil Deakin]

Attachment: Updated patch

Comment 16

[Neil Deakin]

> Did you forget to update widget/os2/ ?

I didn't forget. I don't think we're required to update os2. However, I changed the code as best I could here.

Comment 17

[Mats Palmgren (inactive)]

Comment on attachment 675167 [details] [diff] [review]
Updated patch

> widget/nsIRollupListener.h

Remove these two lines:
    * If aGetLastRolledUp is true, then return the last rolled up popup,
    * if this is supported.

> widget/os2/nsWindow.cpp
> +  nsIWidget* rollupWidget = rollupListener->GetRollupWidget();

make that nsCOMPtr<nsIWidget> for consistency (3 occurrences)

> widget/os2/os2FrameWindow.cpp

ditto (1 occurrence)

r=mats with those nits

Comment 18

[Mats Palmgren (inactive)]

(In reply to Neil Deakin from comment #16)
> > Did you forget to update widget/os2/ ?
> 
> I didn't forget. I don't think we're required to update os2. However, I
> changed the code as best I could here.

Thanks.  I don't know to what extent we're required to maintain
os2, qt etc.  In this case I suppose the files wouldn't compile so it
would be noticed and fixed eventually, but for other changes it seems
dangerous to just leave them as is.  Perhaps the minimum requirement
should be to add a line like so:
#error "needs to be updated for bug 701760"
to give maintainers a heads-up?

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6a9691cfc118

Comment 21

[ashishsvkumar]

I am facing the same issue on firefox 17.0.1 . Will this be resolved for F17 ?

Comment 22

[Mike Kaply [:mkaply]]

Could this have caused regression bug 862330?

Comment 23

[Mike Kaply [:mkaply]]

Am I reading this wrong, or was the entire concept of consuming rollup events removed?

Comment 24

[Mats Palmgren (inactive)]

(In reply to Michael Kaply (mkaply) from comment #23)
> Am I reading this wrong, or was the entire concept of consuming rollup
> events removed?

No, it changed from being a param of CaptureRollupEvents() to being the return
value of Rollup().  Other than that it should work the same IUIC.