454047 - Pop-up locationbar is editable after session restore

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Jeroen]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080906062345 Minefield/3.1b1pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080906062345 Minefield/3.1b1pre

After session restore, the locationbar of a pop-up window is editable. 

Reproducible: Always

Steps to Reproduce:
1. Enable session restore (if not enabled)
2. Open a pop-up window
3. Close Firefox
4. Restart Firefox
5. See that the locationbar is editable, while it probably shouldn't
Actual Results:  
Locationbar is editable. 

Expected Results:  
Locationbar is not editable.

Comment 1

[Dão Gottwald [:dao]]

This should be handled in BrowserStartup:

799   if (gURLBar && document.documentElement.getAttribute("chromehidden").indexOf("toolbar") != -1) {
800     gURLBar.setAttribute("readonly", "true");
801     gURLBar.setAttribute("enablehistory", "false");
802   }

Not sure why that doesn't work after session restore.

Comment 2

[Simon Bünzli]

(In reply to comment #1)
> Not sure why that doesn't work after session restore.

Because our code runs after BrowserStartup and the state of the address bar isn't dynamically updated when modifying the chromehidden attribute (through setting window.toolbar.visible).

Comment 3

[(no longer active)]

This will be fixed by my patch in bug 495495.

Comment 4

[(no longer active)]

Bug 495495 landed, which means this should be fixed now.  CCing Marcia and Henrik for verifying this...

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Patch on bug 495495 has been backed out. Reopening.

Comment 6

[(no longer active)]

Bug 495495 has landed, so this should be fixed.

Comment 7

[u279076]

Attachment: Minimized testcase

Comment 8

[u279076]

Added test to Litmus: https://litmus.mozilla.org/show_test.cgi?id=7887
in-litmus+

Comment 9

[(no longer active)]

(In reply to comment #8)
> Added test to Litmus: https://litmus.mozilla.org/show_test.cgi?id=7887
> in-litmus+

Is the Litmus test really correct?  It implies that two popup windows should be opened, while I don't see how it can be, because clicking on the link in the test case only opens one popup window.

Comment 10

[(no longer active)]

Bug 495495 landed on 1.9.1.2 as well.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #9)
> Is the Litmus test really correct?  It implies that two popup windows should be
> opened, while I don't see how it can be, because clicking on the link in the
> test case only opens one popup window.

Right. It should be updated. Only one pop-up should be visible. Anthony, it would be also nice when we would add those test pages to our litmus testcase repository instead on Bugzilla.

Comment 12

[u279076]

(In reply to comment #11)
> (In reply to comment #9)
> > Is the Litmus test really correct?  It implies that two popup windows should be
> > opened, while I don't see how it can be, because clicking on the link in the
> > test case only opens one popup window.
> 
> Right. It should be updated. Only one pop-up should be visible. 
The testcase is correct.  Session restores the original pop-up which causes the script to run once more creating the second pop-up.  I actually test my test cases before I post them.  This is the behaviour in Firefox.  Perhaps you should try the test case first.

>
> Anthony, it would be also nice when we would add those test pages to our 
> litmus testcase repository instead on Bugzilla.

I think it needs to live in both places.  AFAIK, I don't have access to commit to Litmus storage.  Feel free.

Comment 13

[u279076]

Henrik, in-litmus+.  There is a testcase in litmus.  Whether you think it is broken or not does not change in-litmus+.

Comment 14

[(no longer active)]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #9)
> > > Is the Litmus test really correct?  It implies that two popup windows should be
> > > opened, while I don't see how it can be, because clicking on the link in the
> > > test case only opens one popup window.
> > 
> > Right. It should be updated. Only one pop-up should be visible. 
> The testcase is correct.  Session restores the original pop-up which causes the
> script to run once more creating the second pop-up.  I actually test my test
> cases before I post them.  This is the behaviour in Firefox.  Perhaps you
> should try the test case first.

I did test it before commenting.  For me, the second popup which should come from the restoring of the javascript: URL is blocked by the popup blocker, and only one popup gets restored.  If I disable the popup blocker, then two popup windows are opened.

Comment 15

[u279076]

The fact that one window get's blocked still does not discount the fact that there are two popup windows when restoring.  Naturally, I assumed since you want to test the UI of a popup window that you would run this with popup blocker disabled.  Otherwise, the test is useless.  I suppose I should not have made that assumption.  I'll add a step to the test.

Comment 16

[(no longer active)]

(In reply to comment #15)
> The fact that one window get's blocked still does not discount the fact that
> there are two popup windows when restoring.  Naturally, I assumed since you
> want to test the UI of a popup window that you would run this with popup
> blocker disabled.  Otherwise, the test is useless.  I suppose I should not have
> made that assumption.  I'll add a step to the test.

That assumption is invalid.  The popup blocker does not block popups initiated by user clicks, so even when popup blocking is enabled, clicking on the test link correctly shows a popup window.  I don't think users need to disable the popup blocker for this test.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #12)
> I think it needs to live in both places.  AFAIK, I don't have access to commit
> to Litmus storage.  Feel free.

Pushed as http://hg.mozilla.org/webtools/litmus/rev/918a1613275c. In the future just send me the testcase and the # and I can upload it immediately. Can you please update your test tomorrow?

(In reply to comment #13)
> Henrik, in-litmus+.  There is a testcase in litmus.  Whether you think it is
> broken or not does not change in-litmus+.

We used that flag in the past (during Firefox 3.5 development) to indicate that a new Litmus test needs to be updated to show the correct steps.

Comment 18

[u279076]

> Pushed as http://hg.mozilla.org/webtools/litmus/rev/918a1613275c. In the future
> just send me the testcase and the # and I can upload it immediately. Can you
> please update your test tomorrow?
> 
I'll update it now...thanks.

Comment 19

[u279076]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2

Verified fixed on all above builds...