Split dojo/_base/html.js

[Eugene Lazutkin]

Split into two parts:

• node geometry (coords, marginBox, contentBox)
• the rest (attr, style, class)

By splitting it we can separate the unrelated functionality, simplify our lives (e.g., easier to track what exactly was changed), and provide a way for custom builds that do not want to include one of them.

buildUtil.baseMappings in util/buildscripts/buildUtil.js should be updated to reflect the change.

[Eugene Lazutkin]

Possible names suggested during the RFC process:

Author	1st file	2nd file
Bill Keese	geometry.js	html.js
Bill Keese	geometry.js	style.js
James Burke	box.js	html.js
Mike Wilcox	dom.js	style.js

Nobody proposed to split into more than two files.

[Eugene Lazutkin]

(In [25225]) base: split html.js into several manageable files, !strict, refs #9641.

[Bryan Forbes]

Currently, there are a few references to win.doc() in dom-geometry.js. I'm attaching a patch to fix that.

[Rawld Gill]

(In [25239]) fixed dependency vector and other typos (that caused failure in IE); refs #9641

[Eugene Lazutkin]

Note: Rawld took care of Bryan's patch in [25239].

[Eugene Lazutkin]

(In [25242]) html: added a reference to a sniff package for consistency; need to move it later to has, !strict, refs #9641.

[bill]

(In [25243]) spacing fixes, missing semicolons, removed unused variables, fixed a few comments, refs #9641 !strict

[bill]

I checked over the changes. They look good. A few things I noticed:

• I'm getting a failure in html_rtl test (scrollingQuirksIframeVertSmall), not sure if it was there before or not. This is on FF4/win.
• you removed the node = byId(node) from replaceClass(), won't that break when user passes in an id?
• in dojo._fixIeBiDiScrollLeft() you removed a checked for IE6, not sure why or if the code still works in that case (it seems suspicious though)
• in dojo.attr() that connect()/disconnect() calls should be replaced by calls to on(), but that's outside the scope of your changes

[Eugene Lazutkin]

Replying to bill:

I checked over the changes. They look good. A few things I noticed:

• I'm getting a failure in html_rtl test (scrollingQuirksIframeVertSmall), not sure if it was there before or not. This is on FF4/win.

I saw it too on FF4/linux today, but not on Chrome. I actually tried to debug it, and the test is quite convoluted with two embedded timers --- I couldn't figure out what it supposed to do, so I have no idea if it is a bug in our scroller code, a bug in the test, or a bug in FF4.

• you removed the node = byId(node) from replaceClass(), won't that break when user passes in an id?

That's my omission of the recent change. Somehow I missed it when I was merging. Will fix.

• in dojo._fixIeBiDiScrollLeft() you removed a checked for IE6, not sure why or if the code still works in that case (it seems suspicious though)

That was not my intention. Let me re-check and put the missing piece back.

• in dojo.attr() that connect()/disconnect() calls should be replaced by calls to on(), but that's outside the scope of your changes

That was intentional. My first goal was to make the split work, and there are some things I can do to improve the code:

• Refactor module return values (right now dojo is returned).
• Use has() instead of sniffing if possible. In any case the sniff module duplicates all its variable as has checks, so we should use has.
• Update other core modules to reference new modules directly, so we can build code automatically with minimal dependencies.

[Eugene Lazutkin]

Patch to convert dom-class functions to local, with explicit export, and short names.

[Eugene Lazutkin]

(In [25250]) html: added the mising byId() conversion to replaceClass(), minor reformatting, !strict, refs #9641.

[Eugene Lazutkin]

(In [25251]) html: added missing improvement to _fixIeBiDiScrollLeft(), !strict, refs #9641.

[Bryan Forbes]

Patch to convert dom-class functions to override-able module functions with short names. Added wrappers in dojo/_base/html.js.

[bill]

(In [25253]) dojo._getBorderBox() was removed in html.js refactor. Use dojo.position() instead.

Also, replace unnecessary use of private dojo._getMarginSize() with dojo.position().

Refs #9641, #11622, #13107 !strict.

[bill]

(In [25254]) Replace unnecessary use of private dojo._getMarginSize() with dojo.position(). Refs #9641, #11622, #13107 !strict.

[bill]

(In [25271]) "global" is defined on dojo object, not as part of hash which [will be] returned by dojo/_base/window. refs #9641 !strict.

[bill]

(In [25272]) nevermind, I can synchronize "global" on dojo object with a "global" property in the hash which [will be] returned by dojo/_base/window. refs #9641 !strict.

[bill]

(In [25276]) forEach() is in array, not lang, refs #9641 !strict. (I did notice the comment to stop using forEach altogether, but I'll leave that to Eugene.)

[Eugene Lazutkin]

(In [25279]) html: eliminated forEach(), !strict, refs #9641.

[bill]

(In [25621]) Fix usages of removed functions dojo._getBorderBox() and dojo._setOpacity(), refs #9641 !strict.

[Eugene Lazutkin]

(In [25704]) html: moved all dom-* into dom/, added classList-based xxxClass functions, added more public API to dom/geometry, !strict, refs #9641.

[Eugene Lazutkin]

#13104 is closed by the previous commit.

[Eugene Lazutkin]

#13105 is closed by the previous commit.

[Eugene Lazutkin]

#13106 is closed by the previous commit.

[Eugene Lazutkin]

#13107 is closed by the previous commit.

[Eugene Lazutkin]

#9747 is closed by the previous commit.

[Eugene Lazutkin]

#11465 is closed by the previous commit.

[bill]

(In [25716]) fix breakage from [25704], refs #9641

[bill]

(In [25717]) fix breakage from [25704], refs #9641, #11465

[bill]

(In [25718]) fixing another breakage from [25704], refs #9641

[Eugene Lazutkin]

(In [25720]) html: fixed typos related to margin box calculations (inspected the node itself, instead of its parent), !strict, refs #9641.

[bill]

(In [25721]) Fix more breakage from [25704]. In 1.6 dojo.marginBox() etc. as a setter doesn't return a value. After [25704] it returns the node. This breaks dijit's layoutChildren() method which mixes in the return value to the widget (in the private resize() method).

It would perhaps be better if dojo.marginBox() etc. worked according to it's spec, which says: Returns an object in the expected format of box (regardless if box is passed).

Refs #9641 !strict.

[Eugene Lazutkin]

(In [25722]) html: typo fixes in class.js, better work with edge cases, !strict, refs #9641.

[Eugene Lazutkin]

(In [25723]) html: swapped class implementations, !strict, refs #9641.

[Eugene Lazutkin]

Replying to bill:

It would perhaps be better if dojo.marginBox() etc. worked according to it's spec, which says: Returns an object in the expected format of box (regardless if box is passed).

The only sure proof way to do that is to call dojo.getMarginBox() right after dojo.setMarginBox(). I suspect that it'll be very slow, because the latter triggers re-rendering, and the former will wait for it to finish. In many cases you don't want to wait, so it'll add an unnecessary delay.

The same goes for dojo.contentBox().

I suggest to change the doc rather than the other way around.

[Eugene Lazutkin]

Robot has missed [25745]: html: moved all dom/* modules to dom-*, removed classList path in dom-class, removed all public aliases in dom-geometry, !strict, refs #9641.

Effectively it reverts [25704] and subsequent fixes/refinements.

[Eugene Lazutkin]

Reverted back previously closed tickets: #13104, #13105, #13106, #13107, #9747, #11465. The last two were marked as wontfix as per the decision made during the last meeting.

[bill]

(In [25746]) dom/construct has been moved back to dom-construct; refs #9641 !strict. ps: not sure how to test this file.

[Eugene Lazutkin]

It looks like robot missed [25901]: dom: all DOM modules are made "baseless", and html.js was corrected to provide the same API as before, expect all direct users of dom modules to be broken, !strict, refs #9641.

[ben hockey]

Replying to elazutkin:

It looks like robot missed [25901]: dom: all DOM modules are made "baseless", and html.js was corrected to provide the same API as before, expect all direct users of dom modules to be broken, !strict, refs #9641.

in ie, this definition of has (​http://bugs.dojotoolkit.org/browser/dojo/dojo/trunk/dom-attr.js?rev=25901#L176) shadows this definition of has (​http://bugs.dojotoolkit.org/browser/dojo/dojo/trunk/dom-attr.js?rev=25901#L2) which causes this line to fail ​http://bugs.dojotoolkit.org/browser/dojo/dojo/trunk/dom-attr.js?rev=25901#L152 since its calling the has on L176.

[ben hockey]

a small case to demo what i'm describing in the above comment:

(function (has) {

has('abc');

var attr = {
    has : function has() {
        console.log('inner');
    }
};

})(function () { console.log('outer'); });

[Bryan Forbes]

Not only will that definition of has mask the other, this module is using named function expressions which cause leaks in IE (​http://kangax.github.com/nfe/). We either need to null the leaked references, use function declarations and then assign those functions to the object, or use plain function expressions with no names (I vote for using function declarations).

[Eugene Lazutkin]

Missed commit [25911]: dom: fixed some inline docs, renamed a few functions to avoid name clashes, thx neonstalwart!, !strict, refs #9641.

[Eugene Lazutkin]

Replying to BryanForbes:

Not only will that definition of has mask the other, this module is using named function expressions which cause leaks in IE (​http://kangax.github.com/nfe/). We either need to null the leaked references, use function declarations and then assign those functions to the object, or use plain function expressions with no names (I vote for using function declarations).

Could you provide more information?

var f = function g(){};

AFAIK, memory leaks on IE when both names are used (f and g) anywhere. In this case g is never used.

Another thing is in order to leak a memory in any way you have to create an opportunity for it to be reclaimed. Irreclaimable memory with no external references is called a memory leak. In the case of modules in order to create a leak we have to either load the same module repeatedly losing references to old instances, or unload them in any other way. AFAIK, there are no provisions for that => all modules and their functions are in memory since the time they were loaded irrespective of how many references they have.

What did I miss?

[bill]

From my reading of the article, this won't cause any memory leaks on IE, although it might be different if you had different function definitions depending on the browser, like:

marginBox: dojo.isIE ? function ieMarginBox(){ ... }  :
          function otherMarginBox(){ ... };

But the code doesn't have that, AFAICT.

However, adding the function names does bloat the size of the files. FWIW, I updated my ​spreadsheet, and the HTML code (after shrinksafe) is currently running at 20.9K, compared to 14.5K in 1.6. Of course that's due to much more than the unnecessary function names.

[Eugene Lazutkin]

Replying to bill:

However, adding the function names does bloat the size of the files.

My hope was that gzip will take care of extra names.

The reason for naming functions was to help with debugging this stuff. Apparently it helps to generate correct stack traces, and aids in manual debugging as well by providing a name on a function object. Not on IE though.

[bill]

Sure, I understand the debugging value, although I think the loader/builder might have an option to insert function names?

Anyway, here's the size w/function names after gzip:

mac:dojo bill$ wc dom*gz
       4      31     885 dom-attr.js.gz
       1      32     884 dom-class.js.gz
       7      46    1394 dom-construct.js.gz
       3      29     880 dom-form.js.gz
       8      58    2331 dom-geometry.js.gz
       4      22     846 dom-prop.js.gz
       9      50    1351 dom-style.js.gz
       0      22     772 dom.js.gz
       4      27     775 domReady.js.gz
      40     317   10118 total

And without function names:

mac:dojo bill$ wc dom*gz
       1      29     866 dom-attr.js.gz
       2      27     860 dom-class.js.gz
       7      46    1391 dom-construct.js.gz
       3      27     867 dom-form.js.gz
       9      73    2285 dom-geometry.js.gz
       2      32     836 dom-prop.js.gz
       7      35    1344 dom-style.js.gz
       0      23     772 dom.js.gz
       4      27     775 domReady.js.gz
      35     319    9996 total

I guess the difference is pretty minor.

[Bryan Forbes]

A named function expression has the following pattern (as Eugene pointed out):

var f = function foo(){};

I don't want to rewrite the whole article here, but here's a good explanation of what should happen:

var f = function foo(){
    return typeof foo; // "foo" is available in this inner scope
};
// `foo` is never visible "outside"
typeof foo; // "undefined"
f(); // "function"

And what happens in IE:

var f = function foo(){
    return typeof foo; // "foo" is available in this inner scope
};
// `foo` is visible "outside"
typeof foo; // "function"
f(); // "function"

Not only does the identifier "leak" into the enclosing scope, but two distinct functions are actually created here:

var f = function foo(){};
f === foo; // false in IE

The two functions also reference each other by closure which is a circular reference and we all know how well IE6 and IE7 deal with circular references through a closure (they don't). Explicitly declaring var foo and nulling it solves the problem and allows IE to reclaim the memory:

var f = (function(){
  var f = function foo(){};
  var foo = null;
  return f;
})();

This doesn't happen just when doing an if statement or using a ?:, this happens whenever you use a named function expression in IE and don't clean up after it. This also happens when you load a module once (no repeated loading needed). And I'd like to point out that yes, all modules and their functions are in memory, but if IE is creating extra functions that have no way to be referenced and can't be collected until a page refresh, we are leaking in single page apps.

[bill]

See also #13352 as an alternate way to get those symbols for debugging. Only for built code though, so maybe not so useful.

[Eugene Lazutkin]

In [26158]:

dom-geometry: converting set* functions to accept a hash of values compatible with get* functions, thx Bill and Adam!, !strict, refs #9641.

[zhangyp]

check in 26158 breaks preview.php in util/docscripts. Please have a check whether we need to fix a regression or fire another ticket to fix preview.php.

[zhangyp]

BTW, I am using preview.php on chrome and firefox of Linux.

[Eugene Lazutkin]

In [26161]:

dom-geometry: removed private aliases for set* functions, thx Bill for suggestion!, !strict, refs #9641.

[bill]

In [26166]:

Revert dijit to using hash for setMarginBox() and setContentSize(), same as in 1.6. Actually pulled those chunks of code from before the baseless refactor. Refs #9641 !strict.

[bill]

In [26174]:

Final setMarginBox() call. Refs #9641 !strict.

[Eugene Lazutkin]

In [26179]:

html: providing function wrappers for destroy/empty functions to facilitate one-point AOP, thx Bill!, !strict, refs #9641.

[Eugene Lazutkin]

In [26180]:

dom-attr: removed reference to dojo/kernel --- not needed anymore, thx Bill!, !strict, refs #9641.

[Eugene Lazutkin]

In [26181]:

dom-construct: converted external references to internal ones (missed typos), thx Bill!, !strict, refs #9641.

[Eugene Lazutkin]

In [26182]:

dom-prop: needs a dom-construct reference, but in doing so creates a loop, leaving it like that for now, !strict, refs #9641.

[bill]

In [26185]:

Since app code can call domGeometry.empty() etc. directly, need to connect there instead of on dojo.empty(). Fixes Dialog_mouse.html test failures. Refs #9641, fixes #13625 !strict. See also [26107] which enables a single on() call to monitor both dojo.empty() and domConstruct.empty() calls.

[Eugene Lazutkin]

In [26188]:

dom: fixed the missing rename (dojo=>dom) and removed a reference to dojo/kernel, thx Akira Sudoh!, !strict, refs #9641.

[bill]

In [26258]:

Eugene's patch (based on Rawld's advice) to resolve circular dependency problem by using "exports" variable, refs #9641 !strict

[bill]

This ticket can be closed, right? I think (and hope) it's done.

[bill]

In [30353]:

Stop using deprecated dojo/_base/html and dojo/ready in parser and parser tests. Also use doh.is() rather than doh.t() for equality comparison. Refs #9641 !strict.

[bill]

In [30399]:

remove obsolete comment (dom-prop now explicitly requires dom-construct), refs #9641 !strict.