Last Comment Bug 585877 - remove document.height / document.width
: remove document.height / document.width
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla6
Assigned To: :Ms2ger
:
Mentors:
Depends on: 694931 653233 653292 680301 692616 714577
Blocks: 53076 587629 634755 651651 651750 698876
  Show dependency treegraph
 
Reported: 2010-08-10 01:01 PDT by Anne (:annevk)
Modified: 2012-01-19 14:15 PST (History)
19 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (3.62 KB, patch)
2010-08-10 06:09 PDT, :Ms2ger
jonas: review+
jst: superreview+
sayrer: approval2.0-
Details | Diff | Review
Warning patch v1 (4.95 KB, patch)
2010-08-20 12:23 PDT, :Ms2ger
jonas: review+
jst: approval2.0-
Details | Diff | Review
Warning patch v2 (4.64 KB, patch)
2010-08-25 01:26 PDT, :Ms2ger
jst: approval2.0+
Details | Diff | Review
Warning patch (checked in) (4.64 KB, patch)
2010-08-26 01:52 PDT, :Ms2ger
no flags Details | Diff | Review

Description Anne (:annevk) 2010-08-10 01:01:38 PDT
It would be great if support for these proprietary features could be removed from Gecko.

Opera encountered one problem so far with not supporting document.width and since Internet Explorer does not have them either it may not be too late to not forever have to support this.
Comment 1 :Ms2ger 2010-08-10 06:09:38 PDT
Created attachment 464390 [details] [diff] [review]
Patch v1
Comment 2 Jonas Sicking (:sicking) 2010-08-10 08:12:20 PDT
Did you check that this passes tryserver?
Comment 3 :Ms2ger 2010-08-10 10:58:42 PDT
I don't have try access. Tests pass locally, though.

Does this need sr?
Comment 4 Jonas Sicking (:sicking) 2010-08-10 11:07:12 PDT
Comment on attachment 464390 [details] [diff] [review]
Patch v1

Wouldn't hurt
Comment 5 Dão Gottwald [:dao] 2010-08-12 02:14:41 PDT
approval2.0?
Comment 6 Dão Gottwald [:dao] 2010-08-15 02:46:49 PDT
http://hg.mozilla.org/mozilla-central/rev/43b490ef9dab
Comment 8 Robert Sayre 2010-08-17 09:52:01 PDT
This change broke graphs.mozilla.org. A cursory google code search shows that document.height is far from unused, though mostly on aging code paths. I don't see a good reason to take this for Firefox 4.

I'm going to back this out.
Comment 9 Mike Shaver (:shaver -- probably not reading bugmail closely) 2010-08-17 09:56:59 PDT
"Opera had no problems" isn't sufficient research on which basis to remove things, especially without doing a basic search for use.  Please do back it out, Sayre, and let's be a little more careful in the future?
Comment 10 Jonas Sicking (:sicking) 2010-08-17 11:07:42 PDT
Note that "IE has never supported them" was the stronger reason that we felt ok with this.

I'm fine with not taking this for firefox 4, however I don't think "graphserver uses it" is a very good reason for keeping a feature in the web platform. Will look more at google code results though.
Comment 11 Jonas Sicking (:sicking) 2010-08-17 11:10:42 PDT
Ms2ger: Would you mind writing up a patch that adds warnings to the console whenever these properties are used? Probably want to add a flag so that we only warn once per document or some such.
Comment 12 Robert Sayre 2010-08-17 11:11:30 PDT
This is not worth focusing on for Firefox 4, imho.
Comment 13 Mike Beltzner [:beltzner, not reading bugmail] 2010-08-17 12:15:58 PDT
Please back out of mozilla-central default and GECKO20b4_20100817_RELBRANCH
Comment 14 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-08-17 12:47:43 PDT
Backed out on default and the b4 relbranch:
https://hg.mozilla.org/mozilla-central/rev/9aa39b619a19
https://hg.mozilla.org/mozilla-central/rev/9f434423bdf9
Comment 15 Mike Beltzner [:beltzner, not reading bugmail] 2010-08-17 13:31:39 PDT
(removing blocking:beta4+ since the issue's no longer in that beta)
Comment 16 :Ehsan Akhgari (busy, don't ask for review please) 2010-08-17 13:38:43 PDT
The MDC changes also need to be reverted.
Comment 17 Brendan Eich [:brendan] 2010-08-17 18:15:47 PDT
Anne's wrong-footing Sicking! What next? Some Googler bamboozling a JS purist into removing __proto__? :-/

/be
Comment 18 Matt Evans [:mevans] 2010-08-17 22:41:55 PDT
The documentation edits need to backed out as well.
Comment 19 Anne (:annevk) 2010-08-18 01:44:34 PDT
Bredan, geez. Jonas expressed interest in removing legacy stuff on the public-html list so I found some and reported a bug with the information I had on Opera and IE. Did I know your internal systems rely on it...
Comment 20 Brendan Eich [:brendan] 2010-08-18 16:14:50 PDT
Anne, I kid (mostly :-/). But it is not just our internal systems that use this old stuff.

/be
Comment 21 Jonas Sicking (:sicking) 2010-08-18 17:28:19 PDT
Brendan: It almost is only us using it. Google code search turned up a lot of things that didn't use document_height or other similarly named things, a lot of stuff commented out, and a lot of stuff that falls back to using other properties, or that also depends on layers and thus is already broken.

Here are a few things that might be breaking:

http://nk-gesture.googlecode.com/svn (nkGestures.js)
http://accessext.googlecode.com/svn (treebox_class.js) Some gecko specific
                                    thing, also uses XUL
git://github.com/cauld/sideline.git (sideline.js)
http://konverta4.googlecode.com/svn (functions.js)
http://hyperic-hq.svn.sourceforge.net/svnroot/hyperic-hq (footer.js)
git://github.com/cauld/sideline.git (ti-sideline-synch-hack.js)
Comment 22 Robert Sayre 2010-08-18 18:23:13 PDT
(In reply to comment #21)
> 
> Here are a few things that might be breaking:

Removing these is not a terrible idea, but it will take time to research. The fact that one of our sites broke immediately is not a dealbreaker, but it is a bad sign.

Thinking more, I can't see how this change improves the Web very much if there's no risk in removing these properties. I'm pretty sure there are much more important things to focus on.
Comment 23 Olli Pettay [:smaug] 2010-08-19 02:26:08 PDT
Before removing properties like document.height/.width we could warn
in the error console about use of deprecated feature.
That approach has worked reasonable well when removing/no-op'ing some 
Netscapeisms from the event handling code.
Comment 24 :Ms2ger 2010-08-20 12:23:34 PDT
Created attachment 467844 [details] [diff] [review]
Warning patch v1

(This doesn't apply to trunk yet, due to Mounir's recent changes.)
Comment 25 j.j. 2010-08-21 05:52:02 PDT
> Created attachment 467844 [details] [diff] [review]
> Warning patch v1

Wouldn't it be better to recommend
 document.body.clientHeight/clientWidth ?

It has better backwards and browser compatibility than
 document.body.getBoundingClientRect()
Comment 26 Jonas Sicking (:sicking) 2010-08-23 18:25:52 PDT
Comment on attachment 467844 [details] [diff] [review]
Warning patch v1

Though I would also recommend clientWidth/clientHeight.
Comment 27 :Ms2ger 2010-08-24 04:39:50 PDT
Comment on attachment 467844 [details] [diff] [review]
Warning patch v1

Okay, will fix.
Comment 28 Johnny Stenback (:jst, jst@mozilla.com) 2010-08-24 16:52:38 PDT
Comment on attachment 467844 [details] [diff] [review]
Warning patch v1

Please re-request approval once a patch that's ready to land is available.
Comment 29 :Ms2ger 2010-08-25 01:26:37 PDT
Created attachment 468978 [details] [diff] [review]
Warning patch v2
Comment 30 :Ms2ger 2010-08-26 01:52:11 PDT
Created attachment 469385 [details] [diff] [review]
Warning patch (checked in)

Thanks.
Comment 31 Dão Gottwald [:dao] 2010-08-28 00:30:21 PDT
Comment on attachment 469385 [details] [diff] [review]
Warning patch (checked in)

http://hg.mozilla.org/mozilla-central/rev/dc76a10a7bdf
Comment 33 Mike Beltzner [:beltzner, not reading bugmail] 2011-02-23 10:47:55 PST
Does this really need landing? Looks like it landed. Should it be resolved FIXED?
Comment 34 :Ms2ger 2011-02-23 13:25:17 PST
Yes, the main patch here still needs to land, after we branch. It was backed out:

http://hg.mozilla.org/mozilla-central/rev/9aa39b619a19
Comment 35 :Ehsan Akhgari (busy, don't ask for review please) 2011-04-12 18:06:43 PDT
I was going to land this, but I wasn't sure whether the warning patch should be backed out or not (I think it should).
Comment 37 neil@parkwaycc.co.uk 2011-04-21 02:04:58 PDT
(In reply to comment #25)
> Wouldn't it be better to recommend
>  document.body.clientHeight/clientWidth ?
> 
> It has better backwards and browser compatibility than
>  document.body.getBoundingClientRect()

In the (admittedly unusual) case where the body element has a border, then clientHeight/clientWidth deliberately excludes that. (Padding is not excluded.) On the other hand, getBoundingClientRect.height/width returns a float...
Comment 38 Florian Scholz [:fscholz] (MDN) 2011-04-21 02:30:00 PDT
Updated documents mentioned in Comment 32

Added note to https://developer.mozilla.org/en/Firefox_6_for_developers
Comment 39 richard.huber 2012-01-19 11:45:02 PST
Why was this functionality removed? This feature is used in Sun / Oracle ILOM web management interfaces and removal has broken the ability to manage systems using Firefox 6 web browsers and above. Users are then forced to use other browsers.
Although Firefox 6 and above support has been fixed in newer ILOM firmware versions by Oracle on some systems, it still means that older historical systems will not work with Firefox 6+. Surely it would have been better to have an about:config setting to re-enable this functionality if needed?

Note You need to log in before you can comment on or make changes to this bug.