Open Bug 779729 Opened 7 years ago Updated 2 years ago

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

Categories

(Firefox :: Session Restore, defect)

defect
Not set

Tracking

()

People

(Reporter: markh, Unassigned)

Details

Attachments

(1 file)

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.)
Summary: popup windows don't have "extrachrome" in chromehidden attribute → restored popup windows don't have "extrachrome" in chromehidden attribute
Blocks: 779054
Attached patch patch v1Splinter Review
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.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #649252 - Flags: review?(gavin.sharp)
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)
(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.
(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?
(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.
(In reply to neil@parkwaycc.co.uk 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"?
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).
This bug is blocking bug 779054 which is needed for Social API.
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.
Attachment #649252 - Flags: review?(gavin.sharp) → review-
We don't need to track this given that bug 779054 found a workaround.
Assignee: ttaubert → nobody
Status: ASSIGNED → NEW
Please: is this still an issue with e.g. 52.x ESR? 

(57 on the horizon.)
You need to log in before you can comment on or make changes to this bug.