-
Notifications
You must be signed in to change notification settings - Fork 22.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DO NOT MERGE: Technical review for Window Management API docs #28851
Conversation
@michaelwasserman hi there Michael. Please can you give me a technical review for these docs? The best place to start is #28851 (comment), which provides live previews of all the new/updated pages. Sorry for the out-of-the-blue nature of this request. @tomayac said you would be the best person to ask ;-) |
This is awesome, thank you! I started a detailed review, but can I ask for your patience until the week of October 9th? |
@michaelwasserman Yup, w/c October 9th is absolutely fine. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is AMAZING, thank you very much! I left some comments that I hope are helpful, and I'm happy to circle back with another look or discussion as appropriate.
@@ -2214,17 +2214,17 @@ | |||
/en-US/docs/DOM/window.restore /en-US/docs/Web/API/Window/moveTo | |||
/en-US/docs/DOM/window.screen /en-US/docs/Web/API/Window/screen | |||
/en-US/docs/DOM/window.screen.availHeight /en-US/docs/Web/API/Screen/availHeight | |||
/en-US/docs/DOM/window.screen.availLeft /en-US/docs/Web/API/Screen/availLeft | |||
/en-US/docs/DOM/window.screen.availTop /en-US/docs/Web/API/Screen/availTop | |||
/en-US/docs/DOM/window.screen.availLeft /en-US/docs/Web/API/ScreenDetailed/availLeft |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't redirect folks looking for window.screen.availLeft (etc.) to the ScreenDetailed definition; they should continue to see existing https://developer.mozilla.org/en-US/docs/Web/API/Screen/availLeft (etc.) docs about the pre-existing non-standard properties defined on the Screen interface object window.screen. Right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm, this is a difficult situation to decide what is best. I do see your point, but at the same time, I feel that it is good to document the standardized version over the non-standard version in such cases. Plus, it is good to document features in one place rather than two otherwise you run the risk of reader confusion and maintenance difficulties.
I have thought about this a bit, and come to the following executive decision:
- I am going to keep both sets of compat data for the affected properties. To represent both stories in the compat data in one place is too confusing and tricky, and will confuse more than it helps. (I tried to do so yesterday for quite a while, and just gave up in the end!)
- I am keep the documentation for the affected properties in one place — hanging off
ScreenDetailed
. - I will add a note and a section to each of these pages to make the non-standard version support situation clear.
- I will add a section to the main
Screen
page listing the non-standard versions and pointing to where they are documented.
Let me know what you think of these changes (in the next commit).
One more comment: The API isn't currently supported on Chrome mobile, so "Browser compatibility" sections might need updating. |
@michaelwasserman oops! I've actually started responding to your review by fixing this one; see mdn/browser-compat-data#20967. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, with some hopefully helpful comments. Thank you very much!!!
files/en-us/web/api/screen/index.md
Outdated
@@ -38,6 +38,19 @@ _Also inherits properties from its parent {{domxref("EventTarget")}}_. | |||
- {{DOMxRef("Screen.mozBrightness")}} {{Non-standard_Inline}} {{Deprecated_Inline}} | |||
- : Controls the brightness of a device's screen. A double between 0 and 1.0 is expected. | |||
|
|||
## Non-standard properties | |||
|
|||
The following properties are standardized as part of the [Window Management API](/en-US/docs/Web/API/Window_Management_API), thus we have chosen to document their standard form, which is available on the {{domxref("ScreenDetailed")}} interface. However, non-standard versions of these properties are available on the `Screen` interface. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Standardized may imply stronger meaning than the reality? Window Management is "on the standards track". FWIW, an API goal would be to standardize the properties on Screen, then ScreenDetailed would just inherit those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean here. I have updated the wording to the following:
The following properties are specified as part of the Window Management API, which makes them available on the {{domxref("ScreenDetailed")}} interface; this is where we have chosen to document them. However, non-standard versions of these properties are available on the
Screen
interface in browsers that don't support that API. See this page's Browser compatibility table for details of the non-standard support.
@@ -15,8 +15,6 @@ interface moves the current window to the specified coordinates. | |||
> contrast, {{domxref("window.moveBy()")}} moves the window relative to its current | |||
> location. | |||
|
|||
> **Note:** On devices with multiple displays, `moveTo()` positions windows relative to the [multi-screen origin](/en-US/docs/Web/API/Window_Management_API#multi-screen_origin). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, these notes did seem somewhat helpful, if difficult to qualify with compat data.
Perhaps leaving them in, specific to Chromium based browsers would be helpful? Also, optionally noting user agents might clamp requested coordinates to the current/opener display (depending on window-management permission in Chromium based browser), etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above. I think I'm going to leave the clamping behavior for now, and perhaps consider it at a later date.
Thanks for the 2nd round of comments, @michaelwasserman. I've fixed those up now. Let me know if there are any other changes you'd like to see. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thank you very much! (I don't have rights to "Approve")
This is fantastic documentation for the Window Management API, and a great step forward documenting the associated non-standard legacy API surfaces and implementation differences. This will be a great resources for web developers implementing multi-window and multi-screen functionality.
@michaelwasserman wonderful, thank you! I have enjoyed working with you on this Mike; give me a shout in future if/when you work on other items that you'd like to see documented on MDN. OK, so you've given the official engineering approval. Next we are going to try something new — I'm going to open a new PR based on this same branch so that we can keep the corresponding editorial review of this work separate, to try to keeps things a bit more clean (as these PRs can get really noisy). Just mentioning it so that you know what is going on. |
I think I need to close this one so I can open another. |
Note: This technical review is now completed and approved. For the follow-on editorial review, see #29719
Description
The Window Management API was implemented in Chrome 100 (see ChromeStatus entry); this PR provides docs for it.
See my research doc for complete details of the items that have been added.
Motivation
Additional details
Also see:
Related issues and pull requests