Closed
Bug 1134872
Opened 9 years ago
Closed 9 years ago
getElementValueOfCssProperty delegates to the content listener, even in chrome scope
Categories
(Remote Protocol :: Marionette, defect)
Remote Protocol
Marionette
Tracking
(firefox38 fixed, firefox39 fixed)
RESOLVED
FIXED
mozilla39
People
(Reporter: chmanchester, Assigned: galgeek)
Details
(Keywords: pi-marionette-server)
Attachments
(1 file, 3 obsolete files)
4.14 KB,
patch
|
galgeek
:
review+
|
Details | Diff | Splinter Review |
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
Updated•9 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Assignee | ||
Comment 1•9 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 2•9 years ago
|
||
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•9 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•9 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•9 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)
Updated•9 years ago
|
Keywords: ateam-marionette-server
Updated•9 years ago
|
Attachment #8573629 -
Flags: review?(dburns) → review+
Reporter | ||
Comment 6•9 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=92b16a065671
Assignee | ||
Comment 7•9 years ago
|
||
Fixing commit message and a pyflakes notice.
Attachment #8573629 -
Attachment is obsolete: true
Attachment #8574153 -
Flags: review+
Reporter | ||
Comment 8•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/efa7c6cf88fc
Comment 9•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/efa7c6cf88fc
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Comment 10•9 years ago
|
||
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.
status-firefox38:
--- → affected
Comment 11•9 years ago
|
||
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)
Comment 12•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/13819479b7ab
Flags: in-testsuite+
Updated•9 years ago
|
Flags: needinfo?(dburns)
Updated•1 year ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•