David Burns :automatedtester Reporter
	Description • 7 years ago

It looks like there has been a change to the selenium atom that is causing people to get different values from Firefox compared to the other drivers/browsers

	David Burns :automatedtester Reporter
	Comment 2 • 7 years ago

Dave, can you download one of the builds from treeherder, linked from mozreview and see if that works for you as expected

	Dave Hunt [:davehunt] [he/him] ⌚BST
	Comment 3 • 7 years ago

I downloaded the target.dmg from the OS X 10.10 opt build from https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd5dc3c73291c1d431abaebd61bb34b9f83f06ae, extracted Nightly.app, set this as my target binary and the test still failed for the same reason.

Here are my steps to reproduce (using Python selenium package 3.4.3):

from selenium.webdriver import Firefox
from selenium.webdriver.firefox.options import Options
options = Options()
options.binary = '/Users/dhunt/Downloads/Nightly.app/Contents/MacOS/firefox-bin'
firefox = Firefox(firefox_options=options)
firefox.get('https://addons.mozilla.org/en-US/firefox/addon/firebug/')
firefox.find_element_by_css_selector('#preview .thumbnail').click()
assert not firefox.find_element_by_css_selector('#lightbox .next').is_displayed()

	David Burns :automatedtester Reporter
	Comment 4 • 7 years ago

Then this is not because the atom is out of date, we need to figure out what Selenium is doing differently.

	David Burns :automatedtester Reporter
	Comment 5 • 7 years ago

The atom does appear to be slightly out of date so that needs fixing but the underlying issue is we appear to be doing the wrong thing for isDisplayed...

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

Given that we have to update a couple of different atoms lets get this done all at once in this bug. For now we know that there is at least bug 1381519 which might benefit from an update too.

Sadly there are no instructions available in how to build the atoms. So I had to do a bit of investigation...

I queried a bit and found: https://github.com/SeleniumHQ/selenium/wiki/Automation-Atoms

Sadly it doesn't tell that much about compiled fragments, and the link at the bottom is also outdated. But I found build targets like: "./go //javascript/atoms/fragments:get_visible_text:firefox", which generates a minimized version of this specific method. Having a look at our Marionette code I can find the following atom methods in use:

clearElement
getElementAttribute
getElementText
isElementDisplayed
isElementEnabled
isElementSelected

In bug 1245153 Andreas moved the atoms from the /atoms subfolder directly into /testing/marionette. So the build instruction also got removed. But thanks to the history here those are: https://hg.mozilla.org/mozilla-central/diff/3edb67388ad6/testing/marionette/atoms/HOWTO.

So as it looks like we would have to compile the methods for each of the above listed items, and feed the individual functions in atoms.js with the generated code.

Here the build targets which I found for the methods above:

./go //javascript/atoms/fragments:is_enabled:firefox
./go //javascript/atoms/fragments:clear:firefox
./go //javascript/atoms/fragments:get_attribute:firefox
./go //javascript/atoms/fragments:get_visible_text:firefox
./go //javascript/atoms/fragments:is_shown:firefox
./go //javascript/atoms/fragments:is_selected:firefox

I will check how updating our used atoms work, and file dependencies if necessary.

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

I got all the changes integrated, but noticed some problems:

1) The clearElement update is not working given that the code in atom.js is requiring a global window object which does not exist in our case. When I run the code in a scratch pad it works all fine. But in Marionette I get a window reference not found

2) With the update of isElementDisplayed I see a test failure in test_accessibility.py which is not that clear to me and would need further investigation.

3) Exporting those atoms separately seem to add a huge overhead to the code. David, do you know how to get them all exported with as minimal as possible in size?

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

(In reply to Henrik Skupin (:whimboo) from comment #7)
> 1) The clearElement update is not working given that the code in atom.js is
> requiring a global window object which does not exist in our case. When I
> run the code in a scratch pad it works all fine. But in Marionette I get a
> window reference not found

Just to add... where is this code coming from:

> goog.require('bot.action'); goog.exportSymbol('_', bot.action.clear);

I assume it's a Google API. It would be good to have a look if the calling conventions of clear have been changed.

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

(In reply to Henrik Skupin (:whimboo) from comment #7)
> 1) The clearElement update is not working given that the code in atom.js is
> requiring a global window object which does not exist in our case. When I
> run the code in a scratch pad it works all fine. But in Marionette I get a
> window reference not found

After talking to Simon he proposed just to add the parameters for element and window back into the imported function. After I did that all is working fine.

> 2) With the update of isElementDisplayed I see a test failure in
> test_accessibility.py which is not that clear to me and would need further
> investigation.

This needs investigation.

> 3) Exporting those atoms separately seem to add a huge overhead to the code.
> David, do you know how to get them all exported with as minimal as possible
> in size?

