987230 - Clicking the green text on the EV indicator a second time doesn't dismiss the EV information

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect)

Description

[alex_mayorga]

Steps:
- Load https://bugzilla.mozilla.org
- Click the green "Mozilla Foundation (US)" text to the left of the URL bar
- Click the green "Mozilla Foundation (US)" text to the left of the URL bar a second time

Result:
The EV information is shown again.

Expected result:
The EV information is dismissed.

Configuration:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0 ID:20140324030203 CSet: fa098f9fe89c

Initial report:
http://forums.mozillazine.org/viewtopic.php?p=13430297#p13430297

3dr party confirmation:
http://forums.mozillazine.org/viewtopic.php?p=13430315#p13430315

Comment 1

[Alice0775 White]

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/7e6cb031ed8f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131104081507
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/07ea7b11adef
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131104082308
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7e6cb031ed8f&tochange=07ea7b11adef

Regressed by:
07ea7b11adef	Neil Deakin — Bug 596723, Don't consume clicks outside of arrow panels by default, always consume the clicks on anchors of all popups, r=dao,neil

Comment 2

[Sylvestre Ledru [:Sylvestre]]

Not tracking. I don't think we would block a release because of this issue.

Comment 3

[Neil Deakin]

This is because the popup is being anchored to the lock icon image rather than the entire green indicator, so we only ignore mouse events when clicking the image.

The quick, although not quite correct fix, is to just add consumeoutsideclicks="false" to this panel.

Comment 4

[Neil Deakin]

The other possibility is to wait until the arrowpanel animations patch (bug 610545) is in, where this bug will get "fixed" since the mouse event will fire while the closing animation is still happening and prevent it from reopening.

Comment 5

[:Gijs (he/him)]

(In reply to Neil Deakin from comment #3)
> This is because the popup is being anchored to the lock icon image rather
> than the entire green indicator, so we only ignore mouse events when
> clicking the image.
> 
> The quick, although not quite correct fix, is to just add
> consumeoutsideclicks="false" to this panel.

We use the image of combined elements (e.g. toolbarbuttons) in lots of places. Can we ignore mouse events on the binding parent of the anchor? I believe that fixes most of the issues we're having with panels (the same currently happens to the main Firefox menu panel, the history panel, downloads panel, etc.).

(although, interestingly, it seems it wouldn't fix this particular case, because it's not using an anonymous content model :-( )


Could we communicate what the anchor should be somehow? Either by adding a parameter to the openPopup call, or an attribute on the panel, or an attribute on the anchor of the panel? Which of these would you prefer from an API design perspective?

Comment 6

[Justin Dolske [:Dolske]]

Not tracking this as part of Australis.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

This patch seems to fix both this bug, as well as bug 953158.

There might be unintended consequences with other popup types though - I'm curious to know what Neil things.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8407758 [details] [diff] [review]
Patch v1

Or maybe other Neil can look at this, since I think Neil Deakin is about to go on vacation.

Either / or.

Comment 9

[[e-mail]]

So what you're saying is that you want clicks on a containing element to open a popup but anchor it to a contained element, and that the click on the container is therefore reopening the popup?

Comment 10

[Mike Conley (:mconley) (:⚙️)]

(In reply to [e-mail] from comment #9)
> So what you're saying is that you want clicks on a containing element to
> open a popup but anchor it to a contained element, and that the click on the
> container is therefore reopening the popup?

Correct.

Comment 11

[[e-mail]]

I don't know if it would be useful in your case, but we could probably make the popup suppress mouse events on its parent node. This would mainly only work for menubuttons and the like, although maybe it could be made to work for other popups by making them the child of their trigger node.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

(In reply to [e-mail] from comment #11)
> I don't know if it would be useful in your case, but we could probably make
> the popup suppress mouse events on its parent node. This would mainly only
> work for menubuttons and the like, although maybe it could be made to work
> for other popups by making them the child of their trigger node.

Yeah, wouldn't work for the anchor-button + arrowpanel case, which is the one this and bug 953158 are concerned with. :/

My patch here seems to solve both this and bug 953158. Is my solution worth pursuing?

Comment 13

[[e-mail]]

Comment on attachment 8407758 [details] [diff] [review]
Patch v1

I can think of a case where this would be a minor regression.

Thunderbird's filter editor has a listbox with listitems that are XBL bindings that contain elements with popups. (It's possible that other chrome does this too but that was the first one to come to mind.) I wouldn't want to ignore clicks on the whole listitem just because one of its popups was open.

Comment 14

[:Gijs (he/him)]

I actually can't reproduce this on either OS X or Windows, but can on Linux. :-\

Comment 15

[:Gijs (he/him)]

Attachment: use rollupanchor attribute to fix button click,

Comment 16

[:Gijs (he/him)]

Comment on attachment 8449365 [details] [diff] [review]
use rollupanchor attribute to fix button click,

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

Mike, can you look at the browser + panelUI + downloads bits?

Comment 17

[Marco Mucci [:MarcoM]]

Added to Iteration 33.2

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8449365 [details] [diff] [review]
use rollupanchor attribute to fix button click,

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

Kinda strange to have an anchor for an anchor, but I get the drift of this. We'll want to update the popup docs - we should set dev-doc-needed.

Otherwise, the browser and toolkit bits look solid.

Comment 19

[Neil Deakin]

Comment on attachment 8449365 [details] [diff] [review]
use rollupanchor attribute to fix button click,

>+      // Check if the anchor has indicated another node to use for checking
>+      // for roll-up. That way, we can anchor a popup on anonymous content or
>+      // an individual icon, while clicking elsewhere within a button or other
>+      // container doesn't result in us re-opening the popup.
>+      if (anchor) {
>+        nsAutoString rollupAnchor;
>+        anchor->GetAttr(kNameSpaceID_None, nsGkAtoms::rollupanchor,
>+                        rollupAnchor);

This attribute affects whether the event will be consumed and the popup always rolls up, so I think it would be clearer to call this 'consumeanchor'.


>+        if (!rollupAnchor.IsEmpty()) {
>+          nsIDocument* doc = anchor->GetOwnerDocument();
>+          nsIContent* newAnchor = doc->GetElementById(rollupAnchor);
>+          if (newAnchor) {
>+            anchor = newAnchor;
>+          }
>+        }
>     <content>
>       <children includes="observes|template|menupopup|panel|tooltip"/>
>-      <xul:image class="toolbarbutton-icon" xbl:inherits="validate,src=image,label"/>
>+      <xul:image class="toolbarbutton-icon" xbl:inherits="validate,src=image,label,rollupanchor"/>

You could also just put the new attribute on the popup instead and not have to inherit the value. Although I think what you've done is better here. For example, if two popups are attached to the same anchor, this will affect both. I just wanted to make sure that this was intentional.

Comment 20

[Neil Deakin]

Comment on attachment 8407758 [details] [diff] [review]
Patch v1

Don't need this, but the new patch is based on this for some reason and just undoes it all.

Comment 21

[:Gijs (he/him)]

(In reply to Neil Deakin from comment #19)
> Comment on attachment 8449365 [details] [diff] [review]
> use rollupanchor attribute to fix button click,
> 
> >+      // Check if the anchor has indicated another node to use for checking
> >+      // for roll-up. That way, we can anchor a popup on anonymous content or
> >+      // an individual icon, while clicking elsewhere within a button or other
> >+      // container doesn't result in us re-opening the popup.
> >+      if (anchor) {
> >+        nsAutoString rollupAnchor;
> >+        anchor->GetAttr(kNameSpaceID_None, nsGkAtoms::rollupanchor,
> >+                        rollupAnchor);
> 
> This attribute affects whether the event will be consumed and the popup
> always rolls up, so I think it would be clearer to call this 'consumeanchor'.

OK. I'll rename stuff before pushing.

> >+        if (!rollupAnchor.IsEmpty()) {
> >+          nsIDocument* doc = anchor->GetOwnerDocument();
> >+          nsIContent* newAnchor = doc->GetElementById(rollupAnchor);
> >+          if (newAnchor) {
> >+            anchor = newAnchor;
> >+          }
> >+        }
> >     <content>
> >       <children includes="observes|template|menupopup|panel|tooltip"/>
> >-      <xul:image class="toolbarbutton-icon" xbl:inherits="validate,src=image,label"/>
> >+      <xul:image class="toolbarbutton-icon" xbl:inherits="validate,src=image,label,rollupanchor"/>
> 
> You could also just put the new attribute on the popup instead and not have
> to inherit the value. Although I think what you've done is better here. For
> example, if two popups are attached to the same anchor, this will affect
> both. I just wanted to make sure that this was intentional.

Yes, this was intentional - although I thought of the inverse, ie one popup that could be used with multiple anchors...

Comment 22

[:Gijs (he/him)]

(In reply to Neil Deakin from comment #20)
> Comment on attachment 8407758 [details] [diff] [review]
> Patch v1
> 
> Don't need this, but the new patch is based on this for some reason and just
> undoes it all.

Yeah, this is because I messed up my mq fu. I'll fix before landing.

Comment 23

[:Gijs (he/him)]

With the rename, https://hg.mozilla.org/integration/fx-team/rev/21423398898b

Setting leave-open because I think I should write a test.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/21423398898b

Comment 25

[:Gijs (he/him)]

Attachment: test consumeanchor behaviour,

Test. try push: remote:   https://tbpl.mozilla.org/?tree=Try&rev=3bec85117205

Comment 26

[:Gijs (he/him)]

Attachment: test consumeanchor behaviour,

Well, that was dumb. New try push, too: remote:   https://tbpl.mozilla.org/?tree=Try&rev=308a20498f99

Comment 27

[Neil Deakin]

Comment on attachment 8453368 [details] [diff] [review]
test consumeanchor behaviour,


This test passes on my Mac even though I don't have the rollupanchor patch applied. It should be written to fail.


+  <popupset>
+    <popup id="mypopup"
+           onpopupshown="onMyPopupShown(event)"
+           onpopuphidden="onMyPopupHidden(event)">This is a test popup</popup>
+  </popupset>

'popup' is deprecated so you should use menupopup instead. And popupset doesn't do anything so you should just remove it.


+  let isMac = ("nsILocalFileMac" in Ci);
+  let isLinux = ("@mozilla.org/gnome-gconf-service;1" in Cc);
+  let isWindows = ("@mozilla.org/windows-registry-key;1" in Cc); 

You only use the isWindows variable, and you should just use navigator.platform rather than this weird component-checking thing.

+
+  function synthesizeNativeMouseLDown(aElement, aOffsetX, aOffsetY) {
+    let msg = isWindows ? 2 : 1;
+    synthesizeNativeMouseEvent(aElement, aOffsetX, aOffsetY, msg);
+  }
+
+  /**
+  * Fires a synthetic 'mouseup' event on the current about:newtab page.
+  * @param aElement The element used to determine the cursor position.
+  */

Remove this comment which I assume you copied from somewhere else.

Comment 28

[:Gijs (he/him)]

Per discussion in the iteration meeting, let's split off the test - bug 1042092.

Comment 29

[Marco Mucci [:MarcoM]]

Hi Florin, can a QA contact be assigned for verification of this bug.

Comment 30

[Petruta Horea [:phorea], Desktop QA]

Attachment: Screencast with the issue reproduced on Mac OSX.

Comment 31

[Petruta Horea [:phorea], Desktop QA]

Verified as fixed on Aurora 33.0a2 and Nightly 34.0a1 2014-07-23, the EV information is dismissed when it is selected for the second time.
I'm setting the flags too, as Gijs advised on IRC.

I encountered another issue during verification: clicking a few pixels outside the information panel doesn't close it. I could reproduce it back to Firefox 24 so it's not related to this bug. It also reproduces with other information panels. Should I file a new bug for it (I couldn't find one already logged).