Closed Bug 701760 Opened 13 years ago Closed 12 years ago

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

Categories

(Core :: Widget, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: nagle, Assigned: enndeakin)

References

Details

Attachments

(1 file, 1 obsolete file)

91.17 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
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.
Gabor, can you see what's going on here, and if there's a fix for it?
Assignee: nobody → gkrizsanits
Priority: -- → P2
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>.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: P2 → --
Whiteboard: [triage:followup]
(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.
Priority: -- → P2
Priority: P2 → --
Priority: -- → P1
Whiteboard: [triage:followup]
Assignee: gkrizsanits → nobody
Status: NEW → ASSIGNED
Component: General → Widget
Product: Add-on SDK → Core
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → enndeakin
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?
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?
(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.
(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.
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/
Nope, neither of those tests work for me. But the description of the problem given in comment 8 is correct.
(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.
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 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.
Attachment #662176 - Flags: review?(matspal)
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;" ?
Attachment #662176 - Flags: review?(matspal) → review-
Attached patch Updated patchSplinter Review
Attachment #662176 - Attachment is obsolete: true
Attachment #675167 - Flags: review?(matspal)
> 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 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
Attachment #675167 - Flags: review?(matspal) → review+
(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?
https://hg.mozilla.org/mozilla-central/rev/6a9691cfc118
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
I am facing the same issue on firefox 17.0.1 . Will this be resolved for F17 ?
Could this have caused regression bug 862330?
Am I reading this wrong, or was the entire concept of consuming rollup events removed?
Depends on: 862330
(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.
You need to log in before you can comment on or make changes to this bug.