Identifier resolution walks up DOM

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
17 years ago
14 years ago

People

(Reporter: ian, Unassigned)

Tracking

({testcase})

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

17 years ago
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.
(Reporter)

Comment 1

17 years ago
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".

Comment 2

17 years ago
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 ;-)
(Reporter)

Comment 3

17 years ago
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
(Reporter)

Comment 5

17 years ago
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?
(Reporter)

Comment 6

17 years ago
*** 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
(Reporter)

Comment 9

16 years ago
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
(Reporter)

Comment 11

16 years ago
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.

Comment 13

16 years ago
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
(Reporter)

Comment 14

16 years ago
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.
(Reporter)

Comment 16

16 years ago
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.)

Comment 17

16 years ago
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
(Reporter)

Comment 19

16 years ago
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

Comment 27

16 years ago
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
(Reporter)

Comment 31

16 years ago
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.

Comment 32

16 years ago
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.

Comment 33

16 years ago
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...

Comment 35

16 years ago
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
Last Resolved: 16 years ago
Resolution: --- → FIXED
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.