600844 - Site Panel does not close when clicking on Find in Page

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aakash Desai [:aakashd]]

Build Id:
Mozilla/5.0 (Maemo; Linux armv71; rv:2.0b6pre) Gecko/20100930 Namoroka/4.0b7pre Fennec/4.0b1pre

and

Mozilla/5.0 (Android; Linux armv71; rv:2.0b6pre) Gecko/20100930 Namoroka/4.0b7pre Fennec/4.0b1pre

Steps to Reproduce:
1. Click on the favicon left of the url bar
2. Click on "Find in Page"

Actual Results:
The site panel is not closed

Expected Results:
The site panel is closed.

Comment 1

[Matt Brubeck (:mbrubeck)]

Attachment: patch

This is a regression from bug 596619.  Here's a simple fix and a new regression test.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

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

>diff -r bedc4384560b chrome/content/browser-ui.js

>   show: function findHelperShow() {
>+    BrowserUI._hidePopup();
>     this._container.show(this);
>     this.search("");

I don't like managing the popups manually like this. I wonder if we should be just closing the Site Menu after any Page Action is fired? Do we ever want the Site Menu to stay open?

>diff -r bedc4384560b chrome/tests/browser_find.js

>+function test() {
>+  let menu = document.getElementById('identity-container');
>+  let item = document.getElementById('pageaction-findinpage');
>+  let navigator = document.getElementById('content-navigator');

Use "

Comment 3

[Matt Brubeck (:mbrubeck)]

(In reply to comment #2)
> I don't like managing the popups manually like this. I wonder if we should be
> just closing the Site Menu after any Page Action is fired? Do we ever want the
> Site Menu to stay open?

I'd be in favor of closing the site menu automatically on click.

The one case where we might want the site menu to stay open is for the "Add Search Engine" action, which modifies the menu in place.  If so, its click handler could use event.stopPropagation() to prevent its click from bubbling up to the menu.

I'll try this out...

Comment 4

[Matt Brubeck (:mbrubeck)]

Attachment: patch v2

(In reply to comment #2)
> I don't like managing the popups manually like this. I wonder if we should be
> just closing the Site Menu after any Page Action is fired? Do we ever want the
> Site Menu to stay open?

Updated the patch to close the site menu when any page action is clicked, and updated three page actions (Add Search Engine, Forget Password, Clear Permissions) to keep their current behavior of *not* closing the menu.

> Use "

Fixed.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 479889 [details] [diff] [review]
patch v2

Thanks Matt!

Comment 6

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/mobile-browser/rev/15f6e169870b

Comment 7

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Verified:
Mozilla/5.0 (Maemo;Linux armv71; rv:2.0b7pre)Gecko/20101001 Firefox/4.0b7pre Fennec/4.0b2pre

Mozilla/5.0 (Android; Linux armv71; rv2.0b7pre) Gecko/20101001 Firefox/4.0b7pre Fennec/4.0b2pre