Opened 8 years ago

Closed 7 years ago

Last modified 6 years ago

#12342 closed defect (fixed)

IE9: tests/_base/html.html failures

Reported by: Kenneth G. Franqueiro Owned by: Eugene Lazutkin
Priority: blocker Milestone: 1.6.2
Component: HTML Version: 1.6.0rc1
Keywords: ie9 dohfail Cc: Douglas Hays
Blocked By: Blocking:

Description

Just ran a few more test pages ensuring they ran in IE9 standards mode where appropriate, and noticed two test failures in http://archive.dojotoolkit.org/nightly/dojotoolkit/dojo/tests/_base/html.html

One is in testIframeDestroy10095 (presumably referencing ticket #10095), the other is in coordsIframe (specifically line 194, though then the test bails out of the function so I'm not sure what other assertions would also fail).

Attachments (3)

destroyperf.html (3.3 KB) - added by Douglas Hays 7 years ago.
standalone test file showing proposed empty and destroy are significantly faster
fastEmptyAndDestroy.patch (2.2 KB) - added by Douglas Hays 7 years ago.
proposed fix for destroy on IE9
destroymemory.html (1.6 KB) - added by Douglas Hays 7 years ago.
standalone leak test showing no leak, tested with IE9/Win7 and IE6/WinXP using Task Manager

Download all attachments as: .zip

Change History (32)

comment:1 Changed 8 years ago by Eugene Lazutkin

Owner: changed from anonymous to Eugene Lazutkin
Status: newassigned

comment:2 Changed 8 years ago by Kenneth G. Franqueiro

Milestone: tbd1.6.1

comment:3 Changed 8 years ago by bill

Keywords: dohfail added

comment:4 Changed 8 years ago by bill

Milestone: 1.6.21.8

Moving apparently forgotten ticket to 1.8.

comment:5 Changed 7 years ago by bill

Component: CoreHTML

comment:6 Changed 7 years ago by Colin Snover

Milestone: 1.82.0

1.8 has been tagged; moving all outstanding tickets to next major release milestone.

comment:7 Changed 7 years ago by Colin Snover

In [30005]:

IE9+ do not have bugs in offsets, so stop trying to "fix" them. This is why browser sniffs are bad. Refs #12342.

comment:8 Changed 7 years ago by Colin Snover

In [30006]:

IE9+ do not have bugs in offsets, so stop trying to "fix" them. This is why browser sniffs are bad. Refs #12342.
Backport to 1.8.

comment:9 Changed 7 years ago by Colin Snover

  • Surprise, browser sniffs are dumb and break our code. I fixed the sniff for IE to never adjust offsets in >=9 and that resolved the iframe offset test case, and fixed the sniff *in the test suite* that was breaking things.
  • The test for 10095 makes no sense to me, and I suspect the fact that it is failing doesn’t mean a whole lot. Supporting what that test code appears to be trying to do is stupid (delete an element from an iframe from the parent frame, then set the iframe src back to itself, then check to see if the element doesn’t exist anymore? this seems like an incredibly indirect inference to see if iframe.src = iframe.src with a javascript: uri reloads an iframe and nothing more), so I’m not even going to bother spending my time looking into it any more. If someone else wants to do it, feel free. As far as I am concerned this test should be deleted from the test suite.
  • Same thing with the quirks mode tests. Quirks mode is called quirks mode for a reason, and it’s a waste of time to create tests to make sure things are “consistent” when it is being used. It’s 2012, use a bloody doctype. Also, these tests pass just fine when outside the test runner. When they fail it appears to be the issue where border gets folded into the element margin calculation, and there is some code to address that, but obviously the dumb combination of quirks mode iframe in strict mode document doesn’t work great in IE9. Stop using quirks mode!
  • The testSpeed failure hits all versions of IE and is #11200.

As far as I am concerned the tests that are still there and failing probably should not be there at all because supporting anti-patterns like these is dragging the toolkit down, bloating our code, and wasting effort that needs to be focused on leading the industry. I look forward to people telling me I am wrong, but only so long as those people fix the tests first.

Last edited 7 years ago by Colin Snover (previous) (diff)

comment:10 Changed 7 years ago by Colin Snover

In [30007]:

IE9+ do not have bugs in offsets, so stop trying to "fix" them. This is why browser sniffs are bad. Refs #12342.
Backport to 1.7.

comment:11 Changed 7 years ago by bill

Cc: Douglas Hays added

Thanks for the dom-geometry fix.

Quirks mode test failures are covered in #14321. As you said above, the tests run fine when called outside the test runner, the only issue is that the standards mode of the main window (runner.html) leaks down into the iframe loading the *_quirks.html file. So, html_quirks.html should not be changed, except maybe to not run at all if !has("quirks").

Doug should handle the #10095 test failure.

comment:12 Changed 7 years ago by Colin Snover

In [30008]:

As it turns out, has("ie") returns undefined, not false, so it does not need to be checked twice. Refs #12342.

comment:13 Changed 7 years ago by Colin Snover

In [30009]:

IE9 does actually continue to do the wrong thing in quirks mode, it just doesn't do quirks mode in iframes of standards mode documents. Refs #12342, #14321.

comment:14 Changed 7 years ago by Colin Snover

In [30010]:

As it turns out, has("ie") returns undefined, not false, so it does not need to be checked twice. Refs #12342. Backport to 1.8.

comment:15 Changed 7 years ago by Colin Snover

In [30011]:

As it turns out, has("ie") returns undefined, not false, so it does not need to be checked twice. Refs #12342. Backport to 1.7.

comment:16 Changed 7 years ago by Colin Snover

In [30012]:

IE9 does actually continue to do the wrong thing in quirks mode, it just doesn't do quirks mode in iframes of standards mode documents. Refs #12342, #14321. Backport to 1.7.

comment:17 Changed 7 years ago by Colin Snover

In [30013]:

IE9 does actually continue to do the wrong thing in quirks mode, it just doesn't do quirks mode in iframes of standards mode documents. Refs #12342, #14321. Backport to 1.8.

comment:18 Changed 7 years ago by Colin Snover

Quirks mode tests were disabled to fix #14321. So the only remaining failures in this suite are the 10095 test and the intermittent testSpeed failure.

comment:19 Changed 7 years ago by Douglas Hays

IE9 isn't allowing destroy() to do innerHTML = "" for iframe content. This isn't surprising since empty() also doesn't work with innerHTML. I appended a rewrite for destroy that is faster on all browsers that I tested (Chrome,FF,IE6-9 all on a Win 7 PC) and works around the ownerDocument issues associated with iframes. Also, the current code is leaking when calls to destroy() oscillate between iframes since a new container div is created each time. Obviously this patch needs careful review.

Changed 7 years ago by Douglas Hays

Attachment: destroyperf.html added

standalone test file showing proposed empty and destroy are significantly faster

comment:20 Changed 7 years ago by Douglas Hays

Refreshed the patch and performance test file to call destroy from empty (like current trunk) instead of removeChild for proper TABLE element unwinding. Added internal _empty and _destroy functions to avoid redundant computations when unwinding child elements.

Changed 7 years ago by Douglas Hays

Attachment: fastEmptyAndDestroy.patch added

proposed fix for destroy on IE9

comment:21 Changed 7 years ago by bill

I'm OK with the patch.

The current "garbage can" implementation, from #1727 and #2931, has always been debated about whether or not its necessary, and I'm ready to give it up, given the declining popularity of IE6 and IE7, and the leak you mentioned above when oscillating between destroying DOMNodes in different documents. It might be worth testing though. Not necessarily worrying about what sIeve says, but checking actual memory usage.

I'd prefer to have the

try{
	node.innerHTML = ""; // really fast when it works
}catch(e){
       // alternate implementation

code path running for all browsers rather than just IE, since that simplifies the code a bit and I doubt there's much of a performance hit, especially since browsers other than IE are very fast. Either way is OK though.

Changed 7 years ago by Douglas Hays

Attachment: destroymemory.html added

standalone leak test showing no leak, tested with IE9/Win7 and IE6/WinXP using Task Manager

comment:22 Changed 7 years ago by bill

Milestone: 2.01.8.2
Priority: highblocker

Eugene said he would look at this tomorrow.

comment:23 Changed 7 years ago by Eugene Lazutkin

I like the patch --- good thorough work. Please commit.

comment:24 Changed 7 years ago by bill

I also double checked the memory leak; it seems to be working. The original ticket talked about a memory leak with removeChild(), and since you aren't using removeChild() anymore I guess there's nothing to worry about.

comment:25 Changed 7 years ago by Douglas Hays

Resolution: fixed
Status: assignedclosed

In [30096]:

Fixes #12342. Refactor destroy and empty to work with IE9/10 and avoid multi-doc exceptions. Backported attr_reconnect test to 1.7 to work with IE10.

comment:26 Changed 7 years ago by bill

In [30193]:

Rollback [30005] and [30009], which were wrong, and re-broke the html_quirks.html test on IE10. Refs #15773, #12342.

comment:27 Changed 6 years ago by Douglas Hays

In [30338]:

Refs #12342. Backport [30011] and [30096] to 1.6 to support IE 9/10. !strict

comment:28 Changed 6 years ago by Douglas Hays

Milestone: 1.8.21.6.2

comment:29 Changed 6 years ago by bill

In [30400]:

remove dojo/on dependency, it's no longer used, refs #12342 !strict

Note: See TracTickets for help on using tickets.