Skip to content

/ servo

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

Jump to bottom

Screen width and height are undefined #18062

Closed
BojanKogoj opened this issue Aug 13, 2017 · 9 comments
Closed

Screen width and height are undefined #18062

BojanKogoj opened this issue Aug 13, 2017 · 9 comments
Labels
A-content/dom C-has-manual-testcase

Comments

@BojanKogoj
Copy link
Contributor

@BojanKogoj BojanKogoj commented Aug 13, 2017 •
edited

There is an issue with screen width and height, first noticed on whoishostingthis.com/tools/user-agent/, screenshot (left Servo, right Firefox 55):

screen-size-issue2

I tried to reproduce using a test script and got the following result (left Servo, right Firefox 55):

screen-size-issue

Test script used to reproduce:

<!DOCTYPE html>
<html>
    <head>
        <meta charset="utf-8">
        <title>Screen size test</title>
        <style>
            html {
                font-family: "Helvetica Neue",Helvetica,Arial,sans-serif;
            }
            .container {
                width: 600px;
                margin: auto auto;
            }
            h1 {
                margin: 50px 0;
                font-size: 30px;
                text-align: center
            }
            dt {
                width: 260px;
                float: left;
                font-weight: bold;
            }
            dd {
                margin-bottom: 10px;
                margin-left: 280px;
                clear: right;
                height: 20px;
            }
        </style>
    </head>
    <body>
        <div class="container">
            <h1>Screen size test</h1>
            <dl id="values">
            </dl>
            <strong id="error">If you can see this there is an issue and values are not properly loaded</strong>
        </div>
        <script>
            function setElement(name, value) {
                var dt = document.createElement('dt');
                dt.textContent = name;
                var dd = document.createElement('dd');
                dd.textContent = value;

                var outer = document.getElementById('values');
                outer.appendChild(dt);
                outer.appendChild(dd);
            }
            window.onload = function() {
                setElement('window.screen.availWidth', window.screen.availWidth);
                setElement('window.screen.availHeight', window.screen.availHeight);
                setElement('window.screen.width', window.screen.width);
                setElement('window.screen.height', window.screen.height);
                setElement('window.innerWidth', window.innerWidth);
                setElement('window.innerHeight', window.innerHeight);
                setElement('window.outerWidth', window.outerWidth);
                setElement('window.outerHeight', window.outerHeight);
                setElement('screen.colorDepth', screen.colorDepth);
                setElement('screen.pixelDepth', screen.pixelDepth);

                // Remove error message
                var err = document.getElementById('error');
                err.parentElement.removeChild(err);
            }
        </script>
    </body>
</html>

Tested with Servo developer preview on Windows 10.
@jdm
Copy link
Member

@jdm jdm commented Aug 17, 2017 •
edited

Yes, those attributes are missing from our implementation of the Screen interface:

Code: https://dxr.mozilla.org/servo/rev/4d10d39e8fe841c5fe2ac58da2daaa13c10c140e/components/script/dom/webidls/Screen.webidl#8-11
Specification: https://drafts.csswg.org/cssom-view/#dom-screen-availwidth
@jdm
Copy link
Member

@jdm jdm commented Aug 18, 2017

The important part here is getting the screen dimensions. Glutin has a MonitorId type that has a get_dimensions method; we can use the Window::client_window method as a model here (components/script/dom/window.rs) - we need to send a message to the constellation, which will forward the message to the compositor, which can call glutin::get_primary_monitor and return the appropriate information.
@shinglyu
Copy link
Member

@shinglyu shinglyu commented Aug 21, 2017

I guess I can give this a go while I wait my other bugs to be unblocked.

Do we have to worry about other embedders? Is there a good point to abstract away glutin?
@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Aug 21, 2017 •
edited

@shinglyu using a WindowMethod with a glutin specific implementation in ports is totally fine.
@shinglyu
Copy link
Member

@shinglyu shinglyu commented Aug 21, 2017

@paulrouget Ah I see, so the IOCompositor.window can have different implementation of WindowMethods.
@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Aug 21, 2017

@shinglyu yep. All located in /ports/.
@shinglyu
Copy link
Member

@shinglyu shinglyu commented Aug 21, 2017

I wrote a working prototype for screen.width and height using glutin::get_primary_monitor().get_dimensions(). But I'm not sure how to get the availHeight/availWidth. Is there API for getting the available height/width? Or maybe we just implement screen.height/width for now?
@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Aug 21, 2017

As far as I know, winit/glutin doesn't provide the info for availableHeight/Width.
Maybe fallback to the values of height/width for now?
@shinglyu
Copy link
Member

@shinglyu shinglyu commented Aug 22, 2017

One thing that confuses me is that there are tests like /cssom-view-1_dev/xhtml1/window-screen-height-mutation-throws.xht asserting that mutating window.screen.height should throw an exception. But I can't reproduce that on Chrome and Firefox. Is that an outdated test?
@shinglyu shinglyu mentioned this issue Aug 22, 2017
Merged
4 of 5 tasks complete
bors-servo added a commit that referenced this issue Aug 31, 2017
@bors-servo
Auto merge of #18183 - shinglyu:availheight, r=asajeffrey
Loading status checks…
f627526 
Enable screen.width/height/availWidth/availHeight

<!-- Please describe your changes on the following line: -->
Support screen.width/height/availWidth/availHeight using information from glutin. r?@paulrouget

Since glutin don't have `availWidth` and `availHeight` information, I let them fallback to `width` and `height`. If that's not acceptable in terms of webcompat, I can remove that part.

There are some test failures on wpt css about mutating screen.width/height should throw exception, but I can't reproduce that behavior on other major browser, so I keep them disabled. Also there are some media query issues, but I believe that's due to some test harness problem on my test machine.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #18062  (github issue number if applicable).

<!-- Either: -->
- [x] There are tests for these changes in wpt cssom-view
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18183)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Sep 1, 2017
@bors-servo
Auto merge of #18183 - shinglyu:availheight, r=asajeffrey
Loading status checks…
297f474 
Enable screen.width/height/availWidth/availHeight

<!-- Please describe your changes on the following line: -->
Support screen.width/height/availWidth/availHeight using information from glutin. r?@paulrouget

Since glutin don't have `availWidth` and `availHeight` information, I let them fallback to `width` and `height`. If that's not acceptable in terms of webcompat, I can remove that part.

There are some test failures on wpt css about mutating screen.width/height should throw exception, but I can't reproduce that behavior on other major browser, so I keep them disabled. Also there are some media query issues, but I believe that's due to some test harness problem on my test machine.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #18062  (github issue number if applicable).

<!-- Either: -->
- [x] There are tests for these changes in wpt cssom-view
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18183)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Sep 1, 2017
@bors-servo
Auto merge of #18183 - shinglyu:availheight, r=asajeffrey
Loading status checks…
5f21b16 
Enable screen.width/height/availWidth/availHeight

<!-- Please describe your changes on the following line: -->
Support screen.width/height/availWidth/availHeight using information from glutin. r?@paulrouget

Since glutin don't have `availWidth` and `availHeight` information, I let them fallback to `width` and `height`. If that's not acceptable in terms of webcompat, I can remove that part.

There are some test failures on wpt css about mutating screen.width/height should throw exception, but I can't reproduce that behavior on other major browser, so I keep them disabled. Also there are some media query issues, but I believe that's due to some test harness problem on my test machine.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #18062  (github issue number if applicable).

<!-- Either: -->
- [x] There are tests for these changes in wpt cssom-view
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18183)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Sep 11, 2017
@bors-servo
Auto merge of #18183 - shinglyu:availheight, r=<try>
Loading status checks…
84681e3 
Enable screen.width/height/availWidth/availHeight

<!-- Please describe your changes on the following line: -->
Support screen.width/height/availWidth/availHeight using information from glutin. r?@paulrouget

Since glutin don't have `availWidth` and `availHeight` information, I let them fallback to `width` and `height`. If that's not acceptable in terms of webcompat, I can remove that part.

There are some test failures on wpt css about mutating screen.width/height should throw exception, but I can't reproduce that behavior on other major browser, so I keep them disabled. Also there are some media query issues, but I believe that's due to some test harness problem on my test machine.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #18062  (github issue number if applicable).

<!-- Either: -->
- [x] There are tests for these changes in wpt cssom-view
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18183)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Sep 28, 2017
@bors-servo
Auto merge of #18183 - shinglyu:availheight, r=<try>
Loading status checks…
8d4f7f2 
Enable screen.width/height/availWidth/availHeight

<!-- Please describe your changes on the following line: -->
Support screen.width/height/availWidth/availHeight using information from glutin. r?@paulrouget

Since glutin don't have `availWidth` and `availHeight` information, I let them fallback to `width` and `height`. If that's not acceptable in terms of webcompat, I can remove that part.

There are some test failures on wpt css about mutating screen.width/height should throw exception, but I can't reproduce that behavior on other major browser, so I keep them disabled. Also there are some media query issues, but I believe that's due to some test harness problem on my test machine.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #18062  (github issue number if applicable).

<!-- Either: -->
- [x] There are tests for these changes in wpt cssom-view
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18183)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Oct 21, 2017
@bors-servo
Auto merge of #18183 - shinglyu:availheight, r=asajeffrey
Loading status checks…
cb17ee3 
Enable screen.width/height/availWidth/availHeight

<!-- Please describe your changes on the following line: -->
Support screen.width/height/availWidth/availHeight using information from glutin. r?@paulrouget

