Remember window size and location for chrome HTML windows

VERIFIED FIXED in Firefox 62

Status

()

enhancement
VERIFIED FIXED
a year ago
3 months ago

People

(Reporter: bgrins, Assigned: bdahl)

Tracking

unspecified
Firefox 62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed, firefox65 verified, firefox66 verified, firefox67 verified)

Details

Attachments

(1 attachment)

Reporter

Description

a year ago
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

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

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

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

a year 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.
Component: DOM → General
Product: Core → Firefox
Assignee

Updated

a year ago
Assignee: nobody → bdahl
Comment hidden (mozreview-request)

Comment 7

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

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

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

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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/49a11acd0acf
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Reporter

Updated

a year ago
Blocks: 1460663
No longer blocks: 1437647
Depends on: 1437647
Flags: qe-verify+
Can you please provide me with some steps to reproduce and to verify the fix?
Thank you.
Flags: needinfo?(bgrinstead)
Reporter

Comment 15

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

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

11 months ago
Brendan, can you reproduce Comment 16 on Ubuntu?
Flags: needinfo?(bdahl)

Updated

11 months ago
Depends on: 1475262
Assignee

Comment 20

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

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
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.