Closed Bug 246964 Opened 16 years ago Closed 16 years ago

Special-case 'if (document.all)...'-style object detection in JSClass.resolve

Categories

(Core :: JavaScript Engine, defect, P3)

defect

Tracking

()

VERIFIED FIXED
mozilla1.8alpha2

People

(Reporter: brendan, Assigned: brendan)

Details

(Keywords: fixed-aviary1.0, fixed1.7.5, js1.5)

Attachments

(2 files)

To handle IE content that uses document.all without testing for it, we could add
a mode to resolve, signaled by JSRESOLVE_DETECTING being set in JSNewREsolveOp's
flags argument, that the unresolved property reference is the condition of an if
or ?: condition (possibly parenthesized).  Thus, we could fail to resolve when
pages are object-detecting document.all, but willingly resolve when pages use
document.all[id].

I do not propose to handle &&, ||, or ! combinations.

Comments?

/be
Worth a shot, definately. I can hack up the other side of this once you've got
the flag for me to use...
Simple and direct, including js shell it.noisy test fun.

/be
Comment on attachment 150885 [details] [diff] [review]
proposed enhancement

jst, feel free to try this and take advantage of it in dom code....

/be
Attachment #150885 - Flags: review?(shaver)
jst: probably want to insist on JSRESOLVE_QUALIFIED in order to even consider
resolving document.all, but then bail on the resolution (return with null in
*objp) if JSRESOLVE_DETECTING is set.

/be
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla1.8alpha2
This is an interesting approach which might be helpful for cases where an App is
written solely for MSIE without impacting us in cases where the author has coded
for both MSIE and us. It is not clear to me how much this will get you without
supporting more of the MSIE DOM especially MSIE's exposure of elements w/ID as
global vars.

jst, will you do just document.all and not HTMLElement.all ? How much of the
other parts like indexing, tags, item, etc? I can run some scans to see how this
plays out when you have a preliminary patch.
(In reply to comment #5)
> It is not clear to me how much this will get you without
> supporting more of the MSIE DOM especially MSIE's exposure of elements w/ID as
> global vars.

doesn't MSIE also have a global |event| variable, contrary to mozilla, which
pages often use?
(In reply to comment #6)
> 
> doesn't MSIE also have a global |event| variable, contrary to mozilla, which
> pages often use?

yes.

I think the best approach here is to make incremental changes and test and see
how far it gets us, if we find that doing document.all gets us far, but taking
one more step would get a significant improvement, then lets think about that,
separately for each case we run into.
Comment on attachment 150885 [details] [diff] [review]
proposed enhancement

r=shaver
Attachment #150885 - Flags: review?(shaver) → review+
Fix is in.  jst, can you file the DOM followup bug?

/be
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Filed bug 248549 on DOM: Level 0 for followup.

/be
I assume that with this patch something like:

  var isIE = document.all;
  if (isIE) {
  }

will continue to test false in Gecko?
bz: yes, as close reading of bug 248549 should make clear (bug 248549 comment 3
says so directly, and bug 248549 comment 17 says more).

/be
I should have said that this bug's patch doesn do what bz wants, but the second
attachment in bug 248549 does.

/be
Landed on the aviary branch too.
Keywords: fixed-aviary1.0
Keywords: fixed1.7.5
Thanks to Brendan, Boris and Steve. See bug 248549, 253150, 259935.

Steve, with your permission this will be included in the javascript test
library.
Yes, you have my permission. Thanks for listing me as a contributor.
js1_5/Regress/regress-246964.js checked in.
Flags: testcase+
verified fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.