Closed Bug 987230 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Address Bar, defect)

28 Branch
x86_64
Windows 7
defect
Not set
Points:
5

Tracking

()

VERIFIED FIXED
Firefox 33
Iteration:
34.1
Tracking Status
firefox28 --- wontfix
firefox29 - wontfix
firefox30 - wontfix
firefox31 - affected
firefox32 --- affected
firefox33 --- verified
firefox34 --- verified
firefox-esr24 --- unaffected

People

(Reporter: alex_mayorga, Assigned: Gijs)

References

()

Details

(Keywords: dev-doc-needed, regression, Whiteboard: [Australis:P-])

Attachments

(4 files, 1 obsolete file)

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
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
Blocks: 596723
Keywords: regression
Version: Trunk → 28 Branch
Not tracking. I don't think we would block a release because of this issue.
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.
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.
(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?
Flags: needinfo?(enndeakin)
Not tracking this as part of Australis.
Whiteboard: [Australis:P-]
Flags: firefox-backlog? → firefox-backlog+
Whiteboard: [Australis:P-] → [Australis:P-] p=0
Attached patch Patch v1Splinter Review
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.
Attachment #8407758 - Flags: review?(enndeakin)
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.
Attachment #8407758 - Flags: review?(neil)
No longer blocks: fxdesktopbacklog
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?
(In reply to neil@parkwaycc.co.uk 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.
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.
(In reply to neil@parkwaycc.co.uk 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?
Flags: needinfo?(neil)
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.
Attachment #8407758 - Flags: review?(neil)
Flags: needinfo?(neil)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(enndeakin)
I actually can't reproduce this on either OS X or Windows, but can on Linux. :-\
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
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?
Attachment #8449365 - Flags: review?(mconley)
Flags: needinfo?(gijskruitbosch+bugs)
Added to Iteration 33.2
Iteration: --- → 33.2
QA Whiteboard: [qa?]
Whiteboard: [Australis:P-] p=0 → [Australis:P-]
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.
Attachment #8449365 - Flags: review?(mconley) → review+
Points: --- → 5
QA Whiteboard: [qa?] → [qa+]
Iteration: 33.2 → 33.3
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.
Attachment #8449365 - Flags: review?(neil) → review+
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.
Attachment #8407758 - Flags: review?(enndeakin)
(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...
(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.
With the rename, https://hg.mozilla.org/integration/fx-team/rev/21423398898b

Setting leave-open because I think I should write a test.
Attached patch test consumeanchor behaviour, (obsolete) — Splinter Review
Test. try push: remote:   https://tbpl.mozilla.org/?tree=Try&rev=3bec85117205
Attachment #8453356 - Flags: review?(enndeakin)
Well, that was dumb. New try push, too: remote:   https://tbpl.mozilla.org/?tree=Try&rev=308a20498f99
Attachment #8453368 - Flags: review?(enndeakin)
Attachment #8453356 - Attachment is obsolete: true
Attachment #8453356 - Flags: review?(enndeakin)
Attachment #8449365 - Flags: checkin+
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.
Attachment #8453368 - Flags: review?(enndeakin) → review-
Iteration: 33.3 → 34.1
Depends on: 1042092
Per discussion in the iteration meeting, let's split off the test - bug 1042092.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: leave-open
Resolution: --- → FIXED
Hi Florin, can a QA contact be assigned for verification of this bug.
Flags: needinfo?(florin.mezei)
Flags: needinfo?(florin.mezei)
QA Contact: petruta.rasa
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).
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa+] → [qa!]
Target Milestone: --- → Firefox 33
Duplicate of this bug: 1042828
You need to log in before you can comment on or make changes to this bug.