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

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: arai, Assigned: arai)

Tracking

({regression})

Trunk
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

4 years ago
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.

Updated

4 years ago
Keywords: regression

Updated

4 years ago
See Also: → 1198156

Updated

4 years ago
Blocks: 1199875
Assignee

Updated

3 years ago
See Also: → 1285776
Assignee

Comment 1

3 years ago
this is so annoying.
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Assignee

Comment 2

3 years ago
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.
Assignee

Updated

3 years ago
Component: Event Handling → XUL Widgets
Product: Core → Toolkit
Assignee

Comment 3

3 years ago
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.
Assignee

Comment 4

3 years ago
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... :/
Assignee

Comment 5

3 years ago
maybe, we could disable opening popup until next event tick?
Assignee

Comment 6

3 years ago
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
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?
Attachment #8791470 - Flags: review?(mak77) → feedback+
Duplicate of this bug: 1305108
Assignee

Comment 9

3 years ago
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+
Assignee

Comment 11

3 years ago
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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/74aff44c528a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52

Updated

3 years ago
See Also: → 1251569

Updated

3 years ago
No longer blocks: 1199875
Duplicate of this bug: 1199875
Duplicate of this bug: 1285776

Updated

3 years ago
Duplicate of this bug: 1303771

Updated

3 years ago
Duplicate of this bug: 1312285

Comment 17

3 years ago
I believe bug 340839 is also a duplicate.

Updated

3 years ago
See Also: → 1328045
Duplicate of this bug: 1297942
You need to log in before you can comment on or make changes to this bug.