Closed Bug 1325841 Opened 7 years ago Closed 7 years ago

Popup blocked icon in url bar does not disappear even if the site is moved

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Version:
49 Branch
Type:
defect
Priority:
Not set
Severity:
minor

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox50 --- wontfix
firefox51 - wontfix
firefox52 - wontfix
firefox-esr52 --- verified
firefox53 + verified
firefox54 --- verified
firefox55 --- verified

People

(Reporter: euthanasia_waltz, Assigned: Gijs)

References

Details

(Keywords: regression)

Attachments

(2 files)

Bug 1325841 - fix hiding popup icon when navigating the browser,
59 bytes, text/x-review-board-request
Felipe
: review+
jcristau
: approval-mozilla-aurora+
lizzard
: approval-mozilla-beta+
jcristau
: approval-mozilla-esr52+
Details
pop-up blocker.mp4
1.70 MB, video/mp4
Details 
STR:
1. Open any web page having popup to be blocked. (or local html file with <body onload="window.open('https://www.google.com/')">)
2. Make sure popup blocked icon appears in url bar.
3. Move to any other site.
ER:
The icon disappears.
AR:
The icon stays appearing.

mozregression:
 7:09.17 INFO: Last good revision: 8682f94befeb413bafc9e434d3cb387b1ce567d5
 7:09.17 INFO: First bad revision: 3a87296fe4145138c2ce15512bb31f76fe869cb4
 7:09.17 INFO: Pushlog:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=8682f94befeb413bafc9e434d3cb387b1ce567d5&tochange=3a87296fe4145138c2ce15512bb31f76fe869cb4

 7:10.18 INFO: Looks like the following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1273685
Blocks: 1273685
Status: UNCONFIRMED → NEW
status-firefox50: --- → affected
status-firefox51: --- → affected
status-firefox52: --- → affected
status-firefox53: --- → affected
tracking-firefox51: --- → ?
tracking-firefox52: --- → ?
tracking-firefox53: --- → ?
Ever confirmed: true
Flags: needinfo?(felipc)
Keywords: regression
Version: 50 Branch → 49 Branch
Regression since Firefox 49 - I'm marking this fix-optional for 51 and 52 though if we get a simple fix, we could still uplift it.
status-firefox51: affected → fix-optional
status-firefox52: affected → fix-optional
tracking-firefox51: ? → -
tracking-firefox52: ? → -
tracking-firefox53: ? → +
Neil, can you help find someone to fix this regression? It sounds fairly annoying. Thanks.
Flags: needinfo?(enndeakin)
I don't know anything about this.
Flags: needinfo?(enndeakin)
I'll see if I can find time tomorrow to have a look.
Flags: needinfo?(gijskruitbosch+bugs)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(felipc)
Comment on attachment 8849580 [details]
Bug 1325841 - fix hiding popup icon when navigating the browser,

https://reviewboard.mozilla.org/r/122362/#review125104

::: browser/base/content/browser.js:577
(Diff revision 1)
>  
>      if (!this._reportButton)
>        this._reportButton = document.getElementById("page-report-button");
>  
>      if (!gBrowser.selectedBrowser.blockedPopups ||
> -        gBrowser.selectedBrowser.blockedPopups.count == 0) {
> +        !gBrowser.selectedBrowser.blockedPopups.length) {

the beauty of dynamic languages..
Attachment #8849580 - Flags: review?(felipc) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/41284bd3ce5b
fix hiding popup icon when navigating the browser, r=Felipe
https://hg.mozilla.org/mozilla-central/rev/41284bd3ce5b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment on attachment 8849580 [details]
Bug 1325841 - fix hiding popup icon when navigating the browser,

Approval Request Comment
[Feature/Bug causing the regression]: bug 1273685
[User impact if declined]: popup blocker icon hangs around after you go to a different page
[Is this code covered by automated tests?]: yes; this change adds some more.
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: see comment #0
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: no
[Why is the change risky/not risky?]: I realize this looks big, but it's a 1-line change. We were checking for "foo.count", as it were, and we should have been checking for "foo.length". D'oh. Everything else is just tests (which I also happen to be moving).

Note that the patch will likely need a little help to uplift to beta/aurora  because I butchered browser/base/content/test/general/browser.ini in another bug. Happy to provide branch patches for that, or let relman deal with the (trivial) merge conflicts on that file.
[String changes made/needed]: none
Attachment #8849580 - Flags: approval-mozilla-beta?
Attachment #8849580 - Flags: approval-mozilla-aurora?
Component: XUL Widgets → General
Product: Toolkit → Firefox
Target Milestone: mozilla55 → ---
Target Milestone: --- → Firefox 55
Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(brindusa.tot)
Flags: qe-verify+
Attached video pop-up blocker.mp4 — 
Build ID: 20170326030204
User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0

Verified on Firefox Nightly 55.0a1 on Windows 8.1 x 64, Mac OS X 10.12 and Ubuntu 14.04 x64. While testing the fix, I have observed that the pop-up blocked icon is displayed for a few seconds after the URL has been changed and until the new page has been loaded. If the user clicks on the icon, the pop-up blocker drop-down menu will be displayed. The drop-down menu is displayed even after the webpage has been loaded and the pop-up blocked icon has disappeared. 

Is this expected?

In my opinion if the pop-up blocked icon has disappeared when the webpage URL has been changed, there is not need to show again the icon until the page has finished loading.
Flags: needinfo?(felipc)
Flags: needinfo?(brindusa.tot)
(In reply to Vlad Bacia-Mociran [:VladB] from comment #14)
> Created attachment 8851575 [details]
> pop-up blocker.mp4
> 
> Build ID: 20170326030204
> User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:55.0) Gecko/20100101
> Firefox/55.0
> 
> Verified on Firefox Nightly 55.0a1 on Windows 8.1 x 64, Mac OS X 10.12 and
> Ubuntu 14.04 x64. While testing the fix, I have observed that the pop-up
> blocked icon is displayed for a few seconds after the URL has been changed
> and until the new page has been loaded. If the user clicks on the icon, the
> pop-up blocker drop-down menu will be displayed. The drop-down menu is
> displayed even after the webpage has been loaded and the pop-up blocked icon
> has disappeared. 
> 
> Is this expected?
> 
> In my opinion if the pop-up blocked icon has disappeared when the webpage
> URL has been changed, there is not need to show again the icon until the
> page has finished loading.

Is the behaviour you describe from after this patch different from the behaviour before we fixed bug 1273685? Can you provide more detailed steps to reproduce this problem (ie what new site that loads so slowly are you testing with, etc.)?
Flags: needinfo?(felipc) → needinfo?(vlad.bacia)
Worth taking on ESR52 as well?
status-firefox51: fix-optional → wontfix
status-firefox52: fix-optional → wontfix
status-firefox-esr52: --- → affected
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #15)
> Is the behaviour you describe from after this patch different from the
> behaviour before we fixed bug 1273685? Can you provide more detailed steps
> to reproduce this problem (ie what new site that loads so slowly are you
> testing with, etc.)?

The behaviour that I've described is after the patch was applied. In the video from comment 14, the popup blocker icon is clearly visible for a few seconds (or fractions of a second, depending on the internet connection speed) until youtube is loaded or any webpage. While the page loads, the popup blocker button can be clicked and the dropdown menu is displayed.

I've tested this issue on Nightly v53.0a1, Build ID: 20161201030205 (to see the difference) and in this build the popup blocker icon is still displayed after the page without popups has loaded. Build affected by this bug.

My questions are: 
The popup blocker icon should be displayed during the time in which the new page without popups is loaded? Is this expected? Video is in comment 14.
Flags: needinfo?(vlad.bacia)
(In reply to Vlad Bacia-Mociran [:VladB] from comment #17)
> (In reply to :Gijs from comment #15)
> > Is the behaviour you describe from after this patch different from the
> > behaviour before we fixed bug 1273685? Can you provide more detailed steps
> > to reproduce this problem (ie what new site that loads so slowly are you
> > testing with, etc.)?
> 
> The behaviour that I've described is after the patch was applied. In the
> video from comment 14, the popup blocker icon is clearly visible for a few
> seconds (or fractions of a second, depending on the internet connection
> speed) until youtube is loaded or any webpage. While the page loads, the
> popup blocker button can be clicked and the dropdown menu is displayed.
> 
> I've tested this issue on Nightly v53.0a1, Build ID: 20161201030205 (to see
> the difference) and in this build the popup blocker icon is still displayed
> after the page without popups has loaded. Build affected by this bug.
> 
> My questions are: 
> The popup blocker icon should be displayed during the time in which the new
> page without popups is loaded? Is this expected? Video is in comment 14.

I understand your question, but I don't know what the answer is, which is why I've been asking you another question. I'm trying to understand if the state *after both this bug and bug 1273685* (which is the regressor for this bug) is different from *before both this bug and bug 1273685*. And you've not answered that question, because 20161201030205 is after bug 1273685, not before. Please can you find out if the behaviour you're describing is a regression compared to *before* bug 1273685.

If this happened before bug 1273685, this needs to be a new bug and we should find the regression window appropriately and treat it with the relevant level of importance, but it's unrelated to this bug. This is what I suspect.

If this didn't happen before bug 1273685, this either needs to be a new bug or a follow-up fix in this bug, because there might be something wrong with the patch for this bug.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(vlad.bacia)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #16)
> Worth taking on ESR52 as well?

Only if the patch isn't broken, see comment #15 / comment 18.
(In reply to :Gijs from comment #18)
> Please can you find out if the behaviour you're describing is a
> regression compared to *before* bug 1273685.
> 
> If this happened before bug 1273685, this needs to be a new bug and we
> should find the regression window appropriately and treat it with the
> relevant level of importance, but it's unrelated to this bug. This is what I
> suspect.

I've tested again with Nightly v47.0a1, Build ID: 20160201030241 and the behaviour described by me is the same with the one before bug 1273685. In conclusion, the issue from comment 14 is a "new one" and a new bug should be created. I will log the new bug with the corresponding details.

From my point of view, the patch is ok and the issue is fixed.
status-firefox55: fixed → verified
Flags: needinfo?(vlad.bacia)
Comment on attachment 8849580 [details]
Bug 1325841 - fix hiding popup icon when navigating the browser,

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: noticeable bug in the UI, 1-line fix in the code (with added automated tests)
User impact if declined: annoying icon stays in the URL bar, possibly confusing users
Fix Landed on Version: 55
Risk to taking this patch (and alternatives if risky): low, has automated tests, small patch, verified by QE
String or UUID changes made by this patch: nope.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8849580 - Flags: approval-mozilla-esr52?
(In reply to Vlad Bacia-Mociran [:VladB] from comment #20)
> I've tested again with Nightly v47.0a1, Build ID: 20160201030241 and the
> behaviour described by me is the same with the one before bug 1273685. In
> conclusion, the issue from comment 14 is a "new one" and a new bug should be
> created. I will log the new bug with the corresponding details.
> 
> From my point of view, the patch is ok and the issue is fixed.

Thanks! Please feel free to needinfo me on the new bug you file.
Comment on attachment 8849580 [details]
Bug 1325841 - fix hiding popup icon when navigating the browser,

let's take this in aurora54
Attachment #8849580 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
seems this need a rebasing for aurora
Flags: needinfo?(gijskruitbosch+bugs)
same for beta, needs rebasing
(In reply to Sylvestre Ledru [:sylvestre] from comment #26)
> same for beta, needs rebasing

It doesn't have beta approval, it seems, but I'm 99% sure that the aurora patch should graft cleanly onto beta.
Comment on attachment 8849580 [details]
Bug 1325841 - fix hiding popup icon when navigating the browser,

Let's try this on beta 9. It is late for this though. Is it such a common behavior or situation for users that we'd want it on ESR?
Attachment #8849580 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #28)
> Is it such a common
> behavior or situation for users that we'd want it on ESR?

I don't know. Depends how often people use sites that have blocked popups, and then if they navigate on those sites and then notice the icon remaining. I've barely ever seen it myself, but I'm hardly a typical user. :-)

It doesn't look like we have data on this.

TBH, the actual fix is basically a 1-liner, so from that perspective I'm pretty comfortable taking it in ESR. Happy to put up a patch if you like.
Flags: needinfo?(lhenry)
Vlad, did you end up filing a separate bug for the pre-existing issue you identified?
Flags: needinfo?(vlad.bacia)
Yes(In reply to :Gijs from comment #31)
> Vlad, did you end up filing a separate bug for the pre-existing issue you
> identified?

Yes. Bug 1352966 has been created. Thank you.
Flags: needinfo?(vlad.bacia)
Comment on attachment 8849580 [details]
Bug 1325841 - fix hiding popup icon when navigating the browser,

let's take this in esr52
Flags: needinfo?(lhenry)
Attachment #8849580 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
I have reproduced this issue using Firefox 52.0a2 (20161225004004) on Win 8.1 x64.
I can confirm this issue is fixed, I verified using Firefox 53.0b9, 54.0a2 on Win 8.1 x64, Mac OS X 10.11 and Ubuntu 16.04 x64.
status-firefox53: fixed → verified
status-firefox54: fixed → verified
Flags: qe-verify+
Timea, could you please verify this on 52.1esr as well?
Flags: needinfo?(timea.zsoldos)
I can confirm this issue is fixed, I verified using Firefox 52.1esr on Win 8.1 x64, Mac OS X 10.11 and Ubuntu 14.04 x64.
Status: RESOLVED → VERIFIED
status-firefox-esr52: fixed → verified
Flags: needinfo?(timea.zsoldos)
Depends on: 1370018
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: