Closed Bug 147058 Opened 23 years ago Closed 21 years ago

Identifier resolution walks up DOM

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: ian, Unassigned)

References

(
URL
)

Details

(Keywords: testcase)

Attachments

(4 files, 2 obsolete files)

Modified Hixie's DOM Scoping test
767 bytes, text/html
Details
This should get it right.
2.39 KB, patch
Details | Diff | Splinter Review
Change the scope chain only for HTML elements.
2.89 KB, patch
Details | Diff | Splinter Review
Change the scope chain for everything other than for XUL/XBL.
3.17 KB, patch
caillon
: review+
peterv
: superreview+
Details | Diff | Splinter Review
In an event handler, the current scope for identifiers in Mozilla is the current element (|this|), it's parent element, and so on, up to the window object, then the global scope. (Or something like that.) For example: <a hreflang="hreflang of <a> element"> <img src="about:blank" alt="Click to test." id="test" class="class of <img> element" onclick="alert('className = ' + className + '\nhreflang = ' + hreflang + '\ndocumentElement = ' + documentElement));"> </a> ...alerts: className = class of <img> element hreflang = hreflang of <a> element documentElement = [object HTMLHtmlElement] However, if you try it in IE, it fails, saying that hreflang is undefined. Now I don't know which is right, but our behaviour means that <script type="text/javascript"> function TestClass() { this.say = function() { alert('test'); } } var test = new TestClass(); </script> <img src="test" alt="click here" id="test" onclick="test.say()"> ...fails, because test resolves to document.test, the image, and not the object declared in the script block.
I couldn't reproduce this using an onload handler on a <body> element, which may be relevant, but here is a test of this using an onclick handler on an <img> element: http://www.hixie.ch/tests/adhoc/js/dom-binding/001.html IE "passes", we "fail".
URL: http://www.hixie.ch/tests/adhoc/js/do...
Keywords: testcase
As far as I can remember we have sent to envagelism all the bug reports caused by this resolution scheme except one (document.open() with three parameters maps to window.open()). It's also in my DOM release notes if anyone cares ;-)
What backing is there in the specs for this resolution scheme?
A normative and compleat DOM JS binding spec? HAHAHAHAHA! Er, sorry. I blame the w3c. /be
If the specs leave this undefined, then what reason do we have for breaking compatability with IE? Have we brought this issue up to the W3C? Is it something the DOM JS binding should be explicit about?
*** Bug 148725 has been marked as a duplicate of this bug. ***
*** Bug 188951 has been marked as a duplicate of this bug. ***
Mass-reassigning bugs to dom_bugs@netscape.com
Assignee: jst → dom_bugs
brendan? jst? Any response to comment 5? Is this documented anywhere? In the Web Forms spec I am saying: | The scope chain for ECMAScript executed in HTML event handler attributes links | from the activation object for the handler, to its this parameter (the event | target), to the form, to the document, to the default view (the window). ...but that doesn't seem to explain the behaviour seen in the testcase above.
QA Contact: desale → ian
There's no precedent for making <a> add to the scope chain. That does seem like a bug -- it's not compatible with Nav4 or IE -- or anything else (what do Opera and Safari do?). /be
Opera does what IE does. Can't test Safari from here.
I don't know that this is defined anywhere, nor do I know the reason for things being the way they are in Mozilla, they've just always been that way, and until now, that has seemd like the right way to do it. I think changing this might fix a site or two out there, but I bet it will break a bunch as well. Which way we'll lean after a change like this I don't know off the top of my head. Others way in here if you've got thoughts or comments.
I don't know how many sites rely on our behavior but I was surprised by it when I first ran into it. Note that this resolution by walking up the DOM only happens if you have the event handler defined inline rather than defined as a function which is called. See the attached modification of Hixie's test. I think this is confusing
Since Mozilla is the only UA (that I know of) that works in this way, I'd be reasonably sure that fixing this would fix more sites than it would break. If we don't fix this, though, I'd like to at least have an accurate description of what we do.
Ian, Mozilla makes the scope chain of DOM nodes follow the parent chain in the DOM, and the scope (this object) of an event handler is the object on which the event handler is registerd. It's that easy, and sounds fairly reasonable to me, but if no other UA's do the same I guess we're better off following what others do. I guess we could make the scope of event handlers be the global scope or something, but I bet that will break most of our event handlers in xul/xbl. And if we don't start running event handlers in the global scope, then we'd need to change the scope chain on our DOM nodes, and that would change object lifetime since then the JS object graph of our wrapped DOM nodes change, and who knows what consequences that might have. All fixable, but non-trivial.
It's not as simple as you describe. I've seen testcases where that doesn't happen, and there are other cases where we jump up specific chains (e.g. form control events). For example: http://www.hixie.ch/tests/adhoc/js/dom-binding/002.html (form control test) http://www.hixie.ch/tests/adhoc/js/dom-binding/003.html (onload test) I would be quite happy saying that attribute event handlers in XUL have a scope chain equivalent to the DOM tree, but attribute event handlers in HTML have the chain activation object -> this -> document -> window, which is what IE seems to do based on: http://www.hixie.ch/tests/adhoc/js/dom-binding/007.html http://www.hixie.ch/tests/adhoc/js/dom-binding/008.html http://www.hixie.ch/tests/adhoc/js/dom-binding/009.html Seems like this would be faster than checking every node up the tree too, no? (In fact that would be an argument for maybe doing this in XUL, too.)
jst, "Mozilla makes the scope chain of DOM nodes follow the parent chain in the DOM" I don't think that many (if any) public HTML based sites depend upon this resolution strategy. If they take it into account at all it is to work around our different behavior. From a quick grep over the XML files in Mozilla, I see very few places (printPreviewBindings.xml in particular) that appear to depend on this strategy. Most on* event handlers I found use explicit |this|, |document|, |event| and even |window| to scope attributes or make function calls which resolve against |window| anyway. This might even have a positive performance benefit if we don't have to walk up the DOM looking for resolution. "scope (this object) of an event handler is the object on which the event handler is registerd" should stay the same. This is a common idiom and many public HTML based sites use |this| to pass a reference to the element on which the handler is defined. It is also a very common usage in XBL from what I can see. I think what we are talking about is a change in the "scope chain" and not the "scope object |this|" on inline event handlers. From my limited understanding of what is going on, I don't think this would affect that much code in Mozilla.
Hixie: does IE really not include the form (if any) between the input element and the document? Nav2, et al., included the form object in the input element's scope chain, parented by the document. /be
brendan: I was talking about the non-form case. For the form case, IE and we do the same thing (and that is, we don't just go up the tree, we just do activation object -> this -> form -> document -> window). I do not propose changing the form case at all; it is extremely useful (and I am explicitly requiring it in the Web Forms spec).
Good, that's what I implemented way back in 1995 ;-). So, I think Mozilla's DOM needs to separate JS parent (scope chain link) from DOM parentNode (or whatever it's called). jst, is this gonna be hard to do? /be
This part's easy, getting Mozilla to even start with this change will be the challenging part. With this, we die (just hang in a debugger, just exit w/o a debugger) on startup, before any window appears. Not sure what the deal is, but it seems like XBL is not happy wiht this change. Needs more investigation.
I don't understand your patch though > - nsISupports *native_parent = nsnull; > + nsISupports *native_parent = doc; > > if (content) { ... > } ... > if (!native_parent) { > // We're called for a document object (since content is null), > // set the parent to be the document's global object, if there > // is one Wouldn't native_parent be non-null for a document object too (after your change)? So you'll fail to get the document's global object afaict and set the document as its own parent object.
Duh! /me feels stupid. With that fixed, Mozilla starts and appears to run w/o any obvious problems. I'll go stand in the corner for a while now (after I attach a fixed patch, that is) :-)
This appears to work.
Attachment #138370 - Attachment is obsolete: true
Ok, so that patch doesn't get the parenting entirely correct, but at least it shows that Mozilla doesn't appear to completely rely on the current scope setup, so there's still hope! :-)
Attachment #138388 - Attachment is obsolete: true
I pass Hixie's tests 001-009 with the second patch. I get a Error: doPageSetup is not defined when clicking the Page Setup button in print preview.
This patch makes Mozilla change the scope chain of HTML elements to be their form/document, but it does not change the scope chain for non-HTML elements. This should make the testcase run, w/o breaking XUL/XBL etc.
In what way did the previous patch break XUL/XBL? Or will that go in, say, early in 1.8 cycle, so we can track down all the broken XUL/XBL?
Do we want XUL and XBL to have different scope chaining from HTML? I don't know, but the answer isn't obviously "no", IMHO, so I'm not sure anything's broken there (re: comment 29). /be
I'd much rather do this one way for XUL and XBL, and another for everything else (HTML, XHTML, etc) rather than the other way around.
I don't think the existing scope chain in XUL/XBL is very well known or used widely. Having a consistent scope chain for (X)HTML, XUL, and XBL; and improving performance by not having to walk up the DOM looking for matches seem to be clear wins which outweigh the cost of a few minor patches.
Re Bob's comment, I'd say it's not used widely _yet_. There are probably 10,000 programmers who've attempted to understand implicit processing (JS or otherwise) in XUL to date. The XUL JS scope chain is more complex to understand and use than the HTML chain. This is because the XUL command dispatcher introduces an additional scoping algorithm that the learner is also tasked to absorb. This makes XUL JS scopes harder to use, even though they have little to do with command dispatch. Issues with XBL scope also make learning harder, because of the extra scoping concepts they contain. Making XUL and HTML JS scoping the same gets my vote, because it lowers concept density for learners. That makes Mozilla more useable for programmers. I do think, however, that there are wider learning issues with implicit processing in XUL, and applying this fix without debating the wider issues may be premature. - N.
Changing how the scope chain is set up in XUL/XBL scares me. Not because I think a lot of people out there understands and therefore depends on it already, but because there's a significant ammount of XUL/XBL out there already that works today (even if the developers didn't fully understand how the scoping works in XUL/XBL) and some of that code might very well break if we make a change in how the scope chain is set up. So far I like Hixies suggestion the most, and doing that is not much different from what my latest attachment already does. Anyone got any more thoughts on this? Does anyone think it's worth going through all our (and others) XUL/XBL code and take the risks involved with such a change? If not, I'll post a patch...
Well put. I vote equivalent or alternate changes for XUL be considered in the 2.0 timeframe. A separate bug?
Attachment #139080 - Attachment description: Change the scope chain everything other than XUL/XBL. → Change the scope chain for everything other than for XUL/XBL.
Attachment #139080 - Flags: superreview?(peterv)
Attachment #139080 - Flags: review?(caillon)
Comment on attachment 139080 [details] [diff] [review] Change the scope chain for everything other than for XUL/XBL. Is there some way we could warn XUL developers (posting to mozdev project newsgroup perhaps) that we would like to switch XUL too? >+ if (content->IsContentOfType(nsIContent::eHTML)) { >+ if (content->IsContentOfType(nsIContent::eELEMENT | >+ nsIContent::eHTML | >+ nsIContent::eHTML_FORM_CONTROL)) { Not sure why you need the nested check here... >+ if (ni && (ni->NamespaceEquals(kNameSpaceID_XUL) || >+ ni->NamespaceEquals(kNameSpaceID_XBL))) { I don't think we fire events on actual XBL nodes, only on the generated content (which might be in the XUL or HTML namespace)... a check for content->IsContentOfType(nsIContent::eXUL) would probably work just as well.
Comment on attachment 139080 [details] [diff] [review] Change the scope chain for everything other than for XUL/XBL. >Index: dom/src/base/nsDOMClassInfo.cpp >=================================================================== >+ // For normal content nodes, use the document as scope parent. >+ if (content->IsContentOfType(nsIContent::eHTML)) { >+ if (content->IsContentOfType(nsIContent::eELEMENT | >+ nsIContent::eHTML | >+ nsIContent::eHTML_FORM_CONTROL)) { You can drop eHTML here (I doubt eHTML_FORM_CONTROL would ever make sense without eELEMENT but anyway ;-)).
Attachment #139080 - Flags: superreview?(peterv) → superreview+
Comment on attachment 139080 [details] [diff] [review] Change the scope chain for everything other than for XUL/XBL. >+ // For normal content nodes, use the document as scope parent. >+ if (content->IsContentOfType(nsIContent::eHTML)) { >+ if (content->IsContentOfType(nsIContent::eELEMENT | >+ nsIContent::eHTML | Yeah, go ahead and drop this eHTML check. r=caillon. Let's land this so we can get feedback. Neil, the nesting is there so we can avoid going into the else branch if isContentOfType(eHTML), which would be rather silly.
Attachment #139080 - Flags: review?(caillon) → review+
Perhaps as a compromise we could make XUL scope walk up binding parents? Also, a reminder that we don't use XBL nodes outside of the XBL content sink ("use" in the sense that XBL nodes have no particular meaning).
Fixed. I flipped the code around a bit, I check if we're XUL first, then use the parent, if not, I use the doc, except if it's a form control that has a form.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
This caused bug 235730
Nope, it didn't. See my comment in bug 235730.
Ok, fair enough. This did cause bug 242557 and its dependencies, however.
This also regressed bug 126726. But then, it had been fixed incorrectly anyway.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: