Closed
Bug 1325841
Opened 8 years ago
Closed 8 years ago
Popup blocked icon in url bar does not disappear even if the site is moved
Categories
(Firefox :: General, defect)
Tracking
()
People
(Reporter: euthanasia_waltz, Assigned: Gijs)
References
Details
(Keywords: regression)
Attachments
(2 files)
(deleted),
text/x-review-board-request
|
Felipe
:
review+
jcristau
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
jcristau
:
approval-mozilla-esr52+
|
Details |
(deleted),
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
Updated•8 years ago
|
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.
Neil, can you help find someone to fix this regression? It sounds fairly annoying. Thanks.
Flags: needinfo?(enndeakin)
Assignee | ||
Comment 5•8 years ago
|
||
I'll see if I can find time tomorrow to have a look.
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(felipc)
Comment hidden (mozreview-request) |
Comment 7•8 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 10•8 years ago
|
||
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/41284bd3ce5b
fix hiding popup icon when navigating the browser, r=Felipe
Comment 11•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee | ||
Comment 12•8 years ago
|
||
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?
Updated•8 years ago
|
Component: XUL Widgets → General
Product: Toolkit → Firefox
Target Milestone: mozilla55 → ---
Updated•8 years ago
|
Target Milestone: --- → Firefox 55
Updated•8 years ago
|
status-firefox54:
--- → affected
Comment 13•8 years ago
|
||
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)
Updated•8 years ago
|
Flags: qe-verify+
Comment 14•8 years ago
|
||
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)
Updated•8 years ago
|
Flags: needinfo?(brindusa.tot)
Assignee | ||
Comment 15•8 years ago
|
||
(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)
Comment 16•8 years ago
|
||
Worth taking on ESR52 as well?
status-firefox-esr52:
--- → affected
Flags: needinfo?(gijskruitbosch+bugs)
Comment 17•8 years ago
|
||
(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)
Assignee | ||
Comment 18•8 years ago
|
||
(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)
Assignee | ||
Comment 19•8 years ago
|
||
(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.
Comment 20•8 years ago
|
||
(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.
Flags: needinfo?(vlad.bacia)
Assignee | ||
Comment 21•8 years ago
|
||
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?
Assignee | ||
Comment 22•8 years ago
|
||
(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 23•8 years ago
|
||
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+
Assignee | ||
Comment 25•8 years ago
|
||
Flags: needinfo?(gijskruitbosch+bugs)
Comment 26•8 years ago
|
||
same for beta, needs rebasing
Assignee | ||
Comment 27•8 years ago
|
||
(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+
Comment 29•8 years ago
|
||
bugherder uplift |
Flags: in-testsuite+
Assignee | ||
Comment 30•8 years ago
|
||
(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)
Assignee | ||
Comment 31•8 years ago
|
||
Vlad, did you end up filing a separate bug for the pre-existing issue you identified?
Flags: needinfo?(vlad.bacia)
Comment 32•8 years ago
|
||
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 33•8 years ago
|
||
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+
Comment 34•8 years ago
|
||
bugherder uplift |
Comment 35•8 years ago
|
||
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.
Comment 36•8 years ago
|
||
Timea, could you please verify this on 52.1esr as well?
Flags: needinfo?(timea.zsoldos)
Comment 37•8 years ago
|
||
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
Updated•8 years ago
|
Flags: needinfo?(timea.zsoldos)
You need to log in
before you can comment on or make changes to this bug.
Description
•