1148716 - Suggestions in location bar cannot be hidden by clicking dropmarker again.

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Tooru Fujisawa [:arai]]

Steps To Reproduce:
  1. open Nightly with clean profile
  2. open new tab
  3. type following in location bar and hit enter
       https://bugzilla.mozilla.org/
  4. open new tab
  5. click dropmarker in location bar
  6. click dropmarker in location bar again

Expected Results:
  Suggestions are shown in step 5, and hidden in step 6

Actual Results:
  Suggestions are shown in step 5, and hidden only a moment and shown again in step 6

Last good revision: 02ab5234c39e
First bad revision: 203e26771e77
Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=02ab5234c39e&tochange=203e26771e77

Seems to be regression from bug 1089005.

Comment 1

[Tooru Fujisawa [:arai]]

this is so annoying.

Comment 2

[Tooru Fujisawa [:arai]]

so, the issue is that history-dropmarker always tries to open popup on mousedown.
we should hide the popup instead when the popup is already shown,

https://dxr.mozilla.org/mozilla-central/rev/8a494adbc5cced90a4edf0c98cffde906bf7f3ae/toolkit/content/widgets/autocomplete.xml#2317
>   <binding id="history-dropmarker" extends="chrome://global/content/bindings/general.xml#dropmarker">
> ...
>     <handlers>
>       <handler event="mousedown" button="0"><![CDATA[
>         this.showPopup();
>       ]]></handler>
>     </handlers>
>   </binding>

however, the popup is already hidden by mousedown, before the mousedown event is caught by history-dropmarker.
that means history-dropmarker should do nothing for the case.

we need a way to detect the popup is hidden by the exactly same mousedown event,
or perhaps catch the mousedown event before it hides the popup.

Comment 3

[Tooru Fujisawa [:arai]]

on OSX, popup is hidden before dispatching mousedown event

https://dxr.mozilla.org/mozilla-central/rev/8a494adbc5cced90a4edf0c98cffde906bf7f3ae/widget/cocoa/nsChildView.mm#4453
>   if ([self maybeRollup:theEvent] ||
>       !ChildViewMouseTracker::WindowAcceptsEvent([self window], theEvent, self, isClickThrough)) {
> ...
>   }
> ...
>   mGeckoChild->DispatchInputEvent(&geckoEvent);

so we cannot catch mousedown event before the popup gets hidden.

Comment 4

[Tooru Fujisawa [:arai]]

another tricky way is to check timestamp and avoid showing popup when the popup is hidden just before the mousedown,
but I'd like to avoid such thing... :/

Comment 5

[Tooru Fujisawa [:arai]]

maybe, we could disable opening popup until next event tick?

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Do not re-open location bar suggestion when dropmarker is clicked while the suggestion is opened.

added the following:
  * autocomplete-base-popup.mIsPopupHidingTick
      set to true on popuphiding event, and set back to false on next event tick (by setTimeout with 0)
  * autocomplete-base-popup.isPopupHidingTick
      accessor for mIsPopupHidingTick
  * autocomplete.showHistoryPopupFromDropMarker
      do nothing if this.popup.isPopupHidingTick is true
      otherwise call autocomplete.showHistoryPopup

With unpatched code, when the dropmarker is clicked (actually mousedown-ed) while the popup is opened,
the popup is hidden first, and the mousedown event handler is called, and the mousedown handler opens the popup again.

This patch changes dropmarker to call showHistoryPopupFromDropMarker instead of showHistoryPopup.
That does nothing if the mousedown event handler is called on the same event tick as the popup gets hidden, that means on the same mousedown.
So the popup isn't reopened by clicking dropmarker while the popup is opened.

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 8791470 [details] [diff] [review]
Do not re-open location bar suggestion when dropmarker is clicked while the suggestion is opened.

Review of attachment 8791470 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/widgets/autocomplete.xml
@@ +401,5 @@
>            this.mController.startSearch("");
>          ]]></body>
>        </method>
>  
> +      <method name="showHistoryPopupFromDropMarker">

I'd remove this additional method, and rather change toggleHistoryPopup to check:
if (!this.popup.isPopupHidingTick && !this.popup.popupOpen)
and then from the mousedown handler I'd invoke toggleHistoryPopup.

The cases where toggleHistoryPopup is invoked are quite rare and unlikely to clash with a popuphidden notification (indeed it's all cases where the user is likely to "toggle" the history popup than to mess up with the search text).

@@ +2329,5 @@
>      <implementation>
>        <method name="showPopup">
>          <body><![CDATA[
>            var textbox = document.getBindingParent(this);
> +          textbox.showHistoryPopupFromDropMarker();

The reason we have a showPopup method in this binding escapes me... Couldn't we just merge the code into the mousedown handler itself, and completely remove <implementation>?

And going further we likely don't need this binding at all, couldn't we just inline onmousedown in the xul:dropmarker declaration like
onmousedown="document.getBindingParent(this).toggleHistoryPopup();"
it should just be matter of removing this binding and this line
http://searchfox.org/mozilla-central/rev/c31ad35f39c6187b2e121aa6d3a39b7f67397010/toolkit/content/xul.css#892

what do you think?

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Do not re-open location bar suggestion when dropmarker is clicked while the suggestion is opened.

Thank you for reviewing :)

moved |!this.popup.isPopupHidingTick| to toggleHistoryPopup, and changed history-dropmarker to call it.

with onmousedown attribute, we should also check |event.button|, and I feel current "handler" definition is cleaner than putting them into attribute single line.
(and also I cannot find a way to make onmousedown attribute working...)
so just removed implementation, and moved it to handler.

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 8794413 [details] [diff] [review]
Do not re-open location bar suggestion when dropmarker is clicked while the suggestion is opened.

Review of attachment 8794413 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you!

::: toolkit/content/widgets/autocomplete.xml
@@ +405,5 @@
>        <method name="toggleHistoryPopup">
>          <body><![CDATA[
> +          // If this method is called on the same event tick as the popup gets
> +          // hidden, do nothing to avoid re-opening the popup when the drop
> +          // marker is clicked while the popup is still opened.

nit: s/opened/open/

Comment 11

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/74aff44c528af7a3ee1d4a35eb447f36e3e1520f
Bug 1148716 - Do not re-open location bar suggestion when dropmarker is clicked while the suggestion is opened. r=mak

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/74aff44c528a

Comment 17

[Mathew Hodson]

I believe bug 340839 is also a duplicate.