From Bugzilla Helper: User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:0.9.1+) Gecko/20010617 BuildID: 2001061720 When I click more than once on the lock icon to open up the page info window, it opens up extra page info windows (one extra per click). Ex. When I double click on the lock, it opens up 2 page info windows. Reproducible: Always Steps to Reproduce: 1.Open any web page in Mozilla. 2.Click on the lock icon two or more times. 3. Actual Results: Two or more page info windows are opened. Expected Results: First click should open page info windows. Subsequent clicks should either (1) Do nothing. (2) Bring window to front if it is behind another window.

Confirmed on Win32 build 2001061610.

Happens on Linux as well. The View->Page Info menu will also open multiple copies, as will control-I

cc'ing people

->security

->Crypto

Moving to PSM.

Is it really a bug though? Suppose I want to compare two seperate pages? The real problem in this case only shows up once bug 52730 is checked in. The changes that bug 52730 will make means that not all the information shown by page info will be generated before the dialog is shown (it would take way too long). Once the window that opened the page info window moves on to another page, the DOM for the old page is gone (naturally), so the old page info window can no longer generate any new info about that old page. It used to crash outright, but I fixed that. I think that it's really just a minor usability thing, and that allowing the user to have more than one page info window open at a time is worth it. On the other hand, if it does need to be fixed, it should be fixed at the same time as bug 76250, because both of them require that we change every place that opens page info.

Well, I thought there are two different situations in which you will / will not want to keep this multi-page-info feature: 1) If you're clicking the Page Info icon twice on the SAME page, you will (pretty sure) get the SAME info displayed on two Page Info windows. I don't need this. No, thanks. 2) If you're clicking the Page Info icon on each of the two or more DIFFERENT pages, you'll get two or more different page info displayed. And that's very useful as mentioned by Daniel. Yes, please keep this feature.

Ok, so basically you just want the lock icon to trap a double click and just pretend it was a single click?

I'm sorry to have mislead you by my previous comment. "Clicking on Page Inco icon twice" != "Double clicking the icon" "Clicking on Page Inco icon twice" == "Clicking the icon once, wait a sec, and then click again" Current behaviour is that: BOTH Double-clicking AND clicking+wait+clicking on the page info icon will give you two identical page info windows. Therefore, trapping double-clicking will only solve half of the problem. The page info window has to remember its "parent window" from which it's spawned and not to spawn from THAT parent window again when user clicks the page info icon. I hope I have cleared up my ideas.

p3 Agree with the design proposed, which I rephrase. A page info window should remember which page generated it. Requesting the page info (in all ways this can be done) should only create a new one if it doesn't exist. Otherwise it should raise the existing window.

Moving all P3 and P4 bugs targetted to 2.1 to future.

removing nsenterprise keyword from PSM bugs with target milestone of future.

Mass assigning QA to ckritzer.

*** Bug 97101 has been marked as a duplicate of this bug. ***

This bug has nothing to do with PSM, or at least it is not specific to PSM. Security page is just one way to expose it. Another way is right click the page, select View Page Info menu - the same dialog appears. Now repeat it - another instance of this dialog appears. Now click security icon - third instance of this dialog appears (the only difference is the active page). I expected to have only one instance of dialog (OK, the last click should switch active page). This is a generic issue, and should be solved for all different ways to invoke this dialog, for example by keeping a global map from URL moniker to dialog instance.

Reassigning to Page Info.

accepting

Confirmed this bug on build ID: 2002053012 (Moz 1.0 Original). Single click or Double click, either on of them opened the duplicated "Page Info" text box whether the click is quick or 5 seconds apart. I agree with Additional Comment #8. This should be the way to go. Will have to take a look at bug #76250. FletchSOD

Not so easy... What if I open page info, edit something in the server, reload, and open another page info to compare? Well... page info could remember the DOM (adding a hidden attribute to the page DOM?), but then what happens if the page has scripts which change the page? (e.g.: its images). If a page info window is going to be reused, it must also listen for mutation events and update itself... just some random thoughts...

> What if I open page info, edit something in the server, reload, and open another > page info to compare? Then you're one of about .01% of users that will do this, and you need to find another way.

>> What if I open page info, edit something in the server, reload, and open another >> page info to compare? >> > Then you're one of about .01% of users that will do this, and you need to find > another way. E.g.: page info could provide Copy button that would copy page information to clipboard or Save As button that would save information as text or XML file. Now you can do anything with your page info: send it as mail, open two documents and compare them manually, compare files using your favorite diff utility, etc. Much better and more generic than hacks like remembering DOM, etc. But even this functionality is useful to only 0.1% users, and multiple page info dialogs should be fixed even if time does not permit providing this functionality.

Changing milestore to future

Re: Comment 22... I think that's best implemented as an extension.

This is a port of the Firefox BrowserPageInfo() but without the stray \t and the unnecessary convolutions. From Comment 11: > A page info window should remember which page generated it. Requesting the page > info (in all ways this can be done) should only create a new one if it doesn't > exist. Otherwise it should raise the existing window.

Comment on attachment 373498 [details] [diff] [review] Patch v1.0 Port Firefox BrowserPageInfo() Why not simply compare win.gDocument against doc (or content.document)?

> Why not simply compare win.gDocument against doc (or content.document)? (Hmm. I'm just following the logic in the Firefox Bug 217611.) If two different documents have the same .location do we want to consider them the same for the purposes of BrowserPageInfo() even if they are opened in different windows. Or put it more simply, how does == or === work on document objects?

(In reply to comment #27) > If two different documents have the same .location do we want to consider them > the same for the purposes of BrowserPageInfo() even if they are opened in > different windows. Or put it more simply, how does == or === work on document > objects? If you reload a window then I believe you get a new document object, and if the page is dynamically generated then it could be different to the previous one. By contrast if you use document.open to replace the document it has the same location, yet it is typically a completely different document.

Comment on attachment 373498 [details] [diff] [review] Patch v1.0 Port Firefox BrowserPageInfo() Oh, and of course clicking the security icon should switch to the security tab in an existing page info window. As an optional bonus, make pressing ^I twice only open Page Info once ;-)

> As an optional bonus, make pressing ^I twice only open Page Info once ;-) Doesn't my current patch do that?

> If you reload a window then I believe you get a new document object, and if the > page is dynamically generated then it could be different to the previous one. > By contrast if you use document.open to replace the document it has the same > location, yet it is typically a completely different document. In that case I'll stick to comparing .location By the way do I have to cast document.location.toString() first or will javascript magically do this for me?

> Oh, and of course clicking the security icon should switch to the security tab > in an existing page info window. Sigh. Fixed. I was planning to do this later, after I ported The reload pageinfo functionality from the Firefox pageinfo implementation. As it is this fix is a kludge. > As an optional bonus, make pressing ^I twice only open Page Info once ;-) I may be able to leverage toOpenWindowByType() after a bit of enhancement to that function but I'll do that in another bug once I get the ability to reload pageinfo in order to ensure that the security information is up to date. >> If you reload a window then I believe you get a new document object, and if the >> page is dynamically generated then it could be different to the previous one. >> By contrast if you use document.open to replace the document it has the same >> location, yet it is typically a completely different document. > In that case I'll stick to comparing .location K.I.S.S. and all that. The end user probably doesn't care about the subtleties of changing documents. If they see the same URL in the location bar they wouldn't expect another pageinfo window to open.

(In reply to comment #31) > By the way do I have to cast document.location.toString() first or will > javascript magically do this for me? You mean .spec, and yes, it will, but once each time through the loop... (In reply to comment #32) > > Oh, and of course clicking the security icon should switch to the security tab > > in an existing page info window. > Sigh. Fixed. I was planning to do this later, after I ported The reload > pageinfo functionality from the Firefox pageinfo implementation. As it is this > fix is a kludge. So what does this patch do that "reload pageinfo" wouldn't? > K.I.S.S. and all that. The end user probably doesn't care about the subtleties > of changing documents. If they see the same URL in the location bar they > wouldn't expect another pageinfo window to open. Sounds like we really need this "reload pageinfo" feature instead.

>> Sigh. Fixed. I was planning to do this later, after I ported The reload >> pageinfo functionality from the Firefox pageinfo implementation. As it is this >> fix is a kludge. > So what does this patch do that "reload pageinfo" wouldn't? Well with this kludge, BrowserPageInfo() needs knowledge of the internals of pageInfo and if that changes then BrowserPageInfo() needs to be changed as well. With a "reloadIt() API we can hide the implementation details from BrowserPageInfo(). > Sounds like we really need this "reload pageinfo" feature instead. Especially if you open pageinfo from the security thingy in the status bar you'll need to show up to date data. Marking this as dependent on Bug 444917 (It should be possible for an extension to reload Page Info on a different document)

(In reply to comment #34) > With a "reloadIt() API we can hide the implementation details from BrowserPageInfo(). Attachment 272703 [details] [diff] doesn't seem to provide such an API...

>> With a "reloadIt() API we can hide the implementation details from BrowserPageInfo(). > Attachment 272703 [details] [diff] doesn't seem to provide such an API... Who said that we can't make improvements while porting stuff from Firefox?

> Sounds like we really need this "reload pageinfo" feature instead. Now depends on |resetPageInfo(args)| from attachment 380862 [details] [diff] [review] in Bug 444917. Apply that patch first before testing this one. Clicking on the security icon a second time will reload the current document into the pageInfo window if the content.location is the same. If not a new pageInfo window is opened. In either case the pageInfo window is brought into focus if it isn't already. > As an optional bonus, make pressing ^I twice only open Page Info once ;-) This should be fixed. > Why not simply compare win.gDocument against doc (or content.document)? The end user probably doesn't care about the subtleties of changing documents. If they see the same URL in the location bar they wouldn't expect another pageinfo window to open.

Comment on attachment 381697 [details] [diff] [review] Patch v1.2 Use resetPageInfo(args) [Noted that this is not intended to be a perfect fix for the ^I ^I issue] >+// doc - document to use for source, or null for this window's document Nit: s/this window/current tab/ >+// initialTab - name of the initial tab to display, or null for the first tab Nit: s/name/id/

Comment on attachment 381697 [details] [diff] [review] Patch v1.2 Use resetPageInfo(args) >+ win.resetPageInfo(args); I just noticed that this needs a real doc. So you have to move/copy the doc calculation. (In reply to comment #33) > (In reply to comment #31) > > By the way do I have to cast document.location.toString() first or will > > javascript magically do this for me? > You mean .spec, and yes, it will, but once each time through the loop... ...so an extra relatedUrl = relatedUrl.spec; wouldn't hurt. Although given that you need doc to exist above, that would simplify the relatedUrl code and let you add the .spec directly.

Comment on attachment 381697 [details] [diff] [review] Patch v1.2 Use resetPageInfo(args) So, the way I broke Page Info was a) open page info b) reload page c) "open" page info again... probably related to the doc issue above.

Oh, and reopening page info doesn't reset the tab to General, should it? If you have Media/Forms/Links open you also lose the selection, which seems odd.

>>+// doc - document to use for source, or null for this window's document > Nit: s/this window/current tab/ Fixed. >>+// initialTab - name of the initial tab to display, or null for the first tab > Nit: s/name/id/ Fixed. >>+ win.resetPageInfo(args); > I just noticed that this needs a real doc. So you have to move/copy the doc > calculation. Fixed. >> You mean .spec, and yes, it will, but once each time through the loop... > ...so an extra relatedUrl = relatedUrl.spec; wouldn't hurt. Although given that > you need doc to exist above, that would simplify the relatedUrl code and let > you add the .spec directly. The location object doesn't have a .spec property. It does have a .href property but MDC says to use location.toString(). Modulo this, fixed. > So, the way I broke Page Info was a) open page info b) reload page c) "open" > page info again... probably related to the doc issue above. WFM now. > Oh, and reopening page info doesn't reset the tab to General, should it? If you In the other patch I have: +function resetPageInfo(args) [...] + if (args && args.initialTab) + showTab(args.initialTab); I think that it makes sense that if the user already has a pageinfo window open and focused on a particular tab, there is an expectation that it would remain on that tab (unless the caller specifies an explicit initialTab as the Security icon does). > have Media/Forms/Links open you also lose the selection, which seems odd. I too noticed this. But the same thing happens without these two patches applied and you do |BrowserPageInfo(null, "linksTab");|

(In reply to comment #42) > The location object doesn't have a .spec property. It does have a .href > property but MDC says to use location.toString(). Modulo this, fixed. Sorry, I meant .href but I didn't realise MDC prefers toString(). > I too noticed this. But the same thing happens without these two patches > applied and you do |BrowserPageInfo(null, "linksTab");| Must be a page info bug then.

Carrying forward r+/sr+ The new code path didn't return anything. At least one caller expects BrowserPageInfo() to always return a window.

> Created an attachment (id=381989) > [for checkin] Patch v1.3a return the window always. Interdiff: <https://bugzilla.mozilla.org/attachment.cgi?oldid=381784&action=interdiff&newid=381989&headers=0>

Not only that, it should have been a strict JS warning. Maybe I forgot to look.

Pushed as http://hg.mozilla.org/comm-central/rev/fa8bc9e192a8 - leaving to Philip to mark FIXED if everything needed has been done here.