86400 - Multiple page info windows can be opened.

Status: RESOLVED FIXED

(SeaMonkey :: Page Info, defect)

Description

[David Carroll]

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.

Comment 1

[doctor__j]

Confirmed on Win32 build 2001061610.

Comment 2

[David Einstein]

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

Comment 3

[John Unruh]

cc'ing people

Comment 4

[Asa Dotzler [:asa]]

->security

Comment 5

[Mitchell Stoltz (not reading bugmail)]

->Crypto

Comment 6

[Bob Lord]

Moving to PSM.

Comment 7

[Daniel Brooks [:db48x]]

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.

Comment 8

[doctor__j]

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.

Comment 9

[Daniel Brooks [:db48x]]

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

Comment 10

[doctor__j]

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.

Comment 11

[Stephane Saux]

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.

Comment 12

[Stephane Saux]

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

Comment 13

[Stephane Saux]

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

Comment 14

[John Unruh]

Mass assigning QA to ckritzer.

Comment 15

[:deanis74]

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

Comment 16

[Michael Entin]

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.

Comment 17

[John Unruh]

Reassigning to Page Info.

Comment 18

[Daniel Brooks [:db48x]]

accepting

Comment 19

[Sam J. Fleet]

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

Comment 20

[Nicolás Lichtmaier]

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...

Comment 21

[:deanis74]

> 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.

Comment 22

[Michael Entin]

>> 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.

Comment 23

[Travis Chase]

Changing milestore to future

Comment 24

[:deanis74]

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

Comment 25

[Philip Chee]

Attachment: Patch v1.0 Port Firefox BrowserPageInfo()

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 26

[[e-mail]]

Comment on attachment 373498 [details] [diff] [review]
Patch v1.0 Port Firefox BrowserPageInfo()

Why not simply compare win.gDocument against doc (or content.document)?

Comment 27

[Philip Chee]

> 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?

Comment 28

[[e-mail]]

(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 29

[[e-mail]]

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 ;-)

Comment 30

[Philip Chee]

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

Comment 31

[Philip Chee]

> 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?

Comment 32

[Philip Chee]

Attachment: Patch v.1.1

> 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.

Comment 33

[[e-mail]]

(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.

Comment 34

[Philip Chee]

>> 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)

Comment 35

[[e-mail]]

(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...

Comment 36

[Philip Chee]

>> 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?

Comment 37

[Philip Chee]

Attachment: Patch v1.2 Use resetPageInfo(args)

> 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 38

[[e-mail]]

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 39

[[e-mail]]

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 40

[[e-mail]]

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.

Comment 41

[[e-mail]]

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.

Comment 42

[Philip Chee]

Attachment: Patch v1.3 better fix.

>>+// 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");|

Comment 43

[[e-mail]]

(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.

Comment 44

[Philip Chee]

Attachment: [for checkin] Patch v1.3a return the window always.

Carrying forward r+/sr+

The new code path didn't return anything. At least one caller expects BrowserPageInfo() to always return a window.

Comment 45

[Philip Chee]

> 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>

Comment 46

[[e-mail]]

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

Comment 47

[Robert Kaiser]

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

Comment 48

[Rob Thijssen [:grenade (EET/UTC+0300)]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=41295eed296a3ef9fc5f59599c4528127656ea5b