Closed
Bug 402348
Opened 17 years ago
Closed 17 years ago
popup statusbar notification switches to incorrect tab when tab closed
Categories
(Firefox :: Tabbed Browser, defect, P3)
Firefox
Tabbed Browser
Tracking
()
VERIFIED
FIXED
Firefox 3 beta3
People
(Reporter: wgianopoulos, Assigned: wgianopoulos)
References
Details
(Keywords: regression)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
If you have 2 tabs open and tab 2 contains a site with a blocked pop-up, closing tab 2 results in the blocked pop-up status bar indicator being present on tab 1. This indicator persists through visits to other sites not containing any pop-ups.
Steps to reproduce:
1. With 2 browser tabs open.
2. load http://www.mozilla.org/ in tab 1.
3. load http://www.pgatour.com/ in Tab 2.
4. Verify that the blocked pop-up statusbar icon is present in Tab 2.
5. Verify that the blocked pop-up statusbar icon is not present in Tab 1.
6. Close Tab 2.
7. Note that the blocked pop-up statusbar icon is now present in Tab 1.
There are really 2 issues here.
1. Why is the blocked pop-up statusbar icon not removed when the tab is closed.
2. Why is the blocked pop-up statusbar icon ever displayed. Isn't it now redundant?
Comment 1•17 years ago
|
||
It is a recent regression: http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=2007-10-19+16%3A00&maxdate=2007-10-19+22%3A00
I think it could be Bug 398135 (by the lack of other possibilities).
Do you think it should not display the info icon in the statusbar? I think it is useful information if you have switched off the moving notification bar. And in most cases there is plenty of room there.
Assignee | ||
Comment 2•17 years ago
|
||
I was suggesting removing it because it is redundant if you use the notification bar. I kind of figured people who cared would have the notification bar enabled. But I suppose there are those who just hate the notification bar, yet whould still like to have a blocked popup notification.
Comment 3•17 years ago
|
||
bz, any hints for what to look for that we're doing wrong, if it's Fx's fault and not the fault of bug 398135 that it no longer works?
Flags: blocking-firefox3?
Comment 4•17 years ago
|
||
Hmm. So what bug 398135 did is that removing a node from the DOM now fully tears down its XBL binding (which didn't use to happen before).
So that's the first thing to look at: are any relevant XBL-bound nodes being removed from the DOM? If so, what happens then?
It could well be a core XBL problem, but in that case I'd still love some guidance as to where the relevant UI code lives.
Assignee | ||
Comment 5•17 years ago
|
||
What appears to be broken, is the optimization here:
http://lxr.mozilla.org/seamonkey/source/browser/base/content/tabbrowser.xml#711
that tries to avoid calling updatePageReport if the blocked pop-up status is the same on the old and new active page.
It appears that this.mCurrentBrowser.pageReport is no longer valid at this point and is now always false.
Assignee | ||
Comment 6•17 years ago
|
||
(In reply to comment #5)
> What appears to be broken, is the optimization here:
>
> http://lxr.mozilla.org/seamonkey/source/browser/base/content/tabbrowser.xml#711
>
> that tries to avoid calling updatePageReport if the blocked pop-up status is
> the same on the old and new active page.
>
> It appears that this.mCurrentBrowser.pageReport is no longer valid at this
> point and is now always false.
>
Sorry, always false in the case where you case the active tab, is what I meant to say.
Comment 7•17 years ago
|
||
Ah, indeed. <method name="removeTab"> removes the tab node from the DOM (which tears down all the XBL bindings), and only then calls |this.updateCurrentBrowser()|. That means that in updateCurrentBrowser the binding on this.mCurrentBrowser is gone, and this.mCurrentBrowser.pageReport is is undefined.
Do we feel that tearing down the binding like this is too incompatible a change? If not, we probably want to change tabbrowser to deal (e.g. null out this.mCurrentBrowser in removeTab, and skip the optimization if !this.mCurrentBrowser.
Comment 8•17 years ago
|
||
Or remove the node from the DOM after updating the current tab.
Comment 9•17 years ago
|
||
Oddly enough dbaron did this for suite tabbrowser in bug 131456...
Assignee | ||
Comment 10•17 years ago
|
||
I went for the simpler approach. I thought at this stage of the game for 1.9 a simple patch was preferred over moving a lot of code around.
Assignee: nobody → wgianopoulos
Status: NEW → ASSIGNED
Attachment #288086 -
Flags: superreview?
Attachment #288086 -
Flags: review?
Assignee | ||
Updated•17 years ago
|
Attachment #288086 -
Flags: superreview?(mconnor)
Attachment #288086 -
Flags: superreview?
Attachment #288086 -
Flags: review?(gavin.sharp)
Attachment #288086 -
Flags: review?
Assignee | ||
Updated•17 years ago
|
Attachment #288086 -
Attachment is obsolete: true
Attachment #288086 -
Flags: superreview?(mconnor)
Attachment #288086 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 11•17 years ago
|
||
Guess I should have looked into comment #9 more closely.
Attachment #288091 -
Flags: superreview?
Attachment #288091 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•17 years ago
|
Attachment #288091 -
Flags: superreview? → superreview?(mconnor)
Assignee | ||
Comment 12•17 years ago
|
||
(In reply to comment #9)
> Oddly enough dbaron did this for suite tabbrowser in bug 131456...
>
So this will fix a leak as well? Great!
Comment 13•17 years ago
|
||
Comment on attachment 288091 [details] [diff] [review]
Better patch
SF isn't needed for /browser
Attachment #288091 -
Flags: superreview?(mconnor)
Assignee | ||
Updated•17 years ago
|
Attachment #288091 -
Flags: superreview?(mconnor)
Updated•17 years ago
|
Attachment #288091 -
Flags: superreview?(mconnor)
Assignee | ||
Comment 14•17 years ago
|
||
Comment on attachment 288091 [details] [diff] [review]
Better patch
Sorry. Didn't notice you removed that. Sometimes I have trouble getting bugzilla to fill in review requests.
Updated•17 years ago
|
Flags: blocking-firefox3? → blocking-firefox3+
Updated•17 years ago
|
Priority: -- → P3
Target Milestone: --- → Firefox 3 M11
Assignee | ||
Comment 17•17 years ago
|
||
(In reply to comment #8)
> Or remove the node from the DOM after updating the current tab.
>
In looking at this more closely, I think that even if the moving the code approach is taken, I would still like this patch.
Nulling out this.mCurrentBrowser in removeTab is still probably a good idea, and I have no idea exactly how the current code at
http://lxr.mozilla.org/seamonkey/source/browser/base/content/tabbrowser.xml#711
works the first time through.
Comment 18•17 years ago
|
||
Comment on attachment 288091 [details] [diff] [review]
Better patch
Not sure why we explicitly call updateCurrentBrowser in the removeTab case, since the onselect handler already takes care of that for us, but that's something for another bug I guess. Sorry for the delay here.
Attachment #288091 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Updated•17 years ago
|
Keywords: checkin-needed
Comment 19•17 years ago
|
||
Checking in browser/base/content/tabbrowser.xml;
/cvsroot/mozilla/browser/base/content/tabbrowser.xml,v <-- tabbrowser.xml
new revision: 1.255; previous revision: 1.254
done
Comment 20•17 years ago
|
||
Verified.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008021704 Minefield/3.0b4pre
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3) Gecko/2008020511 Firefox/3.0b3 ID:2008020511
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•