Closed Bug 1277090 Opened 8 years ago Closed 7 years ago

Have chrome getElementAttribute only return attributes and not conflate

Categories

(Remote Protocol :: Marionette, defect)

Product:
Remote Protocol
Component:
Marionette
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox51 wontfix, firefox52 fixed, firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox51 --- wontfix
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: automatedtester, Assigned: whimboo)

References

(Blocks 1 open bug)

Details

(Whiteboard: [spec][17/01])

Attachments

(3 files, 2 obsolete files)

Bug 1277090 - getElementAttribute() has to only return attributes.
58 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1277090 - Update unit tests for getElementAttribute() changes.
58 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1277090 - Make use of get_property() in firefox-ui tests where necessary.
58 bytes, text/x-review-board-request
impossibus
: review+
Details 
just like we do when speaking to content
Depends on: 1277083
Using the Selenium atom we are conflating properties and attributes which is not
thing we really want to be doing.

Review commit: https://reviewboard.mozilla.org/r/56726/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56726/
Attachment #8758480 - Flags: review?(ato)
Attachment #8758481 - Flags: review?(ato)
Due to the conflation, we were returning the wrong thing on get_attribute,
Updated tests to call get_property when that is what they meant.

Review commit: https://reviewboard.mozilla.org/r/56728/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56728/
It would be great if we can get this tested with the Firefox UI tests and especially update tests before its getting landed. It will certainly break lots of tests.
(In reply to Henrik Skupin (:whimboo) from comment #3)
> It would be great if we can get this tested with the Firefox UI tests and
> especially update tests before its getting landed. It will certainly break
> lots of tests.

I can't see how to run this on try, could you give me the necessary try syntax for update tests?
Flags: needinfo?(hskupin)
Update tests cannot be run on try given that updates only work for nightly and release builds but not inbound. So two steps have to be done:

* You will have to do it manually locally via `mach firefox-ui-update --binary %path to second last nightly build%.

* Given that you do server changes you can trigger the firefox-ui-functional for linux64 debug on try and check the puppeteer unittest results. Those cover a good portion of the ui modules and give you results for a Firefox build containing your changes.

Finally both test methods from above have to work given that our tests have to handle both APIs. If there are failures we most likely would need a wrapper in puppeteer which detects the correct method to be used, given that you don't want a backward compatibility.
Flags: needinfo?(hskupin)
Comment on attachment 8758480 [details]
MozReview Request: Bug 1277090: Have Marionette return only attributes from getElementAttribute. r?ato

https://reviewboard.mozilla.org/r/56726/#review53708

::: testing/marionette/driver.js:1789
(Diff revision 1)
> -      resp.body.value = atom.getElementAttribute(el, name, this.getCurrentWindow());
> +
> +      if (element.isBooleanAttribute(el, name)) {
> +        if (el.hasAttribute(name)) {
> +          resp.body.value = "true";
> +        } else {
> +          resp.body.value = null;
> +        }
> +      } else {
> +        resp.body.value = el.getAttribute(name);
> +      }

What is the purpose of changing this to do the same we do in content?

Does XUL/XBL/chrome have boolean attributes?  I suspect boolean attributes are only a thing in HTML doctypes.
Attachment #8758480 - Flags: review?(ato)
https://reviewboard.mozilla.org/r/56726/#review53708

> What is the purpose of changing this to do the same we do in content?
> 
> Does XUL/XBL/chrome have boolean attributes?  I suspect boolean attributes are only a thing in HTML doctypes.

While we might not have it now, if we move over to browser.html for the front end or react front end like being tested in Tofino we will have those types of elements there.
https://reviewboard.mozilla.org/r/56726/#review53708

> While we might not have it now, if we move over to browser.html for the front end or react front end like being tested in Tofino we will have those types of elements there.

This sounds positively circumstancial.  I imagine if we do that, significant portions of Marionette will have to be rewritten to make that possible.
https://reviewboard.mozilla.org/r/56726/#review53708

> This sounds positively circumstancial.  I imagine if we do that, significant portions of Marionette will have to be rewritten to make that possible.

So you are r- because you want Chrome to act differently to content? That is a really silly reason.
(In reply to David Burns :automatedtester from comment #9)
> https://reviewboard.mozilla.org/r/56726/#review53708
> 
> > This sounds positively circumstancial.  I imagine if we do that, significant portions of Marionette will have to be rewritten to make that possible.
> 
> So you are r- because you want Chrome to act differently to content? That is
> a really silly reason.

More to the point, it is extremely poor API design. We should either throw operation not supported, as was there previously or we should have exactly the same behaviour.
Comment on attachment 8758480 [details]
MozReview Request: Bug 1277090: Have Marionette return only attributes from getElementAttribute. r?ato

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56726/diff/1-2/
Attachment #8758480 - Flags: review?(ato)
Comment on attachment 8758481 [details]
MozReview Request: Bug 1277090: Update tests to get properties instead of attributes r?ato

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56728/diff/1-2/
https://reviewboard.mozilla.org/r/56726/#review53708

> So you are r- because you want Chrome to act differently to content? That is a really silly reason.

No, I support the intention of this patch to move us off the Selenium atom.

What I’m trying to understand is if XUL elements have boolean attributes and if all the branches in this if-condition are valid in chrome.

For example if <input> elements on XUL windows are real HTML elements where the `disabled` attribute is a boolean attribute, this change makes sense.  If they don’t, this patch is adding a code path that will never be executed.

The other branches in the if-condition look fine to me.
https://reviewboard.mozilla.org/r/56728/#review55362

::: testing/marionette/harness/marionette/tests/unit/test_mouse_action.py:53
(Diff revision 2)
>          click_el = self.marionette.find_element(By.ID, "resultContainer")
>  
>          def context_menu_state():
>              with self.marionette.using_context("chrome"):
>                  cm_el = self.marionette.find_element(By.ID, "contentAreaContextMenu")
> -                return cm_el.get_attribute("state")
> +                return cm_el.get_property("state")

So as I understand it, previously `get_attribute` called down to the Selenium atom that conflated attributes and properties.  Does this mean this is actually a property or is it in attribute?

I know very little about XUL.  Perhaps :whimboo would know.
Flags: needinfo?(hskupin)
(In reply to Andreas Tolfsen ‹:ato› from comment #14)
> https://reviewboard.mozilla.org/r/56728/#review55362
> 
> ::: testing/marionette/harness/marionette/tests/unit/test_mouse_action.py:53
> (Diff revision 2)
> >          click_el = self.marionette.find_element(By.ID, "resultContainer")
> >  
> >          def context_menu_state():
> >              with self.marionette.using_context("chrome"):
> >                  cm_el = self.marionette.find_element(By.ID, "contentAreaContextMenu")
> > -                return cm_el.get_attribute("state")
> > +                return cm_el.get_property("state")
> 
> So as I understand it, previously `get_attribute` called down to the
> Selenium atom that conflated attributes and properties.  Does this mean this
> is actually a property or is it in attribute?
> 
> I know very little about XUL.  Perhaps :whimboo would know.

This means that it will return the property. The patch returns the same as the current approach
Flags: needinfo?(hskupin)
(In reply to David Burns :automatedtester from comment #15)
> (In reply to Andreas Tolfsen ‹:ato› from comment #14)
> > https://reviewboard.mozilla.org/r/56728/#review55362
> > 
> > ::: testing/marionette/harness/marionette/tests/unit/test_mouse_action.py:53
> > (Diff revision 2)
> > >          click_el = self.marionette.find_element(By.ID, "resultContainer")
> > >  
> > >          def context_menu_state():
> > >              with self.marionette.using_context("chrome"):
> > >                  cm_el = self.marionette.find_element(By.ID, "contentAreaContextMenu")
> > > -                return cm_el.get_attribute("state")
> > > +                return cm_el.get_property("state")
> > 
> > So as I understand it, previously `get_attribute` called down to the
> > Selenium atom that conflated attributes and properties.  Does this mean this
> > is actually a property or is it in attribute?
> > 
> > I know very little about XUL.  Perhaps :whimboo would know.
> 
> This means that it will return the property. The patch returns the same as
> the current approach

Okay, so in conclusion there exists a concept of properties in XUL elements, and these are discrete from attributes.
Status: NEW → ASSIGNED
Comment on attachment 8758480 [details]
MozReview Request: Bug 1277090: Have Marionette return only attributes from getElementAttribute. r?ato

https://reviewboard.mozilla.org/r/56726/#review56614

Fix issue and carry forward.
Attachment #8758480 - Flags: review?(ato) → review+
Comment on attachment 8758481 [details]
MozReview Request: Bug 1277090: Update tests to get properties instead of attributes r?ato

https://reviewboard.mozilla.org/r/56728/#review56616
Attachment #8758481 - Flags: review?(ato) → review+
Pushed by dburns@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7be72cdc9d78
Have Marionette return only attributes from getElementAttribute. r=ato
https://hg.mozilla.org/integration/mozilla-inbound/rev/4517cfdd7f79
Update tests to get properties instead of attributes r=ato
The landing of this patch totally broke our Firefox UI tests:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-searchStr=Firefox%20UI&filter-tier=2&filter-tier=3&selectedJob=30310056

As mentioned very early on that bug changes to our tests would also have to be made, and with a special focus on update tests. As checked also the try build showed those failures but looks like results haven't been checked before the landing. :/
Depends on: 1280879
You pushed code without the modifications I requested: The element.isBooleanAttribute path is a dead code path in chrome.
Flags: needinfo?(dburns)
Just to add, it would have been nice to have the old behavior still be around but marked as deprecated (until the next ESR release) and not completely removed. That way fixing our fx-ui-update tests would be much easier.
(In reply to Henrik Skupin (:whimboo) from comment #22)
> Just to add, it would have been nice to have the old behavior still be
> around but marked as deprecated (until the next ESR release) and not
> completely removed. That way fixing our fx-ui-update tests would be much
> easier.

I don’t think the old behaviour of get_attribute is desirable, but seen in the context of backwards compatibility for the fx-ui-update tests, it might make sense to keep around a shim emulating the old behaviour until all the relevant fixes have ridden the trains:

    def get_conflated_attribute(name):
        rv = marionette.get_property(name)
        if rv is not None:
            rv = marionette.get_attribute(name)
        return rv
It wouldn't help to have such a method in Marionette unless its getting backported to all other branches, which might not be what we want. As mentioned on bug 1280879 we might need an additional layer in fx-ui-tests instead.
(In reply to Henrik Skupin (:whimboo) from comment #24)
> It wouldn't help to have such a method in Marionette unless its getting
> backported to all other branches, which might not be what we want. As
> mentioned on bug 1280879 we might need an additional layer in fx-ui-tests
> instead.

I would suggest putting such a function in the tests and have each test currently using get_attribute call get_confalted_attribute instead.
(In reply to Andreas Tolfsen ‹:ato› from comment #21)
> You pushed code without the modifications I requested: The
> element.isBooleanAttribute path is a dead code path in chrome.

boolean attributes exist in Chrome e.g. https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/disabled
Flags: needinfo?(dburns)
https://hg.mozilla.org/mozilla-central/rev/7be72cdc9d78
https://hg.mozilla.org/mozilla-central/rev/4517cfdd7f79
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
(In reply to David Burns :automatedtester from comment #26)
> (In reply to Andreas Tolfsen ‹:ato› from comment #21)
> > You pushed code without the modifications I requested: The
> > element.isBooleanAttribute path is a dead code path in chrome.
> 
> boolean attributes exist in Chrome e.g.
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/disabled

No they don’t.  If you set <xul:button disabled="false">, the button becomes enabled.  For _boolean attributes_ in HTML, it doesn’t matter what the value is.

There is also a test in element.isBooleanAttribute that returns early if the document does not have the correct namespace, which means the if condition in your patch has no chance of ever running:

    https://github.com/mozilla/gecko-dev/blob/master/testing/marionette/element.js#L982
Flags: needinfo?(dburns)
Backed out in https://hg.mozilla.org/mozilla-central/rev/7fb69043ac05
Status: RESOLVED → REOPENED
status-firefox50: fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla50 → ---
I think this (at least in part) superseded by bug 1275273.
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Flags: needinfo?(dburns)
Resolution: --- → WONTFIX
No, this is not the case. As of now we still have to support Firefox 49 and 45ESR for update tests. Removing the get_attribute() method on central now will cause breakage in our tests. It's similar to what we have already seen on bug 1280879.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
No one has suggested removing the getAttribute command.
(In reply to Andreas Tolfsen ‹:ato› from comment #32)
> No one has suggested removing the getAttribute command.

So may you can give a bit more descriptive explanation why you think this bug is a wontfix and in some parts superseeded by bug 1275273?
Agreed with David I will take this finally over. We should really get it landed for Firefox 52, and keep it around until Firefox 45 is no longer supported. Means with Firefox 55 we can remove the conflating.
Assignee: dburns → hskupin
Status: REOPENED → ASSIGNED
status-firefox52: --- → affected
status-firefox53: --- → affected
(In reply to Andreas Tolfsen ‹:ato› from comment #6)
> Does XUL/XBL/chrome have boolean attributes?  I suspect boolean attributes
> are only a thing in HTML doctypes.

Just to answer this question... I'm not aware of anything in XUL that lists support for boolean attributes. So I think that we can perfectly ignore this type.
Comment on attachment 8758481 [details]
MozReview Request: Bug 1277090: Update tests to get properties instead of attributes r?ato

https://reviewboard.mozilla.org/r/56728/#review98360

::: testing/marionette/harness/marionette/tests/unit/test_mouse_action.py:53
(Diff revision 2)
>          click_el = self.marionette.find_element(By.ID, "resultContainer")
>  
>          def context_menu_state():
>              with self.marionette.using_context("chrome"):
>                  cm_el = self.marionette.find_element(By.ID, "contentAreaContextMenu")
> -                return cm_el.get_attribute("state")
> +                return cm_el.get_property("state")

Yes, this is a property and not an attribute on the context menu.
No longer blocks: 1277065
Blocks: 1323048
Attachment #8758480 - Attachment is obsolete: true
Attachment #8758481 - Attachment is obsolete: true
Comment on attachment 8818083 [details]
Bug 1277090 - Update unit tests for getElementAttribute() changes.

https://reviewboard.mozilla.org/r/98180/#review98862
Attachment #8818083 - Flags: review?(ato) → review+
Comment on attachment 8818082 [details]
Bug 1277090 - getElementAttribute() has to only return attributes.

https://reviewboard.mozilla.org/r/98178/#review98864

This looks fine, but the Firefox UI tests are missing from the try run.  Can you do one to make sure they aren’t affected?

::: testing/marionette/client/marionette_driver/marionette.py:75
(Diff revision 2)
> +        except errors.UnknownCommandException:
> +            # remove when 55 is stable
> +            return self.get_attribute(name)

Can you please document this code path?
Attachment #8818082 - Flags: review?(ato) → review+
Comment on attachment 8818082 [details]
Bug 1277090 - getElementAttribute() has to only return attributes.

https://reviewboard.mozilla.org/r/98178/#review98866

Also, can you please improve the commit message for the first patch?  I don’t think the first line is accurate enough and I’d like to see some more context.
(In reply to Henrik Skupin (:whimboo) from comment #33)
> (In reply to Andreas Tolfsen ‹:ato› from comment #32)
> > No one has suggested removing the getAttribute command.
> 
> So may you can give a bit more descriptive explanation why you think this
> bug is a wontfix and in some parts superseeded by bug 1275273?

It’s probably not a WONTFIX.  Bug 1275273 attempts to address cases where attributes and properties are conflated in the UI tests, but not in a backwards compatible way.  This is why I haven’t finished it.

It probably makes more sense to land these patches first, and then return to finish bug 1275273.
(In reply to Andreas Tolfsen ‹:ato› from comment #44)
> It probably makes more sense to land these patches first, and then return to
> finish bug 1275273.

I totally agree on that.
Pushed by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/837d6a698a1b
getElementAttribute() has to only return attributes. r=ato
https://hg.mozilla.org/integration/autoland/rev/46499bc17178
Update unit tests for getElementAttribute() changes. r=ato
Sorry had to backout this for tc-Fxfn-l(en-US), e.g., https://treeherder.mozilla.org/logviewer.html#?job_id=7914360&repo=autoland#L7561
Flags: needinfo?(hskupin)
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5e9f3113e1e7
Backed out changeset 46499bc17178 for tc-Fxfn-l(en-US)
https://hg.mozilla.org/integration/autoland/rev/337fb0c52354
Backed out changeset 837d6a698a1b
(In reply to Andreas Tolfsen ‹:ato› from comment #42)
> This looks fine, but the Firefox UI tests are missing from the try run.  Can
> you do one to make sure they aren’t affected?

And that comment I totally missed and as result we completely failed. :(

Issue here is that I perfectly tested it with older builds of Firefox where this method is not implemented, but actually missed builds of the same branch. In such a case there is no UnknownCommandException, and the fallback is not called.

It means that the code changes which I wanted to do on bug 1323048 we have to do here as well, and that as the first commit.

Sorry for that!
Flags: needinfo?(hskupin)
Blocks: 1316984
No longer blocks: 1316984
Depends on: 1316984
To not have get into conflicts with get_property() calls for the current l10n Puppeteer module, I would like to get bug 1316984 solved first before I continue here.
Comment on attachment 8820267 [details]
Bug 1277090 - Make use of get_property() in firefox-ui tests where necessary.

https://reviewboard.mozilla.org/r/98176/#review100254

Try showed a single failure for test_ssl_status_after_restart.py which I missed to update because it is skipped in e10s mode. A follow-up for the first commit has been uploaded which fixes it.
Attachment #8820267 - Flags: review?(mjzffr)
Comment on attachment 8820267 [details]
Bug 1277090 - Make use of get_property() in firefox-ui tests where necessary.

https://reviewboard.mozilla.org/r/99800/#review100266
Attachment #8820267 - Flags: review?(mjzffr) → review+
Comment on attachment 8820267 [details]
Bug 1277090 - Make use of get_property() in firefox-ui tests where necessary.

https://reviewboard.mozilla.org/r/99800/#review100300

Landing this patch failed due to a merge conflict. I will have to wait for the next merge to m-c before I will try it again.
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a82d159dd357
Make use of get_property() in firefox-ui tests where necessary. r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/30491b409e10
getElementAttribute() has to only return attributes. r=ato
https://hg.mozilla.org/integration/autoland/rev/16e579588613
Update unit tests for getElementAttribute() changes. r=ato
Depends on: 1326236
When uplifting this test-only patch to aurora please make sure to also uplift the patch on bug 1326236.
Whiteboard: [checkin-needed-aurora][uplift together with patch on bug 1326236]
Depends on: 1329263
Whiteboard: [spec][17/01]
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: