| Summary: | window.innerWidth/Height should not include page scale | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Sami Kyostila <skyostil> | ||||||||||
| Component: | DOM | Assignee: | Kenneth Rohde Christiansen <kenneth> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | allan.jensen, anilsson, anssi.kostiainen, ap, ddkilzer, dglazkov, efidler, fsamuel, kenneth, manyoso, peter, simon.fraser, tonikitoo, webkit.review.bot | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Bug Depends on: | |||||||||||||
| Bug Blocks: | 68075 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Sami Kyostila
2012-01-18 11:08:18 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? Created attachment 122969 [details]
Patch
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 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. (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 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 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 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 Created attachment 123303 [details]
Patch
- Added note to ChangeLog about modifying two existing tests.
Comment on attachment 123303 [details] Patch Clearing flags on attachment: 123303 Committed r105512: <http://trac.webkit.org/changeset/105512> All reviewed patches have been landed. Closing bug. (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. > 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? (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) (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. 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. Reopening to attach new patch. Created attachment 132058 [details]
Patch
Comment on attachment 132058 [details]
Patch
Landed in 110981.
|