Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

child and children cannot use vector as q #559

Closed
dalugm opened this issue Jan 16, 2024 · 6 comments
Closed

child and children cannot use vector as q #559

dalugm opened this issue Jan 16, 2024 · 6 comments

Comments

@dalugm
Copy link

dalugm commented Jan 16, 2024

Version

1.0.40

Platform
Operating System: macOS 12.7.2
Clojure version: 1.11.1
JDK vendor and version: 21.0.2

Browser vendor: chrome
Browser version: 120.0.6099.216
WebDriver version: 120.0.6099.109

(also happens in the newest firefox version and geckodriver version)

Symptom

The test html file:

<!DOCTYPE html>
<html lang="en">
  <head>
    <meta content="text/html; charset=utf-8" http-equiv="Content-Type">
    <meta name="viewport" content="width=device-width, initial-scale=1">
    <title>Template</title>
  </head>
  <body>
    <div class="z_table">
      <table>
        <thead>
          <tr>
            <th width="140">head1</th>
            <th width="140">head1</th>
            <th width="140">head1</th>
            <th width="140">head1</th>
            <th width="140">head1</th>
            <th width="140">head1</th>
            <th width="140">head1</th>
            <th></th>
          </tr>
        </thead>
        <tbody>
          <tr>
            <td>body1</td>
            <td>body1</td>
            <td>body1</td>
            <td>body1</td>
            <td>body1</td>
            <td> </td>
            <td width="140"></td>
            <td></td>
          </tr>
        </tbody>
      </table>
      <table>
        <thead>
          <tr>
            <th width="140">head2</th>
            <th width="140">head2</th>
            <th width="140">head2</th>
            <th width="140">head2</th>
            <th width="140">head2</th>
            <th width="140">head2</th>
            <th width="140">head2</th>
            <th></th>
          </tr>
        </thead>
        <tbody>
          <tr>
            <td>body2</td>
            <td>body2</td>
            <td>body2</td>
            <td>body2</td>
            <td>body2</td>
            <td> </td>
            <td width="140"></td>
            <td></td>
          </tr>
        </tbody>
      </table>
      <table>
        <thead>
          <tr>
            <th width="140">head3</th>
            <th width="140">head3</th>
            <th width="140">head3</th>
            <th width="140">head3</th>
            <th width="140">head3</th>
            <th width="140">head3</th>
            <th width="140">head3</th>
            <th>head3</th>
          </tr>
        </thead>
        <tbody>
          <tr>
            <td>body3</td>
            <td>body3</td>
            <td>body3</td>
            <td>body3</td>
            <td>body3</td>
            <td>body3</td>
            <td width="140">body3</td>
            <td>body3</td>
          </tr>
        </tbody>
      </table>
    </div>
  </body>
</html>

Here's the clj file

(ns test
  (:require
   [etaoin.api :as e]))

(def driver (e/chrome))
(e/go driver "file:///test.html")

(let [table-wrapper-q {:tag :div :class :z_table}
      table-el-list (e/query-all driver [table-wrapper-q {:tag :table}])
      last-el (last table-el-list)]
  (e/children driver last-el [{:tag :thead}]) ; error
  (e/children driver last-el {:tag :thead})   ; normal
  (e/child driver last-el [{:tag :thead} {:tag :th :index 8}]) ; error
  (e/child driver last-el {:tag :th :index 8})                 ; normal
  )

As the comment, if I use the vector as the q, it will throw an error:

  1. Unhandled java.lang.Exception
    Wrong query: [{:tag :thead} {:tag :th, :index 8}]

Both child and children have this problem, while the doc says

https://cljdoc.org/d/etaoin/etaoin/1.0.40/api/etaoin.api#child

Actual behavior
Throw an error

Expected behavior
Work like query query-all

Diagnosis
No idea.. I'm a clj noobie just learning to use clojure...

Action
Just report, doesn't have the ability to fix this, let me know if you have any more question ;)

@dgr
Copy link
Contributor

dgr commented Jul 20, 2024

I'm working on this one.

@dgr
Copy link
Contributor

dgr commented Jul 24, 2024

