Closed Bug 1148716 Opened 7 years ago Closed 6 years ago

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


(Toolkit :: XUL Widgets, defect)

Not set



Tracking Status
firefox52 --- fixed


(Reporter: arai, Assigned: arai)



(Keywords: regression)


(1 file, 1 obsolete file)

Steps To Reproduce:
  1. open Nightly with clean profile
  2. open new tab
  3. type following in location bar and hit enter
  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

Seems to be regression from bug 1089005.
Keywords: regression
See Also: → 1198156
Blocks: 1199875
See Also: → 1285776
this is so annoying.
Assignee: nobody → arai.unmht
OS: Mac OS X → All
Hardware: x86 → All
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,
>   <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.
Component: Event Handling → XUL Widgets
Product: Core → Toolkit
on OSX, popup is hidden before dispatching mousedown event
>   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.
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... :/
maybe, we could disable opening popup until next event tick?
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.
Attachment #8791470 - Flags: review?(mak77)
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
it should just be matter of removing this binding and this line

what do you think?
Attachment #8791470 - Flags: review?(mak77) → feedback+
Duplicate of this bug: 1305108
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.
Attachment #8791470 - Attachment is obsolete: true
Attachment #8794413 - Flags: review?(mak77)
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/
Attachment #8794413 - Flags: review?(mak77) → review+
Bug 1148716 - Do not re-open location bar suggestion when dropmarker is clicked while the suggestion is opened. r=mak
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
See Also: → 1251569
No longer blocks: 1199875
Duplicate of this bug: 1199875
Duplicate of this bug: 1285776
Duplicate of this bug: 1303771
Duplicate of this bug: 1312285
I believe bug 340839 is also a duplicate.
See Also: → 1328045
Duplicate of this bug: 1297942
You need to log in before you can comment on or make changes to this bug.