1672788 - ChromeWebElements are recorded as ContentWebElements in the ReferenceStore

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P3)

Description

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

For example, run testing/marionette/harness/marionette_harness/tests/unit/test_element_rect_chrome.py which calls self.marionette.find_element(By.ID, 'textInput').rect:

1603388141178	Marionette	DEBUG	2 -> [0,27,"WebDriver:FindElement",{"using":"id","value":"textInput"}]
1603388141179	Marionette	TRACE	child data in {"strategy":"id","selector":"textInput","opts":{"startNode":null,"timeout":0,"all":false}}
1603388141179	Marionette	TRACE	child result {<elem>}
1603388141180	Marionette	TRACE	parent out result {"data":{"browsingContextId":36,"id":0.5817461624520781,"webElRef":{"element-6066-11e4-a52e-4f735466cecf":"8335330c-19c2-4247-b498-6a892ec9189a"}}}
1603388141180	Marionette	TRACE	ReferenceStore after {"8335330c-19c2-4247-b498-6a892ec9189a":{"browsingContextId":36,"id":0.5817461624520781,"webElRef":{"element-6066-11e4-a52e-4f735466cecf":"8335330c-19c2-4247-b498-6a892ec9189a"}}}

Note that the result is ContentWebElement ({"element-6066-11e4-a52e-4f735466cecf":"8335330c-19c2-4247-b498-6a892ec9189a"})

This because when we create the ElementIdentifier for the ReferenceStore the call to WebElement.from(el) creates a ContentDOMElement instead since the element's namespace isn't http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul.

Continuing the log:

1603388141180	Marionette	DEBUG	2 <- [1,27,null,{"value":{"element-6066-11e4-a52e-4f735466cecf":"8335330c-19c2-4247-b498-6a892ec9189a"}}]
1603388141180	Marionette	DEBUG	2 -> [0,28,"WebDriver:GetElementRect",{"id":"8335330c-19c2-4247-b498-6a892ec9189a"}]
1603388141180	Marionette	TRACE	parent data {"elem":{"chromeelement-9fc5-4b51-a3c8-01716eedeb04":"8335330c-19c2-4247-b498-6a892ec9189a"}}

The id 8335330c-19c2-4247-b498-6a892ec9189a turns into chromeelement-9fc5-4b51-a3c8-01716eedeb04":"8335330c-19c2-4247-b498-6a892ec9189a via WebElement.fromUUID(id, "chrome")

This gets looked up in the ReferenceStore by id alone, which why is hasn't caused problems yet:

1603388141180	Marionette	TRACE	parent serializedData {"elem":{"browsingContextId":36,"id":0.5817461624520781,"webElRef":{"element-6066-11e4-a52e-4f735466cecf":"8335330c-19c2-4247-b498-6a892ec9189a"}}}
1603388141181	Marionette	TRACE	child data in {"elem":{}}
1603388141181	Marionette	TRACE	child result {"x":8,"y":27,"width":205,"height":30}

Comment 1

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

I also see examples of ChromeWebElements being stored successfully in the ReferenceStore, so it depends on the particular element and if anything this is a bug in isXULElement

Comment 2

[Chris Peterson [:cpeterson]]

Tracking marionette-fission-mvp bugs for Fission Beta milestone (M7).

Comment 3

[Julian Descottes [:jdescottes]]

We could update isXULElement in order to check the namespace of the document instead of the namespace of the element. However, looking at the current usage of isXULElement/isDOMElement, not all the consumers care about the context. Some consumers only want to know if the element is from the XUL namespace or the HTML namespace, in order to process them differently.

Overall, I found two spots which should be about the context rather than the element type:

• element.add (to compute the context variable)
• element.from

For those methods, I think we should use a new API isInXULDocument. Other call sites should not be updated because I think it makes more sense for them to keep checking the namespace of the actual element.

Review of isXULElement usage

element.isSelected

if (element.isXULElement(el)) {
    if (XUL_CHECKED_ELS.has(el.tagName)) {
      return el.checked;
    } else if (XUL_SELECTED_ELS.has(el.tagName)) {
      return el.selected;
    }
  } else if (element.isDOMElement(el)) {
    if (el.localName == "input" && ["checkbox", "radio"].includes(el.type)) {
      return el.checked;
    } else if (el.localName == "option") {
      return el.selected;
    }
  }

Here isXULElement should really return true only for real XUL elements, not for HTMLElements embedded in a XUL doc.

element.add

    const isDOMElement = element.isDOMElement(el);
    const isDOMWindow = element.isDOMWindow(el);
    const isXULElement = element.isXULElement(el);
    const context = isXULElement ? "chrome" : "content";

    if (!(isDOMElement || isDOMWindow || isXULElement)) {
      throw new TypeError(
        "Expected an element or WindowProxy, " + pprint`got: ${el}`
      );
    }
    // ...
    return WebElement.fromUUID(i, context);

Here, context should be set to "chrome" if the element is in a XUL document, even if it is a HTML element. This would be a good candidate to use isInXULDocument.

element.from

    if (element.isDOMElement(node)) {
      return new ContentWebElement(uuid);
    } else if (element.isDOMWindow(node)) {
      if (node.parent === node) {
        return new ContentWebWindow(uuid);
      }
      return new ContentWebFrame(uuid);
    } else if (element.isXULElement(node)) {
      return new ChromeWebElement(uuid);
    }

Here we should create a ChromeWebElement if the context is "chrome". So we should check if the element is in a XUL document rather before checking the type of the element.

interaction.clickElement

  if (element.isXULElement(el)) {
    await chromeClick(el, a11y);
  } else if (specCompat) {
    await webdriverClickElement(el, a11y);
  } else {
    logger.trace(`Using non spec-compatible element click`);
    await seleniumClickElement(el, a11y);
  }

This one is not 100% clear to me. Should an HTML element embedded in a XUL document be handled via chromeClick?

interaction.selectOption

  if (element.isXULElement(el)) {
    throw new TypeError("XUL dropdowns not supported");
  }

Here it seems that isXULElement is really about the element itself and should not consider HTML elements in XUL documents as XUL elements.

It's interesting to note that selectOption throws for XUL elements

interaction.isElementEnabled

  if (element.isXULElement(el)) {
    // check if XUL element supports disabled attribute
    if (DISABLED_ATTRIBUTE_SUPPORTED_XUL.has(el.tagName.toUpperCase())) {
      if (
        el.hasAttribute("disabled") &&
        el.getAttribute("disabled") === "true"
      ) {
        enabled = false;
      }
    }
  }

Here isXULElement should only return true for actual XUL elements, since it's then used against a shortlist of XUL element tagnames.

element.isDOMElement

isDOMElement is the flip side of isXULElement, we should also review the call sites for this method.

Call sites really care about the element type, and not about the context:

• element.isDisabled
• element.isEditable
• element.isEditingHost
• element.isMutableFormControl
• element.isReadOnly

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: Bug 1672788 - [marionette] Record html elements in XUL documents as ChromeWebElements

Add a new element.isInXULDocument API and use it in element.from to decide if the element should be created as a ChromeWebElement or a ContentWebElement

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e3cf08935a2d
[marionette] Record html elements in XUL documents as ChromeWebElements r=marionette-reviewers,whimboo,maja_zf

Comment 6

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/e3cf08935a2d