1134872 - getElementValueOfCssProperty delegates to the content listener, even in chrome scope

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Chris Manchester (limited bugmail, email directly)]

getElementValueOfCssProperty always goes to content [1], making this endpoint unusable for browser ui elements.

[1] https://hg.mozilla.org/mozilla-central/annotate/986e840a2979/testing/marionette/marionette-server.js#l2159

Comment 1

[Barbara Miller (:galgeek)]

Attachment: 0001-Bug-1134872-getElementValueOfCssProperty-delegates-t.patch

Here's the update to getElementValueOfCssProperty and a quick attempt at a unit test to go with it.

I'm just looking for quick feedback right now. (I haven't managed to get this unit test file to run, though I can run other tests in the unit directory.) I copied the test code into test_rendered_element.py, where it raises a Marionette Exception, "curFrame is not defined" on this line: "lockImage = element.value_of_css_property("list-style-image")."  

Switching curFrame to this.curFrame raises a Marionette Exception, too, "this.curFrame is null" on the same line.

Comment 2

[David Burns :automatedtester]

Comment on attachment 8573090 [details] [diff] [review]
0001-Bug-1134872-getElementValueOfCssProperty-delegates-t.patch

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

Make sure to add your test file name to https://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/tests/unit/unit-tests.ini and have b2g = false for it

::: testing/marionette/client/marionette/tests/unit/test_chrome_element_css.py
@@ +17,5 @@
> +
> +
> +class ChromeElementCSSTest(MarionetteTestCase):
> +
> +    def testWeCanGetComputedStyleValueOnChromeElement(self):

please separate words with _ instead of mixedcased function names (I know I did bad by introducing them in the first place and we need to sort that, sorry)

::: testing/marionette/marionette-server.js
@@ +2176,5 @@
> +    if (this.context == "chrome") {
> +      try {
> +        let el = this.curBrowser.elementManager.getKnownElement(
> +            aRequest.parameters.id, this.getCurrentWindow());
> +            this.sendResponse(curFrame.document.defaultView

s/curframe/this.curBrowser/

Comment 3

[Barbara Miller (:galgeek)]

Attachment: 0001-Bug-1134872-getElementValueOfCssProperty-delegates-t.patch

Thanks for your feedback, David!

I'm requesting feedback again, but in the meantime, this code runs fine for me.

Comment 4

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8573471 [details] [diff] [review]
0001-Bug-1134872-getElementValueOfCssProperty-delegates-t.patch

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

Looking good. A few nits below.

::: testing/marionette/client/marionette/tests/unit/test_chrome_element_css.py
@@ +10,5 @@
> +#Unless required by applicable law or agreed to in writing, software
> +#distributed under the License is distributed on an "AS IS" BASIS,
> +#WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> +#See the License for the specific language governing permissions and
> +#limitations under the License.

I think this should be the Mozilla license.

@@ +15,5 @@
> +
> +from marionette import MarionetteTestCase
> +
> +
> +class ChromeElementCSSTests(MarionetteTestCase):

The convention seems to be for test classes to start with "Test".

@@ +18,5 @@
> +
> +class ChromeElementCSSTests(MarionetteTestCase):
> +
> +    def test_we_can_get_css_value_on_chrome_element(self):
> +        test_url = "https://mozqa.com/data/firefox/security/mixedcontent.html"

We don't usually allow hitting the network on buildbot based tests, can you find another element to test?

@@ +23,5 @@
> +        self.marionette.navigate(test_url)
> +
> +        with self.marionette.using_context("chrome"):
> +            element = self.marionette.find_element("id", "page-proxy-favicon")
> +            faviconImage = element.value_of_css_property("list-style-image")

s/faviconImage/favicon_image/

@@ +34,5 @@
> +        with self.marionette.using_context("chrome"):
> +            identity_box = self.marionette.find_element("id", "identity-box")
> +            identity_box.click()
> +            element = self.marionette.find_element("id", "identity-popup-encryption-icon")
> +            lockImage = element.value_of_css_property("list-style-image")

s/lockImage/lock_image/

::: testing/marionette/marionette-server.js
@@ +2176,5 @@
> +    if (this.context == "chrome") {
> +      try {
> +        let el = this.curBrowser.elementManager.getKnownElement(
> +            aRequest.parameters.id, this.getCurrentWindow());
> +            this.sendResponse(this.getCurrentWindow().document.defaultView.getComputedStyle(

You could use let curWin = this.getCurrentWindow(); at the beginning of this block for the two uses and to keep line length down a bit.

Comment 5

[Barbara Miller (:galgeek)]

Attachment: 0001-Bug-1134872-getElementValueOfCssProperty-delegates-t.patch

Thank you again, David and Chris, for the help and feedback!

I think this is about ready for review.

I've successfully tested the updated unit test against my local build.

Comment 6

[Chris Manchester (limited bugmail, email directly)]

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=92b16a065671

Comment 7

[Barbara Miller (:galgeek)]

Attachment: 0001-Bug-1134872-Fixing-getElementValueOfCssProperty-to-s.patch

Fixing commit message and a pyflakes notice.

Comment 8

[Chris Manchester (limited bugmail, email directly)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/efa7c6cf88fc

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/efa7c6cf88fc

Comment 10

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

We want to make sure that we can run our Firefox UI tests with Firefox 38. So this patch we would also like to backport.

Comment 11

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

David, I was assuming that you are using the bugzilla query for backporting patches to Firefox 38. I assume you were relying on my needinfo flags instead. So this bug also warrants a backport.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/13819479b7ab