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

VERIFIED FIXED in mozilla1.8alpha2

Status

()

P3
normal
VERIFIED FIXED
15 years ago
13 years ago

People

(Reporter: brendan, Assigned: brendan)

Tracking

({fixed-aviary1.0, fixed1.7.5, js1.5})

Trunk
mozilla1.8alpha2
fixed-aviary1.0, fixed1.7.5, js1.5
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

15 years ago
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...
(Assignee)

Comment 2

15 years ago
Simple and direct, including js shell it.noisy test fun.

/be
(Assignee)

Comment 3

15 years ago
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)
(Assignee)

Comment 4

15 years ago
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
(Assignee)

Updated

15 years ago
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+
(Assignee)

Comment 10

15 years ago
Fix is in.  jst, can you file the DOM followup bug?

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
(Assignee)

Comment 11

15 years ago
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?
(Assignee)

Comment 13

15 years ago
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
(Assignee)

Comment 14

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

Updated

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

Comment 17

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