Opened 11 years ago

Closed 10 years ago

Last modified 8 years ago

#7028 closed enhancement (fixed)

[core] promote dijit/_base functionality to dojo.html (getViewport, sniff, scrollIntoView)

Reported by: dante Owned by: bill
Priority: high Milestone: 1.5
Component: General Version: 1.1.1
Keywords: Cc: alex, dylan
Blocked By: Blocking:

Description (last modified by bill)

There are some really great useful methods in dijit/_base, which have great use outside of Dijit.

In particular:

  • place.js - to get viewport calculation code
  • sniff.js - adds browser specific tags to html element for css sniffing
  • scroll.js - controls scrolling and other offsets (scrollIntoView, etc)

Proposing to move them into dojo.html as public modules, and deprecating the dijit._base components until 2.0 ...

Everything should be transparent via aliasing and whatnot, and we'll get everything internally using dijit/_base references so no deprecation warnings are shown.

Holding on this change to 1.3 to allow ample testing time.

Attachments (1)

connectTrunk.html (1003 bytes) - added by bill 10 years ago.
test connect to onscroll event on iframe's window object. put in dojo/tests, and run on IE6.

Download all attachments as: .zip

Change History (36)

comment:1 Changed 11 years ago by dante

Description: modified (diff)

comment:2 Changed 11 years ago by bill

Description: modified (diff)

Mark Hays is going to check in dijit/robot.js (part of the robot UI testing), which references dijit.scrollIntoView(). After the promotions above are done, I think some/all of dijit/robot.js will be moveable to dojo/robot.js, so please do that if possible.

comment:3 Changed 11 years ago by James Burke

The ticket #7262 points out that using dojo.html for the viewport functions may be overloading the dojo.html module. I tend to agree, perhaps a dojo.window module to house those functions related to viewports and windows? Or dojo.viewport?

comment:4 Changed 11 years ago by bill

See also #7449 for code to set viewport size.

comment:5 Changed 11 years ago by dante

Milestone: 1.3future
Priority: normalhigh
Status: newassigned

punting until the doc situation is sorted out. Need to ensure this is covered in book and regression testing done. It is a bigger change than it seems on the surface.

comment:6 Changed 11 years ago by bill

Scroll and viewport already have automated tests, and sniff has some minimal tests too.

Unclear if you wanted to move all of place.js or just getViewport() but I'm planning to add tests for place() soon.

Anyway, this shouldn't be marked as milestone future yet priority high.

comment:7 Changed 11 years ago by dante

Priority: highnormal

comment:8 Changed 10 years ago by bill

Summary: [core] promote dijit/_base functionality to dojo.html[core] promote dijit/_base functionality to dojo.html (getViewport, sniff, scrollIntoView)

Note also that dojo.dnd has a getViewport() method. It can be removed when this is fixed.

comment:8 Changed 10 years ago by bill

Note also that dojo.dnd has a getViewport() method. It can be removed when this is fixed.

comment:10 Changed 10 years ago by bill

(In [21274]) Removing with() statements so that this code can be optimized by Closure, and so others can understand it more easily (it took me a long time to figure out which attributes belonged to which objects). Refs #7028 !strict.

comment:11 Changed 10 years ago by bill

Milestone: future1.5
Owner: changed from dante to bill
Status: assignednew

comment:12 Changed 10 years ago by bill

(In [21360]) Move dijit sniff code to core, as dojo.uacss. As before, doing a dojo.require("dojo.uacss") will add the relevant classes to <html>.

The only change is to calculate all the classes in a local variable first, then only modify html.className once, to avoid possible extraneous reflows.

Refs #7028 !strict.

comment:13 Changed 10 years ago by Adam Peller

uacss violates our style guidelines... or at least Owen's guidelines which we've been trying to follow. How about dojo.sniff?

comment:14 Changed 10 years ago by James Burke

The name was my idea. I am open to a different name, but dojo.sniff sounded too much like it was doing the browser sniffing, but it is just using some UA-derived values to set some css classes. I was concerned about the double-acronym, ua and css together, but rationalized it as a new acronym.

What do you think will work better?

comment:15 Changed 10 years ago by James Burke

I promised Bill a code review, here it is:

I prefer this API for dojo.window:

dojo.window.getDocumentWindow(doc) -> dojo.window.get(doc)

dojo.window.scrollIntoView(node, pos) stays the same.

dojo.window.getViewPort() -> dojo.window.getBox() The goal is to try to match dojo.marginBox/dojo.contentBox, since those methods also return t, l, w, h. However, since it is not also a setter, (could it be?) then stick with getBox() instead of box()

The isIE, isWebKit, isOpera usage is unfortunate, we should try to remove what we can. It is worth looking at the featureDetection branch to see if we can leverage some of it. Some options:

For dojo.window.getDocumentWindow(): http://bugs.dojotoolkit.org/browser/branches/featureDetection/dijit/_base/window.js

It uses a dojo.isHostObjectProperty method: http://bugs.dojotoolkit.org/browser/branches/featureDetection/dojo/dojo.js#L254

I am OK porting over dojo.isHostObject() and dojo.isHostObjectProperty() if that is useful. Or just seeing what path IE6 takes in that code and see if we just need that part. The featureDetection version has support for things like Safari 2 and IceBrowser? that we do not care about, but if it comes for free, so be it. But seems like we should be able to remove the isIE usage in getDocumentWindow().

For dojo.window.scrollIntoView, that one looks trickier: http://bugs.dojotoolkit.org/browser/branches/featureDetection/dijit/_base/scroll.js

That one uses a dojo.isReallyIE8 test and it is quite long.

At the very least, maybe get rid of the isMoz, isWebKit and isOpera settings. Seems like we should just delegate to node.scrollIntoView(false) where we can, and some of that UA hoops are just to figure out if we can use that call. In the featureDetection branch, it uses the dojo.isHostMethod() to figure out if it is a method we can call and just uses it where possible. I favor at least that change, and we may be able to just use a simpler check, if(node.scrollIntoView).

As a side note, now that the method is part of Core, Opera support will be more important.

Also, I did not suggest the correct thing to do for this ticket, Bill asked me if I wanted a patch to review, but I said to just commit it in and we can iterate in there, but I believe the desired process is to make sure there is documentation in place before any commit. I forgot about that, I apologize. I have been in the DVCS world for a while and just plain did not think enough about the conventions we have in Dojo. Whatever I can do to help move this along to completion quickly to make up for my mistake, please let me know.

comment:16 Changed 10 years ago by bill

(In [21377]) Some name changes:

  • dojo.window.getDocumentWindow() --> dojo.window.get()
  • dojo.window.getViewport() --> dojo.window.getBox()

Refs #7028 !strict.

comment:17 Changed 10 years ago by bill

(In [21378]) Now that the needed methods are available in core, move DOH robot's _position() and _scrollIntoView() methods from dijit/robot.js to dojo/robot.js. Refs #7028 !strict.

comment:18 Changed 10 years ago by davidmark

Doesn't make a whole hell of a lot of sense for you to try to interpret the code in the feature detection branch. What happened to not wanting to hear about it?

Guessing isn't programming.

comment:19 Changed 10 years ago by Adam Peller

Cc: dylan added

David, that is inappropriate language for trac or for any part of this project. Do it again, and your access will be revoked.

comment:20 in reply to:  18 ; Changed 10 years ago by James Burke

Replying to davidmark:

As I have said before, I respect your knowledge of browser capabilities, and considering specific capability-based changes from the featureDetection branch is worth a look if we are working in those areas in trunk.

However, taking the featureDetection branch wholesale is not something I want to do. How you went about the changes and how you communicated with the team made that incredibly difficult to do, and you made some different higher-level tradeoffs than what we would have made.

Looking at the featureDetection branch for specific function/capability changes is easier to do, and ideally I would not want all the time you previously put into Dojo to go to waste.

comment:21 Changed 10 years ago by bill

Cc: bill removed

Reply to James' original comment from 2 weeks ago:

dojo.window.getDocumentWindow(doc) -> dojo.window.get(doc) dojo.window.getViewPort() -> dojo.window.getBox()

I've updated the names in [21377].

I've also added documentation to the reference doc and the 1.5 release notes.

At the very least, maybe get rid of the isMoz, isWebKit and isOpera settings. Seems like we should just delegate to node.scrollIntoView(false) where we can

node.scrollIntoView() fails in various cases which is why we provided the dijit.scrollIntoView() function. Not sure how bad the failures are but I know the unit tests will fail without those browser checks. The unit tests go over a lot of complicated cases, like iframes in quirks mode and with dir=RTL, etc.

As a side note, now that the method is part of Core, Opera support will be more important.

Right, Doug got opera working before I moved over dijit.scrollIntoView().

As for the code in the featureDetection branch, I'll take a look.

comment:22 Changed 10 years ago by bill

Regarding the featureDetection/getDocumentWindow() code, disregarding the Iceweasel branch that code is basically:

return doc.parentWindow || doc.defaultView;

(It's got some cleverness to precompute whether to use "parentWindow" or "defaultView" but the essence is that it returns whichever is defined for the current browser.)

The problem with the above code is that it breaks apps that connect to onscroll (and maybe other events) on the iframe's window object. This problem is documented in trunk/ (along with the hack to make that connect work):

// In some IE versions (at least 6.0), document.parentWindow does not return a
// reference to the real window object (maybe a copy), so we must fix it as well
// We use IE specific execScript to attach the real window reference to
// document._parentWindow for later use

...

/*
In IE 6, only the variable "window" can be used to connect events (others
may be only copies).
*/

Actually the caching in document._parentWindow has since been disabled, but the point is that IE needs special code for connects to work. It dates back to [4531] and was apparently needed for PopupContainer.js which does a

dojo.event.connect(win, "onscroll", this, "onClick");

The current code source:dijit/trunk/_base/focus.js has that same line about onscroll, although commented out. So I'm not sure how important it is to support the connect to an iframe's window object.

In summary, this code fails with the simplified dojo.window.get():

var realWin = dojo.window.get(myIframe.document);
dojo.connect(realWin, "onscroll", function(){alert("IE onscroll hacked")});

// this connect will only work after above connect call
dojo.connect(myIframe.window, "onscroll", function(){ alert("frames[0].window"); });

However, this works even with a simplified dojo.window.get():

dojo.connect(myIframe.window.document.body, "onscroll", function(){ alert("scroll on body"); });

I'll attach the test case.

James, opinion?

Changed 10 years ago by bill

Attachment: connectTrunk.html added

test connect to onscroll event on iframe's window object. put in dojo/tests, and run on IE6.

comment:23 in reply to:  19 Changed 10 years ago by davidmark

Replying to peller:

David, that is inappropriate language for trac or for any part of this project. Do it again, and your access will be revoked.

So first you screw everything up, then you pitch a snitch when told that you screwed everything up. That would be just like you to self-destruct further by "revoking" my access.

comment:24 in reply to:  22 Changed 10 years ago by davidmark

Replying to bill:

Regarding the featureDetection/getDocumentWindow() code, disregarding the Iceweasel branch that code is basically:

return doc.parentWindow || doc.defaultView;

Wrong. The whole point is that some browsers will not be able to get the window object at all. You have to deal with that case. And you missed the check that determines if the defaultView property is actually a window object. There's nothing in any spec that says it must be a window object (quite the contrary). And there are browsers that do not reference the window object with that property. Granted, they are probably ones you don't "care" about. Cross-browser scripting is about abstractions, not observations. That's why this stuff keeps breaking and you keep dismissing perfectly good browsers rather than admitting that your code doesn't work cross-browser.

(It's got some cleverness to precompute whether to use "parentWindow" or "defaultView" but the essence is that it returns whichever is defined for the current browser.)

Not exactly. See above.

The problem with the above code is that it breaks apps that connect to onscroll (and maybe other events) on the iframe's window object.

First off, the name of the event is "scroll", not "onscroll".

This problem is documented in trunk/ (along with the hack to make that connect work):

By some definition of "documented" which I am unfamiliar with.

// In some IE versions (at least 6.0), document.parentWindow does not return a
// reference to the real window object (maybe a copy), so we must fix it as well
// We use IE specific execScript to attach the real window reference to
// document._parentWindow for later use

Yes I saw that. It has no technical meaning. What is it you are trying to say?

...

/* In IE 6, only the variable "window" can be used to connect events (others may be only copies). */ }}}

Same there. The variable "window?"

Actually the caching in document._parentWindow has since been disabled, but the point is that IE needs special code for connects to work.

Maybe it does and maybe it doesn't. Hard to say without a real explanation. I wouldn't be shocked if the bug is in Dojo and not IE.

It dates back to [4531] and was apparently needed for PopupContainer.js which does a

dojo.event.connect(win, "onscroll", this, "onClick");

I see where the event name confusion originates. And "onClick?!"

The current code source:dijit/trunk/_base/focus.js has that same line about onscroll, although commented out. So I'm not sure how important it is to support the connect to an iframe's window object.

It's not a matter of importance. It works or it doesn't. If it doesn't work, then you have two choices: fix it or document the shortcoming.

In summary, this code fails with the simplified dojo.window.get():

var realWin = dojo.window.get(myIframe.document);

That appears to be your problem right there. Why would you even attempt to do something like that? What is "realWin" supposed to mean?

dojo.connect(realWin, "onscroll", function(){alert("IE onscroll hacked")});

this connect will only work after above connect call dojo.connect(myIframe.window, "onscroll", function(){ alert("frames[0].window"); }); }}}

That makes no sense.

However, this works even with a simplified dojo.window.get():

dojo.connect(myIframe.window.document.body, "onscroll", function(){ alert("scroll on body"); });

That's the least sensible. Why would you attach a scroll listener to the body? Are you expecting quirks mode?

I'll attach the test case.

James, opinion?

You are all still guessing.

comment:25 in reply to:  20 Changed 10 years ago by davidmark

Replying to jburke:

Replying to davidmark:

As I have said before, I respect your knowledge of browser capabilities, and considering specific capability-based changes from the featureDetection branch is worth a look if we are working in those areas in trunk.

No, you whined endlessly and issued back-handed compliments (like the above) every time it was brought up (back when you still had time to make a difference). The train has left the station and this project is now irrelevant.

You are no longer allowed to use it. It was procured under false pretenses.

However, taking the featureDetection branch wholesale is not something I want to do. How you went about the changes and how you communicated with the team made that incredibly difficult to do, and you made some different higher-level tradeoffs than what we would have made.

What you would have made is likely close to what you have made (and are in the process of making worse): an unmaintainable, unsustainable mess.

Looking at the featureDetection branch for specific function/capability changes is easier to do, and ideally I would not want all the time you previously put into Dojo to go to waste.

That's the whole issue right there. You people wasted my time and I didn't have it to waste. Now you can't use it at all. I suggest you delete it so as to eliminate the temptation to use it.

comment:26 Changed 10 years ago by davidmark

If you people hadn't have screwed up this effort last fall, you'd already know the answer to this get window "problem", which isn't a problem at all. You are trying to smash two problems together and thereby confusing yourselves as to what is going wrong and why.

The window "get" method (get a new name for that) should not be used to find IFrame windows by way of their documents. What a loopy concept! Either of these will work (even in IE6) for that purpose, finding the appropriate IFrame window by ID/name:-

var iframe = window.framestestiframe?; var iframe = document.getElementById('testiframe').contentWindow;

IIRC, I had already abstracted that logic in another method, with a note that it should be made part of the base window module (sound familiar?) Now months later, you are in the dark as I am not around to guide you through the branch.

The window "get" method, which only exists to get the window associated with a document object should never be used to accomplish the above. Document that and be done with it. Why does IE (not just 6 BTW) disallow attaching events to whatever object is referenced by the parentWindow property? Who knows? Host objects can do whatever they want. You should just realize that this "round trip" method of getting an IFrame's window object (and they may well be new objects every time you reference these properties) is clearly not a good idea (something that should have been apparent before this issue manifested itself). Again, understand the abstractions, don't wait until issues can be observed as by then it is too late. ;)

comment:27 Changed 10 years ago by davidmark

Wiki broke the code examples. Doesn't like square brackets I take it. Either of these will work:-

var iframe = window.frames['testiframe'];
var iframe = document.getElementById('testiframe').document.parentWindow;

comment:28 Changed 10 years ago by davidmark

And, of course, I pasted the wrong two the second time. These two:-

var iframe = window.frames['testiframe'];
var iframe = document.getElementById('testiframe').contentWindow;

comment:29 Changed 10 years ago by davidmark

I hope it is clear why those last two are the appropriate way to find an IFrame window (and why the previous attempts led you down the wrong road with regard to what should have been a completely unrelated method).

In any event, you still can't use the code from the branch, pending a resolution of my beef with Dylan.

comment:30 in reply to:  21 Changed 10 years ago by davidmark

Replying to bill:

[...]

node.scrollIntoView() fails in various cases which is why we provided the dijit.scrollIntoView() function. Not sure how bad the failures are but I know the unit tests will fail without those browser checks.

They definitely fail with the browser checks. Anything that tries to determine features from a random (and optional) string of characters is obviously not a successful design. Test the problem, not the UA string. Designs based on (sound) abstractions keep, those based on current observations don't (as we've seen over and over).

The unit tests go over a lot of complicated cases, like iframes in quirks mode and with dir=RTL, etc.

Quirks mode is not any more complicated in an IFrame. And there is no reason that RTL quirks should lead to parsing the UA string. It's just a (bad) habit that you've got to break.

As a side note, now that the method is part of Core, Opera support will be more important.

The core doesn't support Opera in any sense of the word "support" that I use. For instance, queries fail lots of basic tests, even in 10 (though granted, they do better than in 9). There's your UA sniffs creeping. That's no good as half of this stuff is written on top of queries and the general public is unlikely to care about what browsers you profess to care about. What they will do is go elsewhere if their favorite sites suddenly break due to a Dojo upgrade.

Right, Doug got opera working before I moved over dijit.scrollIntoView().

And how did he get Opera working (assuming you mean he did something to the script to make it appear to work in some version of Opera?) With a UA sniff?

As for the code in the featureDetection branch, I'll take a look.

It will be hard to understand without a translator.

And before anyone starts bitching about long-winded responses in here:-

  1. You should have more than one forum on the Website.
  2. The developer forum(s) should not be moderated (nobody reads them anyway).

All of these problems because you were "embarrassed" (in front of whom?) by all of the wrong answers I pointed out. Well, just imagine if I had never pointed them out at all. Things would be even worse than they currently are.

comment:31 in reply to:  14 Changed 10 years ago by davidmark

Replying to jburke:

The name was my idea. I am open to a different name, but dojo.sniff sounded too much like it was doing the browser sniffing,

Are you kidding? It is doing browser sniffing. And as if there were not a ton of that in Dijit already. Fix the problems, don't try to hide them by renaming them.

but it is just using some UA-derived values to set some css classes.

And what do you call that? Anything "derived" from the UA string is madness (and doomed to fail). Haven't we seen this thing fail constantly since its inception? It's a constant race to "keep up" with the very latest browsers, yet the thing is still perpetually behind. This is because of browser sniffing and changes in semantics won't help that.

I was concerned about the double-acronym, ua and css together, but rationalized it as a new acronym.

The operative word is rationalized.

What do you think will work better?

Ignoring the UA string. As we've seen repeatedly, there's nothing there to hang your hat on.

comment:32 Changed 10 years ago by bill

(In [21657]) Since we no longer cache the value in _parentWindow (see Cougar's comment about avoid a memory), there's no reason to check if the value is check that variable.

Refs #7028 !strict.

comment:33 Changed 10 years ago by bill

Resolution: fixed
Status: newclosed

As per my comments above about connecting to a window's onscroll event, I don't think it's prudent to further simplify or try to remove the browser sniffing from dojo.window.get()... it might break some apps.

As for scrollIntoView(), it also seems to need sniffing as there's no practical way to do bug-detection (which is what that code does: worksaround bug's in browser's native scrollIntoView() functionality).

So, I'm gonna close this ticket, feel free to muck with the code though if you see some improvement you want to make.

comment:34 Changed 9 years ago by bill

(In [22985]) Remove duplicated getViewport() code. No longer needed now that getViewport() has been moved to dojo core, as dojo.window.getBox(). Refs #7028 !strict.

comment:35 Changed 8 years ago by bill

(In [25366]) Change remaining refs to deprecated dijit.scrollIntoView() to dojo.window.scrollIntoView(). Refs #7028 !strict.

Note: See TracTickets for help on using tickets.