After looking into this, I have a few thoughts.

  1. child and children are less than ideal names for these functions. It seems like the original author was trying to suggest that the element being searched for is a descendent of the starting element. But in "DOM-speak," the term "child" is used to describe a direct child, not what could be a far more remote descendant deeper in the DOM tree. I don't think we can change those now, but we might want to create new functions with better names (something like query-from and query-all-from comes to mind) and deprecate child and children.
  2. The q parameter is described in the docstrings as being similar to that in query. If you look at query, it takes essentially four different types of arguments: a keyword representing an ID; a string representing an XPath expression; a map with CSS, XPath, or the map syntax described in the User Guide; multiple of these previous things wrapped in a vector, which is used as a path to search recursively for the next element, using the previously found element as the root of the search; and finally, the special keyword :active.
  3. That last one, :active, is really problematic because there is only one active element on the page. If you look at the Web Driver spec, there is a special call, Get Active Element, that retrieves the active element. This is different than finding elements, which use Web Driver's Find Element(s) APIs. Etaoin's query API "complects" (as Rich Hickey would say) these two things.
  4. When it comes to child / children, handling :active becomes a problem. What if the active element is not in the DOM subtree rooted at the ancestor-el parameter of child / children? Should we just return the active element regardless of its position in the DOM hierarchy relative to ancestor-el? What would the user expect?

Recommendations:

  1. Create two new functions named query-from and query-all-from which are like the current child and children functions. Have those two new functions implement all of the query options for q except for :active.
  2. Deprecate child and children.
  3. Create a new Etaoin API, get-active-element, which does what it says. There is already a private API, get-active-element*, to handle this (used by things like fill-active), so all that needs to be is create a non-private API with doc strings, tests, etc. Any query tests for :active could be repurposed easily, I suspect.
  4. Deprecate using query with :active. Since it's a special parameter, it can't be removed without breaking existing code, but it can be removed from documentation, etc. Unfortunately, it conflicts with query's use of turning keywords into IDs (you can't search for an element with an ID of "active" using (query driver :active), but that's true today, too.

This would provide query-from and query-all-from with a recursive querying capability starting from a designated root element, which is what @dalugm is looking for, and which Web Driver makes partially available (not the recursive model) with its Find Element(s) From APIs. It would also clean up some current Etaoin rough edges.

Simpler alternative proposals:

  1. Just implement the vector syntax for child and children as is. Don't rename or deprecate anything. Simply document that child and children support the q parameter as in query with the exception of :active which makes no sense in the context of child / children.
  2. The simplest alternative is to modify the docstrings of child and children such that they are correct and don't reference query. That is, alter the documentation to match the existing behavior. IMO, this feels like weak sauce.

@lread
Copy link
Collaborator

lread commented Jul 24, 2024

Wow, nice analysis! Thanks @dgr!

I like all of your primary recommendations (1 through 4). Please proceed as you see fit.

If you feel it helps, feel free to break work down into separate PRs.

@lread
Copy link
Collaborator

lread commented Aug 8, 2024

I've been looking at chromedriver source, and lo and behold, I might have found the influence for the odd naming of child and children:

  FIND_CHILD_ELEMENT = (_Method.POST, '/session/:sessionId/element/:id/element')
  FIND_CHILD_ELEMENTS = (
      _Method.POST, '/session/:sessionId/element/:id/elements')

@dgr
Copy link
Contributor

dgr commented Aug 13, 2024

I've been looking at chromedriver source, and lo and behold, I might have found the influence for the odd naming of child and children:

  FIND_CHILD_ELEMENT = (_Method.POST, '/session/:sessionId/element/:id/element')
  FIND_CHILD_ELEMENTS = (
      _Method.POST, '/session/:sessionId/element/:id/elements')

Interesting. Looks like those were older legacy names that got turned into Find Element(s) From Element in the later versions of the WebDriver spec. Seems like a graceful renaming is therefore warranted.

@dgr dgr mentioned this issue Aug 17, 2024
lread pushed a commit that referenced this issue Aug 17, 2024
* Add query-from and query-all-from

* Deprecate child and children

* Make get-active-element a public API

Previously, get-active-element* was a private API. This simply changes
the name to remove the "*" and makes it public.

* Update doc strings and user guide to deprecate :active

Suggests to use get-active-element instead of (query driver :active).

* Modify get-active-element to use unwrap-webdriver-object

* Update CHANGELOG to better reflect issue 559 changes

* Fix redundant let found by clj-kondo

* Suppress clj-kondo warnings for calling deprecated child/children

* Suppress Eastwood warnings for calling deprecated child/children

Test suite still calls child/children to detect regressions in these
deprecated, but not yet removed, functions.

* Credit dgr in CHANGELOG

* Clarify issues with :active in User Guide and query doc string
@lread
Copy link
Collaborator

lread commented Aug 17, 2024

Completed via #627, thanks @dgr!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants