512006 - factorize the way we handle ESCAPE to close dialogs

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

The patch in bug 455866 add a new dialog, looking at the code, i have realized that all dialogs try to close on DOM_VK_ESCAPE and add an event listener for that.

This patch factorize that by assuming that each dialog which have registered through pushDialog implement a close method.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.1

Comment 2

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

Comment on attachment 395978 [details] [diff] [review]
Patch v0.1

>+            if (aEvent.target == this._edit) {
>+              this._edit.reset();
>+              this._editToolbar(false);
>+            }

Nice cleanup. The only part I'd like to go further with is moving this part into the binding itself.

Why?, try this: open the awesomebar and press the "^" button. Notice we do _not_ reset the previous title caption back into the URLBar. It's really a separate bug, but if we can fix it here, all the better.

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 396193 [details] [diff] [review]
Patch v0.2

I've also removed the URLBAR_EDIT && URLBAR_FORCE flags since they are a bit unuseful now.

Comment 5

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

Comment on attachment 396193 [details] [diff] [review]
Patch v0.2

>diff -r a1fd10fa40a7 chrome/content/bindings.xml

>       <method name="closePopup">

>           this.input.controller.stopSearch();
>+          this.input.reset();

I had to move the .reset into "close", since resetting in closePopup was messing up clicks in the awesomebar. The edit box was being reset and we lost the URL of the desired autocomplete item.

The close method is only called if we want to close the awesomebar without selecting an item.

Comment 6

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

pushed: https://hg.mozilla.org/mobile-browser/rev/0dae7553447c

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #5)
> (From update of attachment 396193 [details] [diff] [review])
> >diff -r a1fd10fa40a7 chrome/content/bindings.xml
> 
> >       <method name="closePopup">
> 
> >           this.input.controller.stopSearch();
> >+          this.input.reset();
> 
> I had to move the .reset into "close", since resetting in closePopup was
> messing up clicks in the awesomebar. The edit box was being reset and we lost
> the URL of the desired autocomplete item.
> 
> The close method is only called if we want to close the awesomebar without
> selecting an item.


my bad. sorry for that.

Comment 8

[Dão Gottwald [::dao]]

bugspam