956753 - findElement.Elem() does not work for non XUL objects

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

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

The method findElement.Elem() doesn't work for non XUL objects like a window or a document, because some MozElement classes assume that localName is always present. We should catch that and most likely return early in:

https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/resource/driver/mozelement.js#L37

Comment 1

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

An example test which will fail is:

var testNewPageLoaded = function () {
  var win = new findElement.Elem(controller.window.document);
}

Comment 2

[Stefan]

I may be able to handle this one.

Comment 3

[Stefan]

Should the isType methods for the various MozMillElement types be changed to something like the following?
(in mozelement.js)
 851 MozMillCheckBox.isType = function MMCB_isType(node) {$                         
 852   return node.hasOwnProperty('localName') && ((node.localName.toLowerCase() == "input" && node.getA>
 853     (node.localName.toLowerCase() == 'toolbarbutton' && node.getAttribute('type') == 'checkbox') ||$
 854     (node.localName.toLowerCase() == 'checkbox'));$                            
 855 };$

It appears to make the test from comment 1 pass, but I'm not sure if this is the change you're looking for.

Comment 4

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

Hi Stefan. Thank you for your interest in helping us on this bug. Let me assign it to you so it's clear to everyone else that you are working on it.

(In reply to Stefan from comment #3)
> Should the isType methods for the various MozMillElement types be changed to
> something like the following?

Most likely all this code would need a refactoring through. But to fix this specific issue your proposal sounds good. That way we can ensure that isType() doesn't raise an exception because localName is not available for a specific node.

Another option would be to add this check to the if condition as pointed out above, so that we do not loop over the subclasses at all. Given that isType() is only used here, it would be fine too.

In the future we should enhance the constructors of the individual classes to also call isType() so we can ensure to not create an e.g. checkbox element out from another node type. But for this we can file a follow-up bug.

Comment 5

[Stefan]

Attachment: bug956753.patch

createInstance now will only run the subclass type checks if the dom element has the localName property.

Comment 6

[Stefan]

Hopefully that patch is sufficient! I may be able to handle your suggestion to enhance the constructors also.

Comment 7

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

Comment on attachment 8385642 [details] [diff] [review]
bug956753.patch

Review of attachment 8385642 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozmill/mozmill/extension/resource/driver/mozelement.js
@@ +35,4 @@
>    var args = { "document": document, "element": elem };
>  
>    // If we already have an element lets determine the best MozMillElement type
> +  if (elem.hasOwnProperty("localName")) {

`elem` can be undefined and this would cause an exception. So you should keep the former check and extent it with &&.

Comment 8

[Stefan]

Attachment: bug956753_redux.patch

Sorry about my first patch, I should be more careful. This one should fix it properly.

Comment 9

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

Comment on attachment 8386056 [details] [diff] [review]
bug956753_redux.patch

Review of attachment 8386056 [details] [diff] [review]:
-----------------------------------------------------------------

That looks good now Stefan! Thanks for the update. Beside the change of the code, you will also have to update the mutt tests which retrieve such kind of element (windows, document). Can you please update those cases to use findElement.Elem() now? 

One instance you can find here:
https://github.com/mozilla/mozmill/blob/master/mutt/mutt/tests/js/testElementsLib/testMozElementWindow.js#L14

r=me but please update the tests. Once done I can get the patch landed.

Comment 10

[Stefan]

Attachment: bug956753_part1.patch

Hey Henrik, it seems checking for the localName property by calling hasOwnProperty gives 6 "elem.hasOwnProperty is not a function" exceptions when running the mutt js tests. I was able to clear these up by checking if localName itself is undefined, as in:

if (elem && elem.localName) {
...

I will upload a second patch containing the mutt test changes.

Comment 11

[Stefan]

Attachment: bug956753_part2.patch

This patch makes the change to the mutt test you linked above. As far as I can tell, this is the only one that needed changing. If you see any others let me know!

Comment 12

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

Comment on attachment 8392378 [details] [diff] [review]
bug956753_part2.patch

Review of attachment 8392378 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine to me and I will get this landed.

Here a hint for the future... Please squash your changes first before creating the patch. And then please use 'git format-patch HEAD^'. I mention that because with those steps changes can be seen in the interdiff on Bugzilla. Thanks.

Comment 13

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

https://github.com/mozilla/mozmill/commit/8c1e02342f8ffd14f70300c56ec26f316cbbbdee (master)

Stefan, let me know if you have interest in working on something else. We have indeed some more challenges and stuff to learn for you.

Comment 14

[Stefan]

That sounds great, I would love to!

Comment 15

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

(In reply to Stefan from comment #14)
> That sounds great, I would love to!

Great. So would you have interest to work on bug 979923? That's something new and shiny.