Closed Bug 1794099 Opened 2 years ago Closed 2 years ago

"WebDriver:GetElementText" started to fail with `TypeError: c is undefined`

Categories

(Remote Protocol :: Marionette, defect, P1)

Firefox 105
defect
Points:
1

Tracking

(firefox-esr102 unaffected, firefox105 wontfix, firefox106 wontfix, firefox107 wontfix, firefox108 fixed)

RESOLVED FIXED
108 Branch
Tracking Status
firefox-esr102 --- unaffected
firefox105 --- wontfix
firefox106 --- wontfix
firefox107 --- wontfix
firefox108 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

(Regression)

Details

(Keywords: regression, Whiteboard: [webdriver:m5][webdriver:relnote])

Attachments

(2 files)

Originally filed as: https://github.com/mozilla/geckodriver/issues/2050

When using the following XML file as testcase the Marionette test as added afterward fails.

XML

<?xml version="1.0" encoding="UTF-8"?>
<dgbRecord xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="http://www.digibis.com/schemas/dgb-bibdetail-v1.0.xsd"><recordHeader><id>779</id><controlNumber>BAB20120007468</controlNumber><section cod="MON">GEN-Monografías</section><originalObjectType>monografia</originalObjectType><thumbnail>http://digibis-int.digibis.com:8080/digibisbib_int/i18n//api/thumbnail.do?id=2730</thumbnail><accessRestriction>digital_contents</accessRestriction><mainTitle>Se puede consultar, ver la ficha y acceder al contenido</mainTitle><publication>Madrid : Digibís, 2012</publication><publicationDate>2012</publicationDate></recordHeader><metadata format="ficha_xml"><bibliographic>
<title label="Título">Se puede consultar, ver la ficha y acceder al contenido</title>
<publication label="Publicación">Madrid : Digibís, 2012</publication>
<physicalDescription label="Descripción física">1</physicalDescription>
<note label="Notas">Prueba de accesos. Búsqueda Europeana</note>
</bibliographic></metadata></dgbRecord>

Marinoette test:

        self.marionette.navigate("file:///Users/foo/testcase.xml")
        elem = self.marionette.find_element(By.TAG_NAME, "publication")
        print(elem.text)

And here the exact failure:

ERROR _a/test_minimized.py TestMinimizedTestCase.test_case - marionette_driver.errors.UnknownException: TypeError: can't access property "display", c is undefined
stacktrace:
	$b@chrome://remote/content/marionette/atom.js:85:50
	X@chrome://remote/content/marionette/atom.js:83:252
	jc@chrome://remote/content/marionette/atom.js:96:68
	oc@chrome://remote/content/marionette/atom.js:101:95
	atom.getElementText/</<@chrome://remote/content/marionette/atom.js:104:295
	atom.getElementText/<@chrome://remote/content/marionette/atom.js:104:481
	atom.getElementText@chrome://remote/content/marionette/atom.js:104:505
	getElementText@chrome://remote/content/marionette/actors/MarionetteCommandsChild.jsm:335:22
	receiveMessage@chrome://remote/content/marionette/actors/MarionetteCommandsChild.jsm:121:31

It means that something in the imported Selenium atoms started to fail.

I can reproduce locally and will run a regression check to find out what exact change caused this regression.

As it looks like this is a very old problem when running Firefox Nightly builds. I'm able to reproduce back to Firefox 97. So I assume it's a preference change that happened for 105 and enabled a feature by default on release which was disabled before. This makes it harder to investigate because mozregression cannot be used.

After inspecting a couple of now globally enabled preferences I found that layout.css.computed-style.styles-outside-flat-tree makes the difference here. The value was changed on bug 1784943.

Given the minimized code of atom.js it's not fully clear to me if that is an expected behavior or not. Emilio mind having a look?

Flags: needinfo?(emilio.alvarez96)
Regressed by: 1784943
Component: Marionette → CSS Parsing and Computation
Product: Testing → Core

I assume that ni? was for me.

Flags: needinfo?(emilio.alvarez96) → needinfo?(emilio)

So, this is kind of expected, but is a side-effect of how we implement XML pretty-printing. If you toggle devtools.inspector.showAllAnonymousContent, you can see on devtools the DOM of the page is really hidden out of the flattened tree, and all the rendered stuff is in a shadow tree.

Our behavior per spec is correct: those elements shouldn't be getting styles (and were getting wrong styles before).

Now there are multiple ways of fixing it.

  • The first (ideal) is fixing marionette to avoid looking at computed styles of elements out of the flattened tree (these returns a style with length == 0).
  • Somehow don't do the XML pretty-printing when using marionette?
  • We could also shove the elements somewhere in a <slot> in the XML pretty-printing code, so that they get back in the flat tree.

I attached a PoC for the later. The former is probably preferred but it's not too much code.

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

(In reply to Emilio Cobos Álvarez (:emilio) from comment #6)

So, this is kind of expected, but is a side-effect of how we implement XML pretty-printing. If you toggle devtools.inspector.showAllAnonymousContent, you can see on devtools the DOM of the page is really hidden out of the flattened tree, and all the rendered stuff is in a shadow tree.

Thanks Emilio for explaining whereby I'm not really familiar with the underlying issue and I would have to dig deeper into that problem. Maybe James has a position here.

Our behavior per spec is correct: those elements shouldn't be getting styles (and were getting wrong styles before).

Now there are multiple ways of fixing it.

  • The first (ideal) is fixing marionette to avoid looking at computed styles of elements out of the flattened tree (these returns a style with length == 0).

Per WebDriver spec we are required to use the Selenium atoms to get the visible text. But I wonder if there is something that could be done on the Atom side to get this fixed. Therefore we might have to know which specific code is causing this problem.

  • Somehow don't do the XML pretty-printing when using marionette?

I've never seen yet that people use WebDriver to automate tasks for XML files. Even it will be rarely used it's a valid case. So I assume this would involve setting the above mentioned pref?

  • We could also shove the elements somewhere in a <slot> in the XML pretty-printing code, so that they get back in the flat tree.

Sorry but I don't know anything about that and would like to defer to James here.

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

It sounds like this isn't really related to XML, so much as the selenium atom code not working well with shadow dom. We should test if the latest version of that code fixes this.

That being said, I don't know that we should do much here. The way that XML pretty printing works is pretty much an implementation detail that people shouldn't depend on. I doubt there are enough people doing this kind of thing that we should change the implementation just so that it works with some pre-shadow-dom selenium code. If people are using WebDriver for reading XML content then replacing the elem.text call with elem.getProperty("textContent") seems more likely to do what they want (and won't be doing style checks that don't make any sense in the context of an XML document).

Flags: needinfo?(james)

(In reply to James Graham [:jgraham] from comment #8)

It sounds like this isn't really related to XML, so much as the selenium atom code not working well with shadow dom. We should test if the latest version of that code fixes this.

See my comment above. It's still not working. I wonder if we should file an issue for Selenium to get this fixed?

Set release status flags based on info from the regressing bug 1784943

The severity field is not set for this bug.
:dholbert, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(dholbert)

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

(In reply to James Graham [:jgraham] from comment #8)

It sounds like this isn't really related to XML, so much as the selenium atom code not working well with shadow dom. We should test if the latest version of that code fixes this.

See my comment above. It's still not working. I wonder if we should file an issue for Selenium to get this fixed?

What do you think about that, James? Not sure if it's possible but maybe worth investigating?

Flags: needinfo?(james)
Severity: -- → S3
Flags: needinfo?(dholbert)

I wonder if we should just add a try/catch in the marionette code to handle this case. It seems worth filing an upstream bug if we can write a non-XML-pretty-print testcase that ends up with the same kind of error.

Flags: needinfo?(james)

(In reply to James Graham [:jgraham] from comment #13)

I wonder if we should just add a try/catch in the marionette code to handle this case. It seems worth filing an upstream bug if we can write a non-XML-pretty-print testcase that ends up with the same kind of error.

Or we handle it differently similar to isElementEnabled in case of XML files but not sure how complicated that could be:
https://searchfox.org/mozilla-central/rev/2809416b216b498ec3d8b0e65c25a135f4f5f37d/remote/marionette/interaction.sys.mjs#729-732

The following code would make it work and I'm happy to provide a patch for Marionette:

   async getElementText(options = {}) {
     const { elem } = options;

-    return lazy.atom.getElementText(elem, this.document.defaultView);
+    if (
+      ["application/xml", "text/xml"].includes(this.document.contentType)
+    ) {
+      return elem.textContent;
+    } else {
+      return lazy.atom.getElementText(elem, this.document.defaultView);
+    }
   }
Flags: needinfo?(james)

I don't think we want to switch on content-type; it's possible to have XML content types that are given style information rather than the default presentation.

From our point of view it makes sense to treat the Selenium atoms as fallible and have some reasonable fallback (e.g. to innerText or textContent) if they fail. That's even a thing the spec could require.

Flags: needinfo?(james)
Component: CSS Parsing and Computation → Marionette
Product: Core → Testing
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/eca05a9ca8e8 [marionette] "Get Element Text" has to return text for pretty-printed XML. r=webdriver-reviewers,jgraham
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/36795 for changes under testing/web-platform/tests
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 108 Branch
Upstream PR merged by moz-wptsync-bot
Blocks: webdriver:m5
Points: --- → 1
Priority: -- → P1
Whiteboard: [webdriver:m5]
Whiteboard: [webdriver:m5] → [webdriver:m5][webdriver:relnote]

No complaints from Selenium nor any report from users yet. So I assume we are fine with that change.

Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: