Bug 72591

Summary: Remove document.width / document.height
Product: WebKit Reporter: Anne van Kesteren <annevk>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, arv, brianh.smartsheet, code.vineet, darin, dglazkov, divya, dominicc, mitz, Ms2ger, ojan, sam, tabatkins, webkit.review.bot, yonathan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
work in progress
webkit.review.bot: commit-queue-
proposed patch
abarth: review-
Updated patch as per review comments
abarth: review+, abarth: commit-queue-
updated_patch none

Description Anne van Kesteren 2011-11-17 02:53:29 PST
WebKit is the only vendor who remains to have support for this.
Comment 1 Anne van Kesteren 2011-11-17 03:05:21 PST
abarth says they're on HTMLDocument.
Comment 2 Adam Barth 2011-11-17 03:06:38 PST
[02:54am] annevk: jarek: only Gecko and WebKit supported them
[02:54am] annevk: jarek: some Google-demo is still using them it seems, but not much more than that as far as I know
[03:00am] abarth: annevk: which demo?
[03:01am] annevk: http://studio.html5rocks.com/#Puzzle because of code in http://studio.html5rocks.com/samples/svg-puzzle/jigsaw.js
[03:01am] annevk: instead of document.width it should use document.body.clientWidth and same for height
Comment 3 Adam Barth 2011-11-17 03:08:50 PST
Created attachment 115551 [details]
work in progress
Comment 4 Vineet Chaudhary (vineetc) 2011-11-17 04:39:05 PST
May be we need to remove below line from Source/WebCore/bindings/objc/PublicDOMInterfaces.h 

@property(readonly) int width;
@property(readonly) int height;
Comment 5 WebKit Review Bot 2011-11-17 05:19:01 PST
Comment on attachment 115551 [details]
work in progress

Attachment 115551 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10477160

New failing tests:
fast/dom/document-width-height-force-layout.html
fullscreen/video-specified-size.html
Comment 6 Adam Barth 2011-11-17 10:22:23 PST
If these functions are exposed in the ObjC bindings, then we can't remove the implementation.  We could potentially restrict the API to just ObjC though.
Comment 7 Dominic Cooney 2011-11-17 22:14:00 PST
(In reply to comment #6)
> If these functions are exposed in the ObjC bindings, then we can't remove the implementation.  We could potentially restrict the API to just ObjC though.

This is what we did for initOverflowEvent in bug 71687. It was public Objective C API; it is now in the Objective C binding only.
Comment 8 Vineet Chaudhary (vineetc) 2011-11-18 12:27:33 PST
Created attachment 115847 [details]
proposed patch

(In reply to comment #7)
> (In reply to comment #6)
> > If these functions are exposed in the ObjC bindings, then we can't remove the implementation.  We could potentially restrict the API to just ObjC though.
> 
> This is what we did for initOverflowEvent in bug 71687. It was public Objective C API; it is now in the Objective C binding only.

Attaching as per above review comments. Moved height/width attribute under "LANGUAGE_OBJECTIVE_C" 
Please let me know your comments on this.
Comment 9 Adam Barth 2011-11-18 15:10:44 PST
Comment on attachment 115847 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115847&action=review

> LayoutTests/fast/dom/document-width-height-force-layout.html:-15
> -            log("document.width = " + document.width);
> -            log("document.height = " + document.height);

Can you change this test to use document.body.clientWidth and document.body.clientHeight rather than remove it?

> LayoutTests/fullscreen/video-specified-size.html:-14
> -            waitForEventTestAndEnd(document, 'webkitfullscreenchange', "video.clientWidth==document.width");

Rather than removing this test, you can just change document.width on this line to document.body.clientWidth.
Comment 10 Vineet Chaudhary (vineetc) 2011-11-18 20:00:00 PST
Created attachment 115928 [details]
Updated patch as per review comments

Updated patch as per review comments.

Using document.body.clientWidth/document.body.clientHeight instead of document.width/document.height.
Comment 11 Adam Barth 2011-11-18 20:05:12 PST
Comment on attachment 115928 [details]
Updated patch as per review comments

View in context: https://bugs.webkit.org/attachment.cgi?id=115928&action=review

> LayoutTests/fast/dom/document-width-height-force-layout.html:15
> -            log("document.width = " + document.width);
> -            log("document.height = " + document.height);
> +            log("document.width = " + document.body.clientWidth);
> +            log("document.height = " + document.body.clientHeight);

I would have updated the left side of these log functions too.
Comment 12 Vineet Chaudhary (vineetc) 2011-11-18 20:14:49 PST
Created attachment 115929 [details]
updated_patch
Comment 13 WebKit Review Bot 2011-11-18 22:05:20 PST
Comment on attachment 115929 [details]
updated_patch

Clearing flags on attachment: 115929

Committed r100847: <http://trac.webkit.org/changeset/100847>
Comment 14 WebKit Review Bot 2011-11-18 22:05:25 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Yonathan Randolph 2011-12-12 16:04:52 PST
document.width (screen pixels) was actually not equivalent to document.body.clientWidth (CSS pixels). This was actually the quirk I used to calculate zoom level in WebKit [1]. I'm curious to know whether there is any other hack to get the zoom level in WebKit.

[1]: http://stackoverflow.com/questions/1713771/how-to-detect-page-zoom-level-in-all-modern-browsers
Comment 16 Adam Barth 2011-12-12 16:11:05 PST
It's unclear whether we want web sites to be able to detect the zoom level.  If we do, we can add an API for that rather than having folks write sites that rely on quirks.
Comment 17 Yonathan Randolph 2011-12-12 16:19:34 PST
It can be quite useful to measure zoom level e.g. to download an image or make a canvas that is at device resolution. I agree that it shouldn't require hacks though.
Comment 18 Brian Harper 2012-02-14 09:45:36 PST
This "fix" will negatively impact our user base, at least those using Safari and Chrome. As Yonathan mentioned, it can be useful to detect zoom, and this change will leave Webkit browsers as the only ones without a method for doing so. It's true that the methods differ between browsers, and it would be preferable to have a standard API for getting this information, but unfortunately there seems to be a philosophical debate among open source browser developers over whether web developers can be trusted to use this information responsibly. Since it's not a security hole of any kind, I'd like to think we should be given the benefit of the doubt and treated as adults.

Judging from the comments, this bug seemed to be based on the incorrect assumption that document.width returned the same value as document.body.clientWidth, and was therefore completely extraneous. Now that you've learned otherwise, I hope you will reconsider, and restore this property, at least until you've more carefully considered the issue of exposing device pixels and/or zoom detection.
Comment 19 Yonathan Randolph 2012-02-14 10:57:26 PST
Brian, you can still use -webkit-text-size-adjust:none to determine zoom level for Webkit. I'm curious whether you know of any trick to get zoom level in Opera; in newer Opera, I'm not aware of any measurement that is in device pixels anymore. btw I put my code at https://github.com/yonran/detect-zoom
Comment 20 mitz 2012-03-06 22:41:24 PST
This change broke a Mac app that uses embedded web content for its UI.
Comment 21 Adam Barth 2012-03-06 22:52:44 PST
Should we add an app-specific quirk?  It should be easy to stub in the API using a script a la http://trac.webkit.org/browser/trunk/Source/WebKit/mac/Misc/MailQuirksUserScript.js
Comment 22 mitz 2012-03-06 23:10:14 PST
(In reply to comment #21)
> Should we add an app-specific quirk?

Doing so would fix this one app of which I happen to know, but not any other apps and websites that may have been broken by this change.

I think the best and safest solution is to revert this change. If there is a strong argument against this (reading comments 0 through 19 in this bug, I failed to see what justified the change), then a linked-on-or-before check would be acceptable.
Comment 23 Adam Barth 2012-03-06 23:20:43 PST
Is there any evidence that this change broke any web sites?  The comments on this bug thus far seem to all be about using this API as a hack to detect the zoom level.
Comment 24 mitz 2012-03-06 23:22:54 PST
(In reply to comment #23)
> Is there any evidence that this change broke any web sites?

Not that I know of.
Comment 25 mitz 2012-03-06 23:23:18 PST
Filed bug 80494.