Since glutin don't have `availWidth` and `availHeight` information, I let them fallback to `width` and `height`. If that's not acceptable in terms of webcompat, I can remove that part.

There are some test failures on wpt css about mutating screen.width/height should throw exception, but I can't reproduce that behavior on other major browser, so I keep them disabled. Also there are some media query issues, but I believe that's due to some test harness problem on my test machine.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #18062  (github issue number if applicable).

<!-- Either: -->
- [x] There are tests for these changes in wpt cssom-view
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18183)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Nov 8, 2017
@bors-servo
Auto merge of #18183 - shinglyu:availheight, r=asajeffrey
Loading status checks…
f01cb93 
Enable screen.width/height/availWidth/availHeight

<!-- Please describe your changes on the following line: -->
Support screen.width/height/availWidth/availHeight using information from glutin. r?@paulrouget

Since glutin don't have `availWidth` and `availHeight` information, I let them fallback to `width` and `height`. If that's not acceptable in terms of webcompat, I can remove that part.

There are some test failures on wpt css about mutating screen.width/height should throw exception, but I can't reproduce that behavior on other major browser, so I keep them disabled. Also there are some media query issues, but I believe that's due to some test harness problem on my test machine.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #18062  (github issue number if applicable).

<!-- Either: -->
- [x] There are tests for these changes in wpt cssom-view
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18183)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Nov 8, 2017
@bors-servo
Auto merge of #18183 - shinglyu:availheight, r=asajeffrey
Loading status checks…
9d2be4a 
Enable screen.width/height/availWidth/availHeight

<!-- Please describe your changes on the following line: -->
Support screen.width/height/availWidth/availHeight using information from glutin. r?@paulrouget

Since glutin don't have `availWidth` and `availHeight` information, I let them fallback to `width` and `height`. If that's not acceptable in terms of webcompat, I can remove that part.

There are some test failures on wpt css about mutating screen.width/height should throw exception, but I can't reproduce that behavior on other major browser, so I keep them disabled. Also there are some media query issues, but I believe that's due to some test harness problem on my test machine.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #18062  (github issue number if applicable).

<!-- Either: -->
- [x] There are tests for these changes in wpt cssom-view
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18183)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Nov 12, 2017
@bors-servo
Auto merge of #18183 - shinglyu:availheight, r=<try>
Loading status checks…
db31197 
Enable screen.width/height/availWidth/availHeight

<!-- Please describe your changes on the following line: -->
Support screen.width/height/availWidth/availHeight using information from glutin. r?@paulrouget

Since glutin don't have `availWidth` and `availHeight` information, I let them fallback to `width` and `height`. If that's not acceptable in terms of webcompat, I can remove that part.

There are some test failures on wpt css about mutating screen.width/height should throw exception, but I can't reproduce that behavior on other major browser, so I keep them disabled. Also there are some media query issues, but I believe that's due to some test harness problem on my test machine.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #18062  (github issue number if applicable).

<!-- Either: -->
- [x] There are tests for these changes in wpt cssom-view
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18183)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Nov 13, 2017
@bors-servo
Auto merge of #18183 - shinglyu:availheight, r=<try>
Loading status checks…
4a8e0ac 
Enable screen.width/height/availWidth/availHeight

<!-- Please describe your changes on the following line: -->
Support screen.width/height/availWidth/availHeight using information from glutin. r?@paulrouget

Since glutin don't have `availWidth` and `availHeight` information, I let them fallback to `width` and `height`. If that's not acceptable in terms of webcompat, I can remove that part.

There are some test failures on wpt css about mutating screen.width/height should throw exception, but I can't reproduce that behavior on other major browser, so I keep them disabled. Also there are some media query issues, but I believe that's due to some test harness problem on my test machine.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #18062  (github issue number if applicable).

<!-- Either: -->
- [x] There are tests for these changes in wpt cssom-view
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18183)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Nov 13, 2017
@bors-servo
Auto merge of #18183 - shinglyu:availheight, r=asajeffrey
Loading status checks…
504ad4d 
Enable screen.width/height/availWidth/availHeight

<!-- Please describe your changes on the following line: -->
Support screen.width/height/availWidth/availHeight using information from glutin. r?@paulrouget

Since glutin don't have `availWidth` and `availHeight` information, I let them fallback to `width` and `height`. If that's not acceptable in terms of webcompat, I can remove that part.

There are some test failures on wpt css about mutating screen.width/height should throw exception, but I can't reproduce that behavior on other major browser, so I keep them disabled. Also there are some media query issues, but I believe that's due to some test harness problem on my test machine.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #18062  (github issue number if applicable).

<!-- Either: -->
- [x] There are tests for these changes in wpt cssom-view
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18183)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-content/dom C-has-manual-testcase
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

Enable screen.width/height/availWidth/availHeight
4 participants
@jdm @paulrouget @BojanKogoj @shinglyu
You can’t perform that action at this time.