Closed Bug 1587627 Opened 2 months ago Closed Last month

Remove `Anon` and `AnonAttribute` strategies from "WebDriver:FindElement" and "WebDriver:FindElements" command

Categories

(Testing :: Marionette, task, P1)

Version 3
task

Tracking

(firefox72 fixed)

RESOLVED FIXED
mozilla72
Tracking Status
firefox72 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files)

Today all the XBL bindings got removed. But we still have XBL support in-tree.

Once bug 1566221 has been fixed we should get started to remove the Anon and AnonAttribute strategies from the find element(s) commands.

Personally I would wait and also sync with the Thunderbird team. I don't know what's the status is for that project. Setting needinfo for Samuel and Geoff.

Flags: needinfo?(samuel.thibault)
Flags: needinfo?(geoff)

The thunderbird marionette part doesn't use them.

Flags: needinfo?(samuel.thibault)

Go ahead. Thanks for checking.

Flags: needinfo?(geoff)

Thanks for those quick replies. Lets also get some feedback from Brian.

Flags: needinfo?(bgrinstead)

Thanks for tracking this. This can for sure go once we don't ship MOZ_XBL anymore in Firefox (Bug 1583314). Also, for reference Thunderbird doesn't use XBL anymore either (https://mail.mozilla.org/pipermail/tb-planning/2019-September/006954.html).

I'm going to flip the relationship on Bug 1566221 to consider this as blocking the "remove XBL support" work (I should rename that bug to not be Gecko-specific, since we've morphed it to track i.e. frontend cleanups as well).

There are a couple of other text hits for "XBL" that are worth investigating too to see if there's any cleanup possible: https://searchfox.org/mozilla-central/search?q=xbl&path=testing%2Fmarionette.

Depends on: 1583314
No longer depends on: remove-xbl-implementation
Flags: needinfo?(bgrinstead)

So we already build with XBL disabled? See also bug 1587662 for some failing Marionette tests, which your query also refers to. As I have read we only want to build without XBL on Android for now, but I also see on MacOS.

If it is ok to just remove all the XBL specific tests for Marionette, and the anon support right now, I can go ahead and make the change.

Flags: needinfo?(bgrinstead)

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #5)

So we already build with XBL disabled? See also bug 1587662 for some failing Marionette tests, which your query also refers to. As I have read we only want to build without XBL on Android for now, but I also see on MacOS.

If it is ok to just remove all the XBL specific tests for Marionette, and the anon support right now, I can go ahead and make the change.

We don't build with XBL disabled in desktop yet (only in GeckoView), but for bug 1587662 I think we might be hitting some weirdness with artifact builds (see https://bugzilla.mozilla.org/show_bug.cgi?id=1510785#c21). That failure should probably be fixable by adding a skip-if = !xbl, but at this point I think it's fine to just remove them. We aren't going to re-add any bindings in desktop, and so we don't need to use Marionette to test them.

No longer depends on: 1583314
Flags: needinfo?(bgrinstead)
See Also: → 1587662

Ok, so lets wait until the artifact build issue has been fixed, so we can easily verify the fix. Afterward we can remove all the Anon code. Thanks.

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #7)

Ok, so lets wait until the artifact build issue has been fixed, so we can easily verify the fix. Afterward we can remove all the Anon code. Thanks.

This should be good to go now, the artifact build issue has been fixed.

See Also: → 1591145

Given that this blocks the final remove of xbl implementations including methods like GetAnonymousElementByAttribute, I'm going to do the removal now.

Assignee: nobody → hskupin
Status: NEW → ASSIGNED

I actually stumbled over a comment in driver.js which makes me wonder how to proceed. Brian, can you give some feedback?

https://searchfox.org/mozilla-central/rev/2a355e56c490768be676689de18629485c9d699b/testing/marionette/driver.js#1809-1810

 // Shadow nodes also show up in getAnonymousNodes, we should
 // ignore them.

How do I have to handle shadow nodes?

Flags: needinfo?(bgrinstead)

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #10)

I actually stumbled over a comment in driver.js which makes me wonder how to proceed. Brian, can you give some feedback?

https://searchfox.org/mozilla-central/rev/2a355e56c490768be676689de18629485c9d699b/testing/marionette/driver.js#1809-1810

 // Shadow nodes also show up in getAnonymousNodes, we should
 // ignore them.

How do I have to handle shadow nodes?

Based on https://searchfox.org/mozilla-central/rev/2a355e56c490768be676689de18629485c9d699b/dom/base/Document.cpp#7996 I think that comment is incorrect, but forwarding to bz to be sure.

Flags: needinfo?(bgrinstead) → needinfo?(bzbarsky)
Blocks: 1591145
See Also: 1591145

getAnonymousNodes in fact does not include shadow stuff.

However, it's not obvious to me that the code linked in comment 10 is using getAnonymousNodes in a way that would be relevant to that comment. It's got a wantedFrame, which is a node it got from ... somewhere? It does document.getBindingParent(wantedFrame), which for a shadow DOM element will in fact return the shadow host; see https://searchfox.org/mozilla-central/rev/7536d7f480a7f18c941a590a2d4c5119d9f52770/dom/base/ShadowRoot.cpp#68 which then propagates to all the shadow DOM bits.

So if wantedFrame is in a shadow tree, then parent will be non-null; this code wants to not run the getAnonymousNodes bit in that case, so checks for the shadowRoot bits.

Presumably the right update to not having XBL is to remove that whole block?

Flags: needinfo?(bzbarsky)

Probably yes. When checking for the addition of this code I ended-up with bug 1237698 and specifically: https://hg.mozilla.org/mozilla-central/rev/ca74b06769ce. Best is to just revert those changes, which is in accordance what Boris proposed.

Priority: -- → P1

The patch removes the usage of elements based on anonymouse nodes,
which are soon to be completely removed. Also code which is no longer
in use by Firefox-UI tests can simply be removed.

Attachment #9104630 - Attachment description: Bug 1587627 - [fxui] Update firefox-puppeteer for removal of anonymouse nodes. r=#webdriver! → Bug 1587627 - [fxui] Update firefox-puppeteer for removal of anonymous nodes. r=#webdriver!
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3ab59c714f29
[fxui] Update firefox-puppeteer for removal of anonymous nodes. r=webdriver-reviewers,maja_zf
https://hg.mozilla.org/integration/autoland/rev/79be8382d57a
[marionette] Remove `Anon` and `AnonAttribute` strategies from "WebDriver:FindElement" and "WebDriver:FindElements" command. r=webdriver-reviewers,maja_zf
Regressions: 1592220

Can we please backout the second patch (https://hg.mozilla.org/integration/autoland/rev/79be8382d57a) given that it causes failures in browser-chrome tests. Thanks.

Flags: needinfo?(sheriffs)
Flags: needinfo?(sheriffs) → needinfo?(hskupin)

The problem here is that test.xul is no longer referenced since it was only referenced in test_anonymous_content.xul. Also the other two referenced files are part of test.xul, and as such three files are not referenced. The short-term fix will be to keep a reference to test.xul in browser_all_files_referenced.js.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1dea418bcf51fbe48925e716b2562878a84e0e5

Flags: needinfo?(hskupin)
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f77279b36116
[marionette] Remove `Anon` and `AnonAttribute` strategies from "WebDriver:FindElement" and "WebDriver:FindElements" command. r=webdriver-reviewers,maja_zf
Status: ASSIGNED → RESOLVED
Closed: Last month
Resolution: --- → FIXED
Target Milestone: --- → mozilla72
You need to log in before you can comment on or make changes to this bug.