Bug 76555 - window.innerWidth/Height should not include page scale
: window.innerWidth/Height should not include page scale
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: HTML DOM
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Kenneth Rohde Christiansen
:
Depends on:
Blocks: 68075
  Show dependency treegraph
 
Reported: 2012-01-18 11:08 PST by Sami Kyostila
Modified: 2012-03-16 03:35 PDT (History)
14 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Sami Kyostila 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 Kenneth Rohde Christiansen 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 Sami Kyostila 2012-01-18 11:17:46 PST
Created attachment 122969 [details]
Patch
Comment 3 Kenneth Rohde Christiansen 2012-01-18 11:22:59 PST
Comment on attachment 122969 [details]
Patch

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 Sami Kyostila 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 Sami Kyostila 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 WebKit Review Bot 2012-01-18 12:22:22 PST
Comment on attachment 122969 [details]
Patch

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 Sami Kyostila 2012-01-20 04:17:43 PST
Created attachment 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 Kenneth Rohde Christiansen 2012-01-20 04:27:41 PST
Comment on attachment 123285 [details]
Patch

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 Sami Kyostila 2012-01-20 06:13:23 PST
Created attachment 123303 [details]
Patch

 - Added note to ChangeLog about modifying two existing tests.
Comment 10 WebKit Review Bot 2012-01-20 07:03:37 PST
Comment on attachment 123303 [details]
Patch

Clearing flags on attachment: 123303

Committed r105512: <http://trac.webkit.org/changeset/105512>
Comment 11 WebKit Review Bot 2012-01-20 07:03:42 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Kenneth Rohde Christiansen 2012-03-15 07:46:11 PDT
(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 Sami Kyostila 2012-03-15 08:01:10 PDT
> 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 Kenneth Rohde Christiansen 2012-03-15 08:15:50 PDT
(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 Sami Kyostila 2012-03-15 08:25:34 PDT
(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 Allan Sandfeld Jensen 2012-03-15 08:37:07 PDT
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 Kenneth Rohde Christiansen 2012-03-15 08:59:32 PDT
Reopening to attach new patch.
Comment 18 Kenneth Rohde Christiansen 2012-03-15 08:59:37 PDT
Created attachment 132058 [details]
Patch
Comment 19 Kenneth Rohde Christiansen 2012-03-16 03:35:03 PDT
Comment on attachment 132058 [details]
Patch

Landed in 110981.