Chris Manchester (limited bugmail, email directly) Reporter
	Description • 10 years ago

I'm trying to convert a test case that clicks the "Open a Private Window" button in "about:privatebrowsing". I can select the button from the content scope with a selector ("button.openPrivate") and get an element back, but the click fails with an ElementNotVisible exception. From chrome scope I try to select the button but get 
"JavascriptException: No such strategy". I guess the visibility check isn't applicable for xul elements, and finding elements with a selector doesn't really make sense in chrome scope, so we have a catch 22.

	Andrew Halberstadt [:ahal]
	Comment 1 • 10 years ago

I actually just noticed the same thing with "send_keys" in about:preferences. The second part of your problem (no such strategy) is because "css selector" isn't supported in chrome scope for some reason, I filed bug 1094441 to enable it.

	Chris Manchester (limited bugmail, email directly) Reporter
	Comment 2 • 10 years ago

(In reply to Andrew Halberstadt [:ahal] from comment #1)
> I actually just noticed the same thing with "send_keys" in
> about:preferences. The second part of your problem (no such strategy) is
> because "css selector" isn't supported in chrome scope for some reason, I
> filed bug 1094441 to enable it.

Cool, thanks Andrew! I guess this might still be an issue if about: pages are in the content process.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 3 • 10 years ago

FYI: Nearly all of the about: pages are in-content XUL documents.

	Andrew Halberstadt [:ahal]
	Comment 4 • 10 years ago

I think Chris is looking into something else, I'll investigate.

	Andrew Halberstadt [:ahal]
	Comment 5 • 10 years ago

The webdriver isDisplayed atom is returning false. Because marionette uses a minified version, I'm unable to step through it but I think the function that ultimately is being called is:
https://code.google.com/p/selenium/source/browse/javascript/atoms/dom.js#557

That's a relatively complicated function, so I'm not sure where along there false is being returned. Because this is xul, I'm not even sure that fixing it upstream is desirable. Maybe we just need to detect whether we're dealing with a xul element and write our own isDisplayed function for it instead of invoking the webdriver atom.

David, do you know if..

1) It's possible to make marionette use the non-minimized atoms so I can step through it with the debugger?
2) This should be fixed in upstream webdriver or not?

	Andrew Halberstadt [:ahal]
	Comment 6 • 10 years ago

FWIW, removing the visibility check here makes everything work:
http://dxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-listener.js#1426

I'll come up with a patch that fixes things on our side while we decide what to do with the upstream function.

	David Burns :automatedtester
	Comment 7 • 10 years ago

(In reply to Andrew Halberstadt [:ahal] from comment #5)
> The webdriver isDisplayed atom is returning false. Because marionette uses a
> minified version, I'm unable to step through it but I think the function
> that ultimately is being called is:
> https://code.google.com/p/selenium/source/browse/javascript/atoms/dom.js#557
> 
> That's a relatively complicated function, so I'm not sure where along there
> false is being returned. Because this is xul, I'm not even sure that fixing
> it upstream is desirable. Maybe we just need to detect whether we're dealing
> with a xul element and write our own isDisplayed function for it instead of
> invoking the webdriver atom.
> 
> David, do you know if..
> 
> 1) It's possible to make marionette use the non-minimized atoms so I can
> step through it with the debugger?

I see that jleyba answered your question

> 2) This should be fixed in upstream webdriver or not?

We could upstream this but it would need to be behind firefox specific conditionals on the atom.



You can also do |./go clean debug-server| and then visit http://localhost:2310/javascript/atoms/test/file_with_xul_test.html

I suspect the issue is that we may need to write this from scratch for XUL because XUL.

ato has rewritten this part of the spec recently (https://github.com/w3c/webdriver/pull/4) and we could have a look at implementing it from scratch

	Andrew Halberstadt [:ahal]
	Comment 8 • 10 years ago

I decided to throw in the towel with regards to figuring out exactly where in isShown() the problem is happening. AutomatedTester thinks we'll basically re-implement it from scratch for XUL elements, so I think for now we should just not call into the Webdriver implementation if we're dealing with XUL.

I filed bug 1096571 to track that change. This bug will remain about solving the issue properly (either by fixing upstream webdriver or implementing our own XUL specific method).

	Andreas Tolfsen ❲:ato❳
	Comment 9 • 10 years ago

Possibly the element visibility algorithm should have an explicit type check that the given input is a DOMNode.

	Dan Mosedale (:dmosedale, :dmose)
	Comment 11 • 9 years ago

A great temporary measure would be to have element.is_displayed() return an NotYetImplemented bug, ideally with a pointer to a link describing how to work around it.  This would at least prevent people from sinking a lot of time into debugging something that it appears that it ought to work.

	Andreas Tolfsen ❲:ato❳
	Comment 12 • 9 years ago

(In reply to Dan Mosedale (:dmose) - needinfo? me for response from comment #11)
> A great temporary measure would be to have element.is_displayed() return an
> NotYetImplemented bug, ideally with a pointer to a link describing how to
> work around it.  This would at least prevent people from sinking a lot of
> time into debugging something that it appears that it ought to work.

There's no such error code in WebDriver as far as I'm aware, but we could use a generic error with a custom message.  I think it's a good idea to avoid confusion.

(In reply to Andrew Halberstadt [:ahal] from comment #5) 
> 1) It's possible to make marionette use the non-minimized atoms so I can
> step through it with the debugger?

I looked into upgrading the atoms about a year ago or so with no success.  Admittedly I didn't try very hard, but the way the atoms have been generated and exported for Marionette is not documented anywhere.  This means it's actually quite hard to figure out exactly which atoms are used in Marionette.

However, upgrading the atoms seems like it could be a separate bug.  I think the earlier suggestion to special case content XUL documents could make sense for the time being, or at least returning an error until we come up with something better.

	Andrew Halberstadt [:ahal]
	Comment 13 • 9 years ago

FTR the change to special case content XUL landed awhile ago in bug 1096571:
https://dxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-listener.js#841

I guess there's a code path that bypasses that check that the loop team must have hit?

	Dan Mosedale (:dmosedale, :dmose)
	Comment 14 • 9 years ago

(In reply to Andreas Tolfsen (:ato) from comment #12)
> (In reply to Dan Mosedale (:dmose) - needinfo? me for response from comment
> #11)
> > A great temporary measure would be to have element.is_displayed() return an
> > NotYetImplemented bug, ideally with a pointer to a link describing how to
> > work around it.  This would at least prevent people from sinking a lot of
> > time into debugging something that it appears that it ought to work.
> 
> There's no such error code in WebDriver as far as I'm aware, but we could
> use a generic error with a custom message.  I think it's a good idea to
> avoid confusion.

That would be wonderful.  Right now, the failure mode is that the wait simply times out, leaving the programmer to think that the element isn't being displayed.

For a custom message, how about something like "element.is_displayed() is not yet implemented for XUL/XBL elements; consider using marionette.find_element() as a workaround".

Is there any chance this could be done relatively soon to avoid devs sinking a lot of time following the wrong path?

	Andreas Tolfsen ❲:ato❳
	Comment 15 • 9 years ago

(In reply to Dan Mosedale (:dmose) - needinfo? me for response from comment #14)
> (In reply to Andreas Tolfsen (:ato) from comment #12)
> > (In reply to Dan Mosedale (:dmose) - needinfo? me for response from comment
> > #11)
> > > A great temporary measure would be to have element.is_displayed() return an
> > > NotYetImplemented bug, ideally with a pointer to a link describing how to
> > > work around it.  This would at least prevent people from sinking a lot of
> > > time into debugging something that it appears that it ought to work.
> > 
> > There's no such error code in WebDriver as far as I'm aware, but we could
> > use a generic error with a custom message.  I think it's a good idea to
> > avoid confusion.
> 
> That would be wonderful.  Right now, the failure mode is that the wait
> simply times out, leaving the programmer to think that the element isn't
> being displayed.
> 
> For a custom message, how about something like "element.is_displayed() is
> not yet implemented for XUL/XBL elements; consider using
> marionette.find_element() as a workaround".
> 
> Is there any chance this could be done relatively soon to avoid devs sinking
> a lot of time following the wrong path?

Just to clarify: Should all calls to to isElementDisplayed in chrome context return with an error, or just calling that on elements in about:* documents?

	Dan Mosedale (:dmosedale, :dmose)
	Comment 16 • 9 years ago

Everywhere, for the main reason that it does the wrong thing in (presumably?) all chrome contexts, which currently leaves any developer using Marionette for chrome testing vulnerable to wasting a bunch of time debugging the wrong thing.

We're certainly seeing the problem in normal chrome context, on an element which only contains an about: URL in an internal iframe down inside the XBL.

	Dave Hunt [:davehunt] [he/him] ⌚BST
	Comment 17 • 8 years ago

I believe this is affecting my ability to wait for a popup notification (door hanger) to be displayed when attempting to install an add-on from https://addons.mozilla.org/. Is there an update or workaround, short of having lots of hard-coded sleeps?

	David Burns :automatedtester
	Comment 18 • 8 years ago

(In reply to Dave Hunt (:davehunt) from comment #17)
> I believe this is affecting my ability to wait for a popup notification
> (door hanger) to be displayed when attempting to install an add-on from
> https://addons.mozilla.org/. Is there an update or workaround, short of
> having lots of hard-coded sleeps?

Unfortunately we are not working on Chrome stuff as a priority as we need to have better parity with webdriver first. I don't know of a workaround currently.

	Andreas Tolfsen ❲:ato❳
	Comment 19 • 8 years ago

If the XUL element supports elementsFromPoint you might be able to use element.isPointerInteractable that I implemented some time ago: https://github.com/mozilla/gecko-dev/blob/master/testing/marionette/element.js#L866

You will unfortunately have to hack Marionette a bit to expose it, but it would be very interesting to hear if that works for you!

	Benjamin Forehand Jr[:b4hand]
	Comment 20 • 7 years ago

I have ran into this bug wanting to test about:addons. Has there been a change that could make this possible, or is the information within this bug still worth following.

	Andreas Tolfsen ❲:ato❳
	Comment 21 • 7 years ago

(In reply to Benjamin Forehand Jr[:b4hand] from comment #20)

> I have ran into this bug wanting to test about:addons. Has there
> been a change that could make this possible, or is the information
> within this bug still worth following.

https://bugzil.la/1321516 tracks enabling the WebDriver-conforming
click implementation by default.  Since then we have exposed a
"moz:webdriverClick" capability that lets you enable it from the
client.

However, I realise when re-reading my last comment that the new
click implementation will only work for DOM elements.  If the
element you’re interacting with is a XUL namespaced element you
will still see the same error.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 22 • 7 years ago

I just tested this issue while working on bug 1414322, and it is no longer a problem. Looks like the update of the Selenium atoms as done via bug 1375660 made it work.

I will add a new testcase for that report with my patch on bug 1414322.