I exported the wrong ones. We don't need /javascript/atoms but /javascript/webdriver/atoms. Given that there were no rules anymore in BUCKET Simon did us the favor and added them back via:

https://github.com/SeleniumHQ/selenium/commit/facd199c31c9d81316a1db3bbb0c10603799bb8b

I will continue on Monday, so we hopefully have updated data mid next week.

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

The command to build all the exports should be the following now:

python buck-out/crazy-fun/be2bf932342e5d67f58c9b26f5065c745d285d0d/buck.pex build //javascript/webdriver/atoms:clear-element-firefox //javascript/webdriver/atoms:get-attribute-firefox //javascript/webdriver/atoms:get-text-firefox //javascript/webdriver/atoms:is-displayed-firefox //javascript/webdriver/atoms:is-enabled-firefox //javascript/webdriver/atoms:is-selected-firefox --show-output

I have updated all of our used atoms, and as it looks like only the accessibility failures will remain. I will do another try push, so that we can get more clarification.

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

Ok, so the accessibility test is broken because of the following reason:

>     # Elements that are either accessible to accessibility API or not accessible
>     # at all
>     falsy_elements = [
>         # Element is only visible to the accessibility API and may be
>         # manipulated by it
>         "button9",
>         # Element is not currently visible
>         "button10"
>     ]
> 
>     displayed_elementIDs = [
>         "button1", "button2", "button3", "button4", "button5", "button6",
>         "button9", "no_accessible_but_displayed"
>     ]

As you can see button 9 has been added to both the 'falsy_elements' and 'displayed_elementIDs'. Checking the style definitions of 'button9' we have:

> style="position:absolute;left:-100px;top:-455px;"

So it means this button has not the displayed=true state because it has been moved off-screen and is not visible to the user.

Having it as part of the displayed list is a bug, and as it looks like an update of the Selenium atom for `is_displayed` fixed that.

	Andreas Tolfsen ❲:ato❳
	Comment 39 • 7 years ago
mozreview-review

Comment on attachment 8913678 [details]
Bug 1375660 - Fix test_l10n.py for title text.

https://reviewboard.mozilla.org/r/185068/#review190950

	Andreas Tolfsen ❲:ato❳
	Comment 40 • 7 years ago
mozreview-review

Comment on attachment 8914524 [details]
Bug 1375660 - Remove duplicate button 9 reference in test_accessibility.

https://reviewboard.mozilla.org/r/185840/#review190952

	Andreas Tolfsen ❲:ato❳
	Comment 41 • 7 years ago
mozreview-review

Comment on attachment 8913679 [details]
Bug 1375660 - Remove Selenium atom: getElementAttribute.

https://reviewboard.mozilla.org/r/185070/#review190954

This atom is, as far as I can tell, used in one single place in the
Marionette code:

> interaction.isElementEnabled = function(el, strict = false) {
>   let enabled = true;
>   let win = getWindow(el);
> 
>   if (element.isXULElement(el)) {
>     // check if XUL element supports disabled attribute
>     if (DISABLED_ATTRIBUTE_SUPPORTED_XUL.has(el.tagName.toUpperCase())) {
>       let disabled = atom.getElementAttribute(el, "disabled", win);
>       if (disabled && disabled === "true") {
>         enabled = false;
>       }
>     }
>   } else {
>     enabled = atom.isElementEnabled(el, {frame: win});
>   }

[…]

In the above code we use it to get a XULElement’s "disabled"
attribute, which we could also get using el.getAttribute.  It
would be preferable to rely on as few Selenium atoms as possible.
As far as I know the only atom we eventually want to rely on is
the getElementText atom; the rest will have to be reimplemented
according to the specification to make Marionette spec conforming.

Instead of upgrading the getElementText atom, I think
it would be prudent to remove it altogether and fix
interaction.isElementEnabled.

	Andreas Tolfsen ❲:ato❳
	Comment 42 • 7 years ago
mozreview-review

Comment on attachment 8913680 [details]
Bug 1375660 - Update Selenium atom: getElementText.

https://reviewboard.mozilla.org/r/185072/#review190956

	Andreas Tolfsen ❲:ato❳
	Comment 43 • 7 years ago
mozreview-review

Comment on attachment 8913681 [details]
Bug 1375660 - Update Selenium atom: isElementDisplayed.

https://reviewboard.mozilla.org/r/185074/#review190958

	Andreas Tolfsen ❲:ato❳
	Comment 44 • 7 years ago
mozreview-review

Comment on attachment 8913682 [details]
Bug 1375660 - Update Selenium atom: isElementEnabled.

https://reviewboard.mozilla.org/r/185076/#review190960

At some point we are going to have to reimplement this too.

	Andreas Tolfsen ❲:ato❳
	Comment 45 • 7 years ago
mozreview-review

Comment on attachment 8913683 [details]
Bug 1375660 - Update Selenium atom: isElementSelected.

https://reviewboard.mozilla.org/r/185078/#review190962

I see we have a lot of work ahead of us.  Do you mind checking that
we have bug files against the “remove Selenium atoms” bug for
implementing spec-conforming versions of the commands using these
atoms?

	Andreas Tolfsen ❲:ato❳
	Comment 46 • 7 years ago
mozreview-review

Comment on attachment 8913684 [details]
Bug 1375660 - Update Selenium atom: clearElement.

https://reviewboard.mozilla.org/r/185080/#review190964

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 47 • 7 years ago
mozreview-review-reply

Comment on attachment 8913679 [details]
Bug 1375660 - Remove Selenium atom: getElementAttribute.

https://reviewboard.mozilla.org/r/185070/#review190954

That makes sense and is easy to fix. An updated patch will contain the removal of this atom.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 48 • 7 years ago
mozreview-review-reply

Comment on attachment 8913683 [details]
Bug 1375660 - Update Selenium atom: isElementSelected.

https://reviewboard.mozilla.org/r/185078/#review190962

Yes, there is bug 1354203 and it already tracks the removal of the individual Selenium atoms.

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

Andreas, I think that it would make sense to put the commands necessary to export the atoms in some readme. But not sure which to use. The readme.html under testing/marionette doesn't seem to be appropriate. Do you have any suggestions?

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

(In reply to Henrik Skupin (:whimboo) from comment #49)

> Andreas, I think that it would make sense to put the commands
> necessary to export the atoms in some readme. But not sure which
> to use. The readme.html under testing/marionette doesn't seem to
> be appropriate. Do you have any suggestions?

I will put the API documentation under testing/marionette/doc/api
in https://bugzil.la/1405757 so that you can put this in
testing/marionette/doc/ExportingAtoms.md or similar, like we have
done for testing/geckodriver/doc/Releasing.md:

	https://searchfox.org/mozilla-central/source/testing/geckodriver/doc/Releasing.md

	Andreas Tolfsen ❲:ato❳
	Comment 57 • 7 years ago
mozreview-review

Comment on attachment 8913679 [details]
Bug 1375660 - Remove Selenium atom: getElementAttribute.

https://reviewboard.mozilla.org/r/185070/#review191520

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

So I will wait for bug 1405757 being fixed on central.

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

(In reply to Henrik Skupin (:whimboo) from comment #58)

> So I will wait for bug 1405757 being fixed on central.

You can actually put the file in there right away.  The directory
isn’t going away.

	Andreas Tolfsen ❲:ato❳
	Comment 61 • 7 years ago
mozreview-review

Comment on attachment 8915539 [details]
Bug 1375660 - Add instructions for updating the Selenium atoms.

https://reviewboard.mozilla.org/r/186748/#review191810

	Pulsebot
	Comment 62 • 7 years ago

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6db2db8b29f4
Fix test_l10n.py for title text. r=ato
https://hg.mozilla.org/integration/autoland/rev/3c3867c8593f
Remove duplicate button 9 reference in test_accessibility. r=ato
https://hg.mozilla.org/integration/autoland/rev/8d29f94988a9
Remove Selenium atom: getElementAttribute. r=ato
https://hg.mozilla.org/integration/autoland/rev/988dca8eca1a
Update Selenium atom: getElementText. r=ato
https://hg.mozilla.org/integration/autoland/rev/fbcd3b841574
Update Selenium atom: isElementDisplayed. r=ato
https://hg.mozilla.org/integration/autoland/rev/64093a2fa94a
Update Selenium atom: isElementEnabled. r=ato
https://hg.mozilla.org/integration/autoland/rev/6a7091b50bbf
Update Selenium atom: isElementSelected. r=ato
https://hg.mozilla.org/integration/autoland/rev/8d47e6059cdf
Update Selenium atom: clearElement. r=ato
https://hg.mozilla.org/integration/autoland/rev/b63029f0a1c2
Add instructions for updating the Selenium atoms. r=ato

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 63 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/6db2db8b29f4
https://hg.mozilla.org/mozilla-central/rev/3c3867c8593f
https://hg.mozilla.org/mozilla-central/rev/8d29f94988a9
https://hg.mozilla.org/mozilla-central/rev/988dca8eca1a
https://hg.mozilla.org/mozilla-central/rev/fbcd3b841574
https://hg.mozilla.org/mozilla-central/rev/64093a2fa94a
https://hg.mozilla.org/mozilla-central/rev/6a7091b50bbf
https://hg.mozilla.org/mozilla-central/rev/8d47e6059cdf
https://hg.mozilla.org/mozilla-central/rev/b63029f0a1c2