Bug 76555 - window.innerWidth/Height should not include page scale
: window.innerWidth/Height should not include page scale
Status: RESOLVED FIXED
: WebKit
HTML DOM
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
: 68075
  Show dependency treegraph
 
Reported: 2012-01-18 11:08 PST by
Modified: 2012-03-16 03:35 PST (History)


Attachments
Patch (7.61 KB, patch)
2012-01-18 11:17 PST, Sami Kyostila
no flags Review Patch | Details | Formatted Diff | Diff
Patch (9.62 KB, patch)
2012-01-20 04:17 PST, Sami Kyostila
no flags Review Patch | Details | Formatted Diff | Diff
Patch (9.84 KB, patch)
2012-01-20 06:13 PST, Sami Kyostila
no flags Review Patch | Details | Formatted Diff | Diff
Patch (4.68 KB, patch)
2012-03-15 08:59 PST, Kenneth Rohde Christiansen
hausmann: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-01-18 11:08:18 PST
The values of window.innerWidth and window.innerHeight seem to return CSS pixels multiplied with the page scale. I have not found an authoritative reference on whether these values should include page scale or not, but seeing as 1) the effects of text zoom is already cancelled out and 2) providing scaled CSS pixel values to web developers does not seem useful, I believe the page scale should not be included.
------- Comment #1 From 2012-01-18 11:13:06 PST -------
I tend to agree with this. What should happen if I do a css -webkit-transform: scale(3) on an iframe? what should its innerWidth return?
------- Comment #2 From 2012-01-18 11:17:46 PST -------
Created an attachment (id=122969) [details]
Patch
------- Comment #3 From 2012-01-18 11:22:59 PST -------
(From update of attachment 122969 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=122969&action=review

> Source/WebCore/page/DOMWindow.cpp:1129
> +    return static_cast<int>(view->visibleContentRect(/* includeScrollbars */ true).height() / (m_frame->pageZoomFactor() * m_frame->frameScaleFactor()));

looks correct

> LayoutTests/fast/dom/iframe-inner-size-scaling.html:5
> +<!DOCTYPE html>
> +<html>
> +<head>
> +    <script>
> +        function debug(str) {

I think there is some smarter way for doing script only tests, though I havent done any myself

> LayoutTests/fast/dom/iframe-inner-size-scaling.html:14
> +            // We must use setTimeout since the innerWidth/innerHeight are not yet valid for the iframe.
> +            window.setTimeout(runTests, 0);

Will this trick always work? We really don't want more flaky tests :-)

> LayoutTests/fast/dom/iframe-inner-size-scaling.html:40
> +            debug("SUCCESS!");

log*, but is not debugging
------- Comment #4 From 2012-01-18 11:23:44 PST -------
I admit I haven't thought about CSS transforms on conjunction with this too deeply, but intuitively it seems that any transform should not affect the value of  innerWidth/Height since the transform does not alter the available CSS coordinate range inside the iframe.
------- Comment #5 From 2012-01-18 11:26:49 PST -------
(In reply to comment #3)
> I think there is some smarter way for doing script only tests, though I havent done any myself

Hmm, I'll have to dig around to find some better examples then. This one is based on fast/dom/client-width-height.html.

> > LayoutTests/fast/dom/iframe-inner-size-scaling.html:14
> > +            // We must use setTimeout since the innerWidth/innerHeight are not yet valid for the iframe.
> > +            window.setTimeout(runTests, 0);
> 
> Will this trick always work? We really don't want more flaky tests :-)

All I can say is that it has worked reliably for me :) If you can suggest a better way then I'm all ears.

> > LayoutTests/fast/dom/iframe-inner-size-scaling.html:40
> > +            debug("SUCCESS!");
> 
> log*, but is not debugging

Sure, I'll fix this.
------- Comment #6 From 2012-01-18 12:22:22 PST -------
(From update of attachment 122969 [details])
Attachment 122969 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11194327

New failing tests:
fast/frames/frame-set-scaling-hit.html
fast/frames/frame-set-rotation-hit.html
------- Comment #7 From 2012-01-20 04:17:43 PST -------
Created an attachment (id=123285) [details]
Patch

 - Rebased.
 - Fixed two failing tests. (The panel geometry needed to be calculated before changing the visual viewport.)
 - Rewrote tests to use existing JS infrastructure.
------- Comment #8 From 2012-01-20 04:27:41 PST -------
(From update of attachment 123285 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=123285&action=review

> LayoutTests/ChangeLog:14
> +        with page scaling.
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * fast/dom/iframe-inner-size-scaling-expected.txt: Added.
> +        * fast/dom/iframe-inner-size-scaling.html: Added.
> +        * fast/dom/window-inner-size-scaling-expected.txt: Added.
> +        * fast/dom/window-inner-size-scaling.html: Added.

the changelog doesnt mention that you modified other tests or why
------- Comment #9 From 2012-01-20 06:13:23 PST -------
Created an attachment (id=123303) [details]
Patch

 - Added note to ChangeLog about modifying two existing tests.
------- Comment #10 From 2012-01-20 07:03:37 PST -------
(From update of attachment 123303 [details])
Clearing flags on attachment: 123303

Committed r105512: <http://trac.webkit.org/changeset/105512>
------- Comment #11 From 2012-01-20 07:03:42 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #12 From 2012-03-15 07:46:11 PST -------
(In reply to comment #0)
> The values of window.innerWidth and window.innerHeight seem to return CSS pixels multiplied with the page scale. I have not found an authoritative reference on whether these values should include page scale or not, but seeing as 1) the effects of text zoom is already cancelled out and 2) providing scaled CSS pixel values to web developers does not seem useful, I believe the page scale should not be included.

I am wondering whether this change we made makes sense as it seems be differ from most widespread mobile implementations.

http://www.quirksmode.org/mobile/tableViewport.html

"Dimensions of the visual viewport in CSS pixels."

This is also what current OS X Safari supports.
------- Comment #13 From 2012-03-15 08:01:10 PST -------
> I am wondering whether this change we made makes sense as it seems be differ from most widespread mobile implementations.
>
> "Dimensions of the visual viewport in CSS pixels."

Is this not what we are currently returning -- CSS pixels without the page scale applied? Or are you referring to the scrollbar bug 81208?
------- Comment #14 From 2012-03-15 08:15:50 PST -------
(In reply to comment #13)
> > I am wondering whether this change we made makes sense as it seems be differ from most widespread mobile implementations.
> >
> > "Dimensions of the visual viewport in CSS pixels."
> 
> Is this not what we are currently returning -- CSS pixels without the page scale applied? Or are you referring to the scrollbar bug 81208?

I think you are correct. It is just very confusing that visibleContentRect doesn't return the rect in CSS units but in layout units.

We should either add a comment or create a mapping method

int FrameView::mapFromLayoutToCSSUnits(LayoutUnit value)
------- Comment #15 From 2012-03-15 08:25:34 PST -------
(In reply to comment #14)
> We should either add a comment or create a mapping method
> 
> int FrameView::mapFromLayoutToCSSUnits(LayoutUnit value)

I like this. Being more explicit about the coordinate systems is a good idea.
------- Comment #16 From 2012-03-15 08:37:07 PST -------
If I understand this right, the page scale unlike page zoom, does change the size of the viewport. Like page zoom it changes the size of CSS pixels, but also changes the size of the viewport (allowing overflow and underflow). 

The question is if the visual viewport in this case, is the unscaled viewport in which case this patch is wrong and the original behaviour right? OR if the we should indeed give the webpage the scaled viewport?

Since page scale is supposed to be transparent to the web application, I think it would make more sense if they got the size of the original unscaled viewport. This value happens to also be thenew  visual viewport in device pixels, but that is not the point: It is the logical viewport in CSS pixels.
------- Comment #17 From 2012-03-15 08:59:32 PST -------
Reopening to attach new patch.
------- Comment #18 From 2012-03-15 08:59:37 PST -------
Created an attachment (id=132058) [details]
Patch
------- Comment #19 From 2012-03-16 03:35:03 PST -------
(From update of attachment 132058 [details])
Landed in 110981.