76555 – window.innerWidth/Height should not include page scale

RESOLVED FIXED Bug 76555

window.innerWidth/Height should not include page scale

https://bugs.webkit.org/show_bug.cgi?id=76555

Summary window.innerWidth/Height should not include page scale

Sami Kyostila

Reported 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.

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
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Kenneth Rohde Christiansen

Comment 1 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?

Sami Kyostila

Comment 2 2012-01-18 11:17:46 PST

Created attachment 122969 [details] Patch

Kenneth Rohde Christiansen

Comment 3 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

Sami Kyostila

Comment 4 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.

Sami Kyostila

Comment 5 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.

WebKit Review Bot

Comment 6 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

Sami Kyostila

Comment 7 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.

Kenneth Rohde Christiansen

Comment 8 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

Sami Kyostila

Comment 9 2012-01-20 06:13:23 PST

Created attachment 123303 [details] Patch - Added note to ChangeLog about modifying two existing tests.

WebKit Review Bot

Comment 10 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>

WebKit Review Bot

Comment 11 2012-01-20 07:03:42 PST

All reviewed patches have been landed. Closing bug.

Kenneth Rohde Christiansen

Comment 12 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.

Sami Kyostila

Comment 13 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?

Kenneth Rohde Christiansen

Comment 14 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)

Sami Kyostila

Comment 15 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.

Allan Sandfeld Jensen

Comment 16 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.

Kenneth Rohde Christiansen

Comment 17 2012-03-15 08:59:32 PDT

Reopening to attach new patch.

Kenneth Rohde Christiansen

Comment 18 2012-03-15 08:59:37 PDT

Created attachment 132058 [details] Patch

Kenneth Rohde Christiansen

Comment 19 2012-03-16 03:35:03 PDT

Comment on attachment 132058 [details] Patch Landed in 110981.

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

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component DOM

Assignee

Kenneth Rohde Christiansen

Reported

2012-01-18 11:08 PST

Modified

2012-03-16 03:35 PDT History

CC List

14 users Show

URL

Keywords

Depends on

Blocks

68075

Dependancies

tree graph