Summary: | Remove document.width / document.height | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Anne van Kesteren <annevk> | ||||||||||
Component: | DOM | Assignee: | 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
Anne van Kesteren
2011-11-17 02:53:29 PST
abarth says they're on HTMLDocument. [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 Created attachment 115551 [details]
work in progress
May be we need to remove below line from Source/WebCore/bindings/objc/PublicDOMInterfaces.h @property(readonly) int width; @property(readonly) int height; 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 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. (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. 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 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. 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 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. Created attachment 115929 [details]
updated_patch
Comment on attachment 115929 [details] updated_patch Clearing flags on attachment: 115929 Committed r100847: <http://trac.webkit.org/changeset/100847> All reviewed patches have been landed. Closing bug. 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 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. 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. 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. 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. 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. 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. |