Bug 72591 - Remove document.width / document.height
: Remove document.width / document.height
Status: RESOLVED FIXED
: WebKit
HTML DOM
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2011-11-17 02:53 PST by
Modified: 2012-03-07 10:51 PST (History)


Attachments
work in progress (1.63 KB, patch)
2011-11-17 03:08 PST, Adam Barth
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
proposed patch (5.22 KB, patch)
2011-11-18 12:27 PST, Vineet Chaudhary (vineetc)
abarth: review-
Review Patch | Details | Formatted Diff | Diff
Updated patch as per review comments (5.69 KB, patch)
2011-11-18 20:00 PST, Vineet Chaudhary (vineetc)
abarth: review+
abarth: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
updated_patch (5.77 KB, patch)
2011-11-18 20:14 PST, Vineet Chaudhary (vineetc)
no flags 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 2011-11-17 02:53:29 PST
WebKit is the only vendor who remains to have support for this.
------- Comment #1 From 2011-11-17 03:05:21 PST -------
abarth says they're on HTMLDocument.
------- Comment #2 From 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 From 2011-11-17 03:08:50 PST -------
Created an attachment (id=115551) [details]
work in progress
------- Comment #4 From 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 From 2011-11-17 05:19:01 PST -------
(From update of attachment 115551 [details])
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 From 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 From 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 From 2011-11-18 12:27:33 PST -------
Created an attachment (id=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 From 2011-11-18 15:10:44 PST -------
(From update of attachment 115847 [details])
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 From 2011-11-18 20:00:00 PST -------
Created an attachment (id=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 From 2011-11-18 20:05:12 PST -------
(From update of attachment 115928 [details])
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 From 2011-11-18 20:14:49 PST -------
Created an attachment (id=115929) [details]
updated_patch
------- Comment #13 From 2011-11-18 22:05:20 PST -------
(From update of attachment 115929 [details])
Clearing flags on attachment: 115929

Committed r100847: <http://trac.webkit.org/changeset/100847>
------- Comment #14 From 2011-11-18 22:05:25 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #15 From 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 From 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 From 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 From 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 From 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 From 2012-03-06 22:41:24 PST -------
This change broke a Mac app that uses embedded web content for its UI.
------- Comment #21 From 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 From 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 From 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 From 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 From 2012-03-06 23:23:18 PST -------
Filed bug 80494.