Bug 72591 – Remove document.width / document.height

• Component: DOM
• Status: RESOLVED FIXED

[Anne van Kesteren]

WebKit is the only vendor who remains to have support for this.

[Anne van Kesteren]

abarth says they're on HTMLDocument.

[Adam Barth]

[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

[Adam Barth]

Created attachment 115551 [details]
work in progress

[Vineet Chaudhary (vineetc)]

May be we need to remove below line from Source/WebCore/bindings/objc/PublicDOMInterfaces.h 

@property(readonly) int width;
@property(readonly) int height;

[WebKit Review Bot]

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

[Adam Barth]

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.

[Dominic Cooney]

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

[Vineet Chaudhary (vineetc)]

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.

[Adam Barth]

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.

[Vineet Chaudhary (vineetc)]

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.

[Adam Barth]

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.

[Vineet Chaudhary (vineetc)]

Created attachment 115929 [details]
updated_patch

[WebKit Review Bot]

Comment on attachment 115929 [details]
updated_patch

Clearing flags on attachment: 115929

Committed r100847: <http://trac.webkit.org/changeset/100847>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Yonathan Randolph]

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

[Adam Barth]

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.

[Yonathan Randolph]

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.

[Brian Harper]

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.

[Yonathan Randolph]

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

This change broke a Mac app that uses embedded web content for its UI.

[Adam Barth]

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

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

[Adam Barth]

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.

(In reply to comment #23)
> Is there any evidence that this change broke any web sites?

Not that I know of.

Filed bug 80494.