779729 - restored popup windows don't have "extrachrome" in chromehidden attribute

Status: NEW

(Firefox :: Session Restore, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

A popup window will usually have an attribute chromehidden="menubar toolbar directories extrachrome ".  When such windows are restored via session restore, the "extrachrome" part of the value is missing.  This means, for example, that pressing Ctrl+H on the restored popup will open the sidebar and show history, whereas it is ignored when the window is first opened (the sidebar has CSS which keeps the sidebar hidden when extrachrome is in that attribute.)

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Since we incorporated the former sessionrestore add-on in bug 328159, session store always opened windows with the feature list "chrome,dialog=no,all". Now, if I remove "all" the attached test passes (and all other SS tests as well).

I couldn't really wrap my head around how these window features work and why 'extrachrome' behaves differently than 'location' or 'menubar' that should be included in 'all' as well. Maybe Gavin knows?

Alternatively, we could only specify "macsuppressanimation" as the only feature. Works as well but I'm not sure which implications this has.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 649252 [details] [diff] [review]
patch v1

As a complete nit, I wonder if the attachment should be named after *this* bug rather than 779054?  Given the test is just checking the "extrachrome" value and this bug is specifically about that attribute, it seems to make sense.  Bug 779054 should have a test which specifically checks the sidebar in a popup (but that test would probably not explicitly check for "extrachrome", but instead just check the actual sidebar)

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Mark Hammond (:markh) from comment #2)
> As a complete nit, I wonder if the attachment should be named after *this*
> bug rather than 779054?  Given the test is just checking the "extrachrome"
> value and this bug is specifically about that attribute, it seems to make
> sense.  Bug 779054 should have a test which specifically checks the sidebar
> in a popup (but that test would probably not explicitly check for
> "extrachrome", but instead just check the actual sidebar)

Oops, yeah. I started working on that other bug and then discovered this specific session store bug. I'll rename that with the next patch or before checking in if the patch is good.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

(In reply to Tim Taubert [:ttaubert] (on vacation, back Sep 5th) from comment #1)
> I couldn't really wrap my head around how these window features work and why
> 'extrachrome' behaves differently than 'location' or 'menubar' that should
> be included in 'all' as well. Maybe Gavin knows?

I don't! Maybe Neil can shed some light?

Comment 5

[[e-mail]]

(In reply to Gavin Sharp from comment #4)
> (In reply to Tim Taubert from comment #1)
> > I couldn't really wrap my head around how these window features work and why
> > 'extrachrome' behaves differently than 'location' or 'menubar' that should
> > be included in 'all' as well. Maybe Gavin knows?
> 
> I don't! Maybe Neil can shed some light?

Well it's quite easy, extrachrome is the only one that doesn't have an associated property. The only supported way I know of to toggle extrachrome is to access the nsIXULWindow chrome flags.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

(In reply to [e-mail] from comment #5)
> Well it's quite easy, extrachrome is the only one that doesn't have an
> associated property. The only supported way I know of to toggle extrachrome
> is to access the nsIXULWindow chrome flags.

So why does it get restored properly when you remove the "all"?

Comment 7

[[e-mail]]

By code inspection, I don't think it does, and I would expect it to hide the extrachrome on all windows after the first (which is always a normal window).

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

This bug is blocking bug 779054 which is needed for Social API.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 649252 [details] [diff] [review]
patch v1

I looked into this more, and I don't think this is what we want. The underlying issue here is that the code in _openWindowWithState and the code path that is followed for opening popup windows is very different, and result in different sets of chrome flags for the resulting window.

One thing I experimented with that led to slightly better behavior was getting rid of the "argString" from _openWindowWithState, and just passing null instead. This changes the value of the "dialog" parameter sent to OpenWindowInternal in nsWindowWatcher::OpenWindow(), which results in chrome flags that more closely match those obtained by normal web content window.open calls (even though they're still not quite the same). I think it also fixed the test in this patch.

I also noticed an issue with the test: rather than using findPopupWindow(), it should use getChromeWindow(win) (from e.g. http://hg.mozilla.org/mozilla-central/annotate/c24a0fd08031/mobile/android/components/LoginManagerPrompter.js#l385), which is more reliable.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

We don't need to track this given that bug 779054 found a workaround.

Comment 11

[Graham Perrin]

Please: is this still an issue with e.g. 52.x ESR? 

(57 on the horizon.)