760410 - Regression in test dnd/testhtml.js ("Unexpectedly found element ID: item1")

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

While working on getting all the JS tests enabled again, I have seen that we have a regression in the following test:

https://github.com/mozautomation/mozmill/blob/master/mutt/mutt/tests/js/dnd/testhtml.js

After the drag and drop action the the element item1 is still present for Mozmill while it doesn't exist anymore in the page. I have verified that with Firebug.

This is working fine with Mozmill 1.5.12 but is broken on master. I assume it has been caused by the MozmillElement feature.

Requesting for blocking Mozmill 2.0. I will search for the regression range.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Yeah, that has been broken since the MozElement class has been introduced in bug 632451.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The same applies to the test (test.xul) which runs under chrome.

Comment 3

[cmtalbert]

Ahal, would you mind taking a look at this?

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Just a note for you Andrew, I will not touch those affected tests by my patch on bug 760418. So you can work independently on it.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

What I meant were the example files which are stored in the extension. But I have moved the dnd tests under mutt/tests/js/controller. Please see the before mentioned bug and my pull request which is still in WIP but hopefully fixed by tomorrow.

Comment 6

[Andrew Halberstadt [:ahal]]

The test is definitely broken, but I don't think the regression is caused by bug 632451 since the dragToElement function calls EventUtils directly.

It looks like the mouseDown followed by mouseMove works as expected, but then the mouseUp event is not being processed. I'll have to look deeper into EventUtils for this one.

Comment 7

[Andrew Halberstadt [:ahal]]

Attachment: Patch 1.0 - Fix broken controller.assertNode* methods

I was mistaken in the previous comment, it is the controller.assertNodeNotExist method that wasn't working. And it was caused by the mozelement refactor.

The problem was that the DOM object was getting stored locally in JS and doesn't get rechecked for existence. Luckily there is a built-in element.exists() method for exactly this reason so the fix is simple.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 631493 [details] [diff] [review]
Patch 1.0 - Fix broken controller.assertNode* methods

As already filed as bug 763056, lets get all those crufty old assert methods removed in favor of the assertions module then.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So lets get the test fixed and re-enabled by using assert/except.ok(elem.exists().

Comment 10

[Andrew Halberstadt [:ahal]]

Attachment: Patch 2.0 - Fix controller/dnd_content.js

Fixes this test. I left the controller.assert* methods alone as they will be removed in the other bug.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 631902 [details] [diff] [review]
Patch 2.0 - Fix controller/dnd_content.js

Please also enable the test in the manifest.

Comment 12

[Andrew Halberstadt [:ahal]]

Attachment: Patch 3.0 - Fix dnd_content.js and dnd_chrome.js

dnd_chrome.js had the same problem so I fixed and enabled it as well.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 631944 [details] [diff] [review]
Patch 3.0 - Fix dnd_content.js and dnd_chrome.js

Wohoo! Great. Thanks Ahal.

Comment 14

[Andrew Halberstadt [:ahal]]

master: https://github.com/mozautomation/mozmill/commit/63f516157dea4844e1e47aec8406f8f0e5f09478