Closed Bug 1453788 Opened 6 years ago Closed 6 years ago

Remember window size and location for chrome HTML windows

Categories

(Firefox :: General, enhancement)

Product:
Firefox
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 62
Tracking Flags:
Tracking Status
firefox62 --- fixed
firefox65 --- verified
firefox66 --- verified
firefox67 --- verified

People

(Reporter: bgrins, Assigned: bdahl)

References

Details

Attachments

(1 file)

Bug 1453788 - Allow top level HTML windows to have persistent window state.
59 bytes, text/x-review-board-request
smaug
: review+
Details 
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.
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);
(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....
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).
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: nobody → bdahl
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-
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 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+
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
https://hg.mozilla.org/mozilla-central/rev/49a11acd0acf
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
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)
(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.
(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)
Brendan, can you reproduce Comment 16 on Ubuntu?
Flags: needinfo?(bdahl)
Depends on: 1475262
(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
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.

Attachment

General

Creator:
Created:
Updated:
Size: