getElementValueOfCssProperty delegates to the content listener, even in chrome scope

RESOLVED FIXED in Firefox 38

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: chmanchester, Assigned: galgeek)

Tracking

({pi-marionette-server})

unspecified
mozilla39
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox38 fixed, firefox39 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 1

4 years ago
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.
Attachment #8573090 - Flags: feedback?(cmanchester)
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/
Attachment #8573090 - Flags: feedback?(cmanchester) → feedback+
(Assignee)

Comment 3

4 years ago
Thanks for your feedback, David!

I'm requesting feedback again, but in the meantime, this code runs fine for me.
Assignee: nobody → galgeek
Attachment #8573090 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8573471 - Flags: feedback?(cmanchester)
(Reporter)

Comment 4

4 years ago
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.
Attachment #8573471 - Flags: feedback?(cmanchester) → feedback+
(Assignee)

Comment 5

4 years ago
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.
Attachment #8573471 - Attachment is obsolete: true
Attachment #8573629 - Flags: review?(dburns)
Attachment #8573629 - Flags: review?(dburns) → review+
(Assignee)

Comment 7

4 years ago
Fixing commit message and a pyflakes notice.
Attachment #8573629 - Attachment is obsolete: true
Attachment #8574153 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/efa7c6cf88fc
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
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.
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.
Flags: needinfo?(dburns)
Flags: needinfo?(dburns)
You need to log in before you can comment on or make changes to this bug.