Closed Bug 1272653 Opened 8 years ago Closed 8 years ago

Implement spec compatible Get Element Attribute and Get Element Property commands

Categories

(Remote Protocol :: Marionette, defect)

Version 3
defect
Not set
normal

Tracking

(firefox48 fixed, firefox49 fixed)

RESOLVED FIXED
mozilla49
Tracking Status
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: ato, Assigned: ato)

References

(Blocks 1 open bug)

Details

(Keywords: pi-marionette-server)

Attachments

(5 files)

The current function GeckoDriver#getElementAttribute uses the Selenium atom that conflates attributes and properties.  According to the WebDriver specification we should not conflate them any more.

Get Element Attribute should get an element’s attribute, and not prioritise its property if there exists one with the same name:

  http://w3c.github.io/webdriver/webdriver-spec.html#dfn-get-element-attribute

Get Element Property should get an element’s property, but not the attribute:

  http://w3c.github.io/webdriver/webdriver-spec.html#dfn-get-element-property
Assignee: nobody → ato
Status: NEW → ASSIGNED
Blocks: 1260233
Blocks: webdriver
Renames test_elementState.py to test_element_state.py to patch
Python naming conventions.  Merges test_getattr*.py tests into
test_element_state.py to match WebDriver specification chapter structure.

Review commit: https://reviewboard.mozilla.org/r/52463/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52463/
Attachment #8752196 - Flags: review?(jgriffin)
Attachment #8752197 - Flags: review?(jgriffin)
Attachment #8752198 - Flags: review?(jgriffin)
Comment on attachment 8752196 [details]
MozReview Request: Bug 1272653 - Merge element state tests; r?jgriffin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52463/diff/1-2/
Attachment #8752196 - Attachment description: MozReview Request: Bug 1272653 - Restructure element state tests; r?jgriffin → MozReview Request: Bug 1272653 - Merge element state tests; r?jgriffin
Comment on attachment 8752197 [details]
MozReview Request: Bug 1272653 - Restructure element state test cases; r?jgriffin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52465/diff/1-2/
Comment on attachment 8752198 [details]
MozReview Request: Bug 1272653 - Implement WebDriver command Get Element Property; r?jgriffin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52467/diff/1-2/
The first three patches restructures the existing element state tests a bit and adds the Get Element Property command.

I plan to adjust getElementAttribute to match the realities of the spec in an upcoming patch later tonight or quite possibly next week.  This may require some backwards compatibility work because existing tests may rely on the Selenium idea of what an attribute is.

It looks like this work will mean we can get ride of the atom.getElementAttribute atom from Selenium, which is good news.
Comment on attachment 8752196 [details]
MozReview Request: Bug 1272653 - Merge element state tests; r?jgriffin

https://reviewboard.mozilla.org/r/52463/#review49499
Attachment #8752196 - Flags: review?(jgriffin) → review+
Attachment #8752197 - Flags: review?(jgriffin) → review+
Comment on attachment 8752197 [details]
MozReview Request: Bug 1272653 - Restructure element state test cases; r?jgriffin

https://reviewboard.mozilla.org/r/52465/#review49503

::: testing/marionette/harness/marionette/tests/unit/test_element_state_chrome.py:24
(Diff revision 2)
>          self.assertNotEqual(self.win, self.marionette.current_window_handle)
>          self.marionette.execute_script("window.close();")
>          self.marionette.switch_to_window(self.win)
>          MarionetteTestCase.tearDown(self)
>  
> -    def test_isEnabled(self):
> +    def test_enabeld(self):

s/enabeld/enabled/
Comment on attachment 8752198 [details]
MozReview Request: Bug 1272653 - Implement WebDriver command Get Element Property; r?jgriffin

https://reviewboard.mozilla.org/r/52467/#review49505

lgtm!
Attachment #8752198 - Flags: review?(jgriffin) → review+
Comment on attachment 8752196 [details]
MozReview Request: Bug 1272653 - Merge element state tests; r?jgriffin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52463/diff/2-3/
Comment on attachment 8752197 [details]
MozReview Request: Bug 1272653 - Restructure element state test cases; r?jgriffin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52465/diff/2-3/
Comment on attachment 8752198 [details]
MozReview Request: Bug 1272653 - Implement WebDriver command Get Element Property; r?jgriffin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52467/diff/2-3/
Comment on attachment 8753034 [details]
MozReview Request: Bug 1272653 - Make Get Element Attribute spec compatible; r?automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52950/diff/1-2/
There are a lot of test failures.  I am assuming many of these stem from the fact that the last and final patch changes Get Element Attribute behaviour that existing Gecko tests rely on.

I will investigate this further on Wednesday.  (I have tomorrow off.)
Comment on attachment 8753034 [details]
MozReview Request: Bug 1272653 - Make Get Element Attribute spec compatible; r?automatedtester

https://reviewboard.mozilla.org/r/52950/#review50122
Attachment #8753034 - Flags: review?(dburns) → review+
Comment on attachment 8752196 [details]
MozReview Request: Bug 1272653 - Merge element state tests; r?jgriffin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52463/diff/3-4/
Comment on attachment 8752197 [details]
MozReview Request: Bug 1272653 - Restructure element state test cases; r?jgriffin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52465/diff/3-4/
Comment on attachment 8752198 [details]
MozReview Request: Bug 1272653 - Implement WebDriver command Get Element Property; r?jgriffin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52467/diff/3-4/
Comment on attachment 8753034 [details]
MozReview Request: Bug 1272653 - Make Get Element Attribute spec compatible; r?automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52950/diff/2-3/
Comment on attachment 8755391 [details]
MozReview Request: Bug 1272653 - Update tests to get property where appropriate; r?automatedtester

https://reviewboard.mozilla.org/r/54588/#review51252
Attachment #8755391 - Flags: review?(dburns) → review+
Comment on attachment 8752196 [details]
MozReview Request: Bug 1272653 - Merge element state tests; r?jgriffin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52463/diff/4-5/
Comment on attachment 8752197 [details]
MozReview Request: Bug 1272653 - Restructure element state test cases; r?jgriffin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52465/diff/4-5/
Comment on attachment 8752198 [details]
MozReview Request: Bug 1272653 - Implement WebDriver command Get Element Property; r?jgriffin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52467/diff/4-5/
Comment on attachment 8753034 [details]
MozReview Request: Bug 1272653 - Make Get Element Attribute spec compatible; r?automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52950/diff/3-4/
Comment on attachment 8755391 [details]
MozReview Request: Bug 1272653 - Update tests to get property where appropriate; r?automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54588/diff/1-2/
The landing of this patch perma broke our firefox-ui-tests! See bug 1275249 for details.
Depends on: 1276182
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.