Closed
Bug 1453788
Opened 7 years ago
Closed 7 years ago
Remember window size and location for chrome HTML windows
Categories
(Firefox :: General, enhancement)
Firefox
General
Tracking
()
VERIFIED
FIXED
Firefox 62
People
(Reporter: bgrins, Assigned: bdahl)
References
Details
Attachments
(1 file)
Right now we track this data for XUL windows with `nsXULWindow::SavePersistentAttributes` and `XULDocument::Persist` which creates a xulstore.json file with data like:
{
"chrome://browser/content/browser.xul": {
"main-window": {
"screenX": "368",
"screenY": "88",
"width": "912",
"height": "523",
"sizemode": "normal"
}
}
}
This means if you close and reopen a XUL window it'll restore to the correct position. The same feature should work with chrome HTML windows.
I don't think we need to support the full [persist] behavior, but could instead assume that top-level chrome windows should remember all of their size and location data. Maybe we we will want to provide the window a way to opt in or out though.
|
Reporter | |
Comment 1•7 years ago
|
||
There's also an issue where the intrinsic size doesn't get set properly when opening an HTML document like:
Services.ww.openWindow(null, "chrome://devtools/content/webconsole/webconsole.html", "_blank", "chrome,titlebar,toolbar,centerscreen,resizable,dialog=no", null);
But it does when opening a XUL document:
Services.ww.openWindow(null, "chrome://devtools/content/webconsole/browserconsole.xul", "_blank", "chrome,titlebar,toolbar,centerscreen,resizable,dialog=no", null);
So you don't see the window open unless if you do something like this in the load event:
```
document.body.style.width = "400px";
document.body.style.height = "300px";
```
Or open a URL like:
Services.ww.openWindow(null, "data:text/html,<body style='width:300px; height: 400px;'></body>", "_blank", "chrome,titlebar,toolbar,centerscreen,resizable,dialog=no", null);
Oddly, opening a URL that sets the width/height on the html tag ends up opening a window that the correct height but very narrow:
Services.ww.openWindow(null, "data:text/html,<html style='width:300px; height: 400px;'></html>", "_blank", "chrome,titlebar,toolbar,centerscreen,resizable,dialog=no", null);
|
|
Reporter | |
Comment 2•7 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #1)
> There's also an issue where the intrinsic size doesn't get set properly when
> opening an HTML document like:
>
> Services.ww.openWindow(null,
> "chrome://devtools/content/webconsole/webconsole.html", "_blank",
> "chrome,titlebar,toolbar,centerscreen,resizable,dialog=no", null);
>
> But it does when opening a XUL document:
>
> Services.ww.openWindow(null,
> "chrome://devtools/content/webconsole/browserconsole.xul", "_blank",
> "chrome,titlebar,toolbar,centerscreen,resizable,dialog=no", null);
Which, I guess does make sense since browserconsole.xul has a default width/height set on the <window> at https://searchfox.org/mozilla-central/rev/a30a60eadcff99d2a043241955d215dbeccad876/devtools/client/webconsole/browserconsole.xul#12. But that value gets overridden if you resize the window and then close and reopen it.
I'm not sure how we should represent that in HTML. For example: should we use style.width/style.height on the body and then later remove them after we restore size at startup? Or maybe introduce [width]/[height] attributes on the <html> tag that are only used for this?
|
||
Comment 3•7 years ago
|
||
> Oddly, opening a URL that sets the width/height on the html tag ends up opening a window that the correct
> height but very narrow:
Worth checking what sizeToContent and friends end up doing in that case....
|
|
Reporter | |
Comment 4•7 years ago
|
||
Brendan and I discussed in a bit more detail and were thinking that a <meta> tag would be a reasonable place to store default width/height/screenX/screenY in chrome HTML docs.
Possibly also would want some way to opt in/out of size tracking - similar to how you can do so with [persist] although likely a single choice of "remember size/position/sizemode" or "don't remember any of them" instead of using the whole list like `persist="screenX screenY width height sizemode"` on every window. Unless if there's some reason why a window would want to remember width/height but not screenX/screenY (we couldn't find any in-tree from a quick search).
|
|
Reporter | |
Comment 5•7 years ago
|
||
As per https://bugzilla.mozilla.org/show_bug.cgi?id=1454445#c11, we can get default sizing / positioning to work already by putting the attributes on the <html> node. For example:
Services.ww.openWindow(null, "data:text/html,<html width='100' height='200' screenX='300' screenY='300'><body></body></html>", "_blank", "chrome", null);
We'll still need to figure out how to persist after repositioning/resizing and check to see if sizemode works.
|
||
Updated•7 years ago
|
Component: DOM → General
Product: Core → Firefox
|
Assignee | |
Updated•7 years ago
|
Assignee: nobody → bdahl
| Comment hidden (mozreview-request) |
|
|
||
Comment 7•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8972351 [details]
Bug 1453788 - Allow top level HTML windows to have persistent window state.
https://reviewboard.mozilla.org/r/240976/#review247258
Nits only, but since it took a bit time to understand what the patch is doing, I could take another look after nits fixed.
::: xpfe/appshell/nsXULWindow.h:147
(Diff revision 1)
> bool GetContentScrollbarVisibility();
> void PersistentAttributesDirty(uint32_t aDirtyFlags);
> nsresult GetTabCount(uint32_t* aResult);
>
> + void LoadPersistentWindowState();
> + nsresult GetPersistentValue(const nsAString& attr,
Please use Mozilla coding style. Arguments should be named aName, so aAttr and aValue. Same in Get/Set* methods in .h and .cpp
::: xpfe/appshell/nsXULWindow.cpp:1655
(Diff revision 1)
> + // Elements must have an ID to be persisted.
> + if (windowElementId.IsEmpty()) {
> + return NS_OK;
> + }
> +
> + nsCOMPtr<nsIDocument> ownerDoc = docShellElement->OwnerDoc();
OwnerDoc() never ever returns null, so no need to null check
::: xpfe/appshell/nsXULWindow.cpp:1696
(Diff revision 1)
> +
> + return NS_OK;
> +}
> +
> +nsresult
> +nsXULWindow::SetPersistentValue(const nsAString& attr,
Couldn't you just pass const nsAtom* as attr name. Then there wasn't need to do slow-ish string equals calls later. And one can always get an nsAString from nsAtom using nsDependentAtomString.
Same with GetPersistentValue
::: xpfe/appshell/nsXULWindow.cpp:1704
(Diff revision 1)
> + nsCOMPtr<dom::Element> docShellElement = GetWindowDOMElement();
> + if (!docShellElement) {
> + return NS_ERROR_FAILURE;
> + }
> +
> + nsCOMPtr<nsIDocument> ownerDoc = docShellElement->OwnerDoc();
OwnerDoc() never ever returns null, so no need to null check
Attachment #8972351 -
Flags: review?(bugs) → review-
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Comment 9•7 years ago
|
||
Also worth mentioning... We've discussed that the "persist" feature for any element should probably not be moved over from XUL document, but for the top level element it is useful. We'd eventually like to move away from xulstore for this too, but the alternatives aren't ready yet. Last, nsXULWindow is not really XUL specific anymore. We could potentially rename it or we could just merge nsXULWindow and nsWebShellWindow as nsWebShellWindow is the only class that inherits from nsXULWindow and as far as I can tell a nsXULWindow is never created by itself.
|
|
||
Comment 10•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8972351 [details]
Bug 1453788 - Allow top level HTML windows to have persistent window state.
https://reviewboard.mozilla.org/r/240976/#review247394
Attachment #8972351 -
Flags: review?(bugs) → review+
| Comment hidden (mozreview-request) |
|
||
Comment 12•7 years ago
|
||
Pushed by bdahl@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/49a11acd0acf
Allow top level HTML windows to have persistent window state. r=smaug
|
||
Comment 13•7 years ago
|
||
| bugherder | ||
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
|
||
Updated•7 years ago
|
|
||
Updated•7 years ago
|
Flags: qe-verify+
|
||
Comment 14•7 years ago
|
||
Can you please provide me with some steps to reproduce and to verify the fix?
Thank you.
Flags: needinfo?(bgrinstead)
|
|
Reporter | |
Comment 15•7 years ago
|
||
(In reply to Oana Botisan from comment #14)
> Can you please provide me with some steps to reproduce and to verify the fix?
> Thank you.
Sure:
1) Set the `devtools.browserconsole.html` pref to true
2) Open the Browser Console (ctrl+shift+j / cmd+opt+j)
3) Move where the Browser Console is on the screen (vertically and horizontally)
4) Resize the Browser Console (width and height)
5) Close the Browser Console
6) Open the Browser Console
The Browser Console window position should be restored to what it was as a result of your changes in (3) and (4).
Flags: needinfo?(bgrinstead)
|
|
||
Comment 16•7 years ago
|
||
Thank you for the info, Brian.
I couldn't reproduce this issue using Windows 10 x64 and macOS 10.13.
I managed to partially reproduce the issue on Ubuntu 16.04 x64 using an older version of Nightly (2018-04-12). The window kept is size, but the location changed.
I retested everything using the latest Nightly 63.0a1 and beta 62.0b7 on Ubuntu 16.04 x64, but the bug is still reproducing. The Browser Console window still keeps the size I set, but it changes its location on the screen.
|
|
Reporter | |
Comment 17•7 years ago
|
||
(In reply to Oana Botisan from comment #16)
> I managed to partially reproduce the issue on Ubuntu 16.04 x64 using an
> older version of Nightly (2018-04-12). The window kept is size, but the
> location changed.
> I retested everything using the latest Nightly 63.0a1 and beta 62.0b7 on
> Ubuntu 16.04 x64, but the bug is still reproducing. The Browser Console
> window still keeps the size I set, but it changes its location on the screen.
Do you see the same behavior for a normal browser window on Ubuntu? For instance: open a new browser window, move/resize it, close it, then open a new browser window again.
Flags: needinfo?(oana.botisan)
|
|
||
Comment 18•7 years ago
|
||
No, I don't get the same behaviour for a new window.
If I open a new window, move/resize it and close it, the new window I open after is the same size and it has the same place it had the first window I opened. I hope that this makes sense.
Flags: needinfo?(oana.botisan)
|
|
Reporter | |
Comment 19•7 years ago
|
||
Brendan, can you reproduce Comment 16 on Ubuntu?
Flags: needinfo?(bdahl)
|
|
Assignee | |
Comment 20•7 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #19)
> Brendan, can you reproduce Comment 16 on Ubuntu?
On ubuntu I get the same behavior regardless of what `devtools.browserconsole.html` is set to. It always remembers size but not position. I think we should file a new bug for that issue.
Flags: needinfo?(bdahl)
|
|
||
Comment 21•6 years ago
|
||
I filled bug 1533386 for the issue with the location of the window.
Under this circumstances, I will mark this bug as verified fixed and the issue regarding the location of the window can be follow in bug 1533386.
Status: RESOLVED → VERIFIED
status-firefox65:
--- → verified
status-firefox66:
--- → verified
status-firefox67:
--- → verified
Flags: qe-verify+
You need to log in
before you can comment on or make changes to this bug.
Description
•