Skip to content

Conversation

hamzaaitbrik
Copy link
Contributor

Hello @stephanlensky. Here's another pull request prior to the last one.

I couldn't resolve the conflict between the base repository and my fork. The conflict between the two branches is in the functions I added/removed/modified. I'll leave resolving the conflicts to you if you don't mind.

Feel free to review the code, and let me know what you think.

Thanks,
Hamza

hamzaaitbrik and others added 27 commits December 20, 2024 17:25
…nd iframes, and adding the functionality to search either by text, or using a tagname and a dictionary of attributes
…s for a more efficient way to find elements, users have total control over which method they want to search; either by tagname, attributes, or text
…e into attrs dictionary, ex: attrs['innerText'] = textValue
…h any combination of tagname, attrs, and text
… the new functions. modifying wait_for to work with the new functions. also, bug fixes
…les the logic of finding elements, instead of duplicating the logic; now find_element_by_tagname_attrs_text and find_elements_by_tagname_attrs_text call _find_elements_by_tagname_attrs_text with an argument return_after_first_match indicating whether we're aiming for a single element, or a list of elements
@hamzaaitbrik
Copy link
Contributor Author

hamzaaitbrik commented Feb 8, 2025

@stephanlensky I might need your help resolving conflicts in this pull request. I did what I could without breaking the existing functionality.

@stephanlensky
Copy link
Member

stephanlensky commented Feb 9, 2025

Hey Hamza, I resolved all of the conflicts and did some small cleanups on a new branch here: https://github.com/stephanlensky/zendriver/tree/hamzaiitbrik/main

Feel free to update your fork to use that branch, I cannot do it on my end since the fork is in your account.

However, I noticed that this is probably not actually working as intended. For example, this test is failing:

>       assert result.tag == "li"
E       AssertionError: assert 'ul' == 'li'
E         
E         - li
E         + ul

This is because your code is returning the <ul> element as the one which contains the text Apples, when it should really be the <li> element. The HTML file used in the test is here

<ul>
    <li aria-label="Apples (42)">Apples</li>
    <li>Bananas</li>
    <li>Carrots</li>
    <li>Donuts</li>
    <li>Eggs</li>
    <li>French Fries</li>
    <li>Grapes</li>
<ul>

Hopefully it is clear why returning <ul> does not make sense.

@hamzaaitbrik
Copy link
Contributor Author

Hello @stephanlensky, you actually did a typo on my name and added an additional "i," it's hamzaaitbrik instead of hamzaiitbrik. Could you please fix that?

As for the test, I just inspected the test_tab.py file and I noticed that the test that fails calls select instead of find, and I'm pretty sure I haven't touched any functionality of the select function. I will update my fork to use that branch once you change its name, and I'll try to run those tests and debug what needs debugging.

Thanks,
Hamza

@stephanlensky
Copy link
Member

stephanlensky commented Feb 9, 2025

Whoops, I'm sorry I actually just linked the wrong test. This is the one that's failing.

Sorry for the typo as well, I pushed a new branch here: https://github.com/stephanlensky/zendriver/tree/hamzaaitbrik/main

@stephanlensky
Copy link
Member

You should be able to replicate the failure locally by running ./scripts/test.sh on that branch.

@stephanlensky
Copy link
Member

Omg, my copy key isn't working, I keep linking the wrong thing. One second, sorry

@stephanlensky
Copy link
Member

Okay, fixed now. For posterity, here is the correct link: https://github.com/stephanlensky/zendriver/blob/hamzaaitbrik/main/tests/core/test_tab.py#L36-L43

@hamzaaitbrik
Copy link
Contributor Author

Haha, no worries! Take your time, and let me know what's up when you're done.

@hamzaaitbrik
Copy link
Contributor Author

Hello @stephanlensky. It took me a while to answer, got little to no free time..

Anyway, I did some debugging to pinpoint where the problem was; and what I found is that the parent element's text returns the child element's text; and that's why when the ul element is first found, the _find_element_by_tagname_attrs_text function returns that element right away.

To try and dig a little bit deeper, I added more code to check if our element that we found by text has children, and those children has the text value we're looking for, and if so; we simply ignore the parent element and return the child. The code is as follows:

# sometimes the parent element is preceived as the element holding the text content while it actually the child that does so, below is the code to handle that situation
if (
    text and matches_text and elem.children
):  # we only check for children containing the text if text was provided and elem.children exists
    # if elem.children:
    for child in elem.children:
        if text in child.text.strip().lower():
            elem = child
            break

The above code was added in the traverse function inside the _find_elements_by_tagname_attrs_text function. It was added exactly when trying to find a match, so that we will always look inside the element's children to try and narrow down only if we're looking by text, and matches_text evaluates to True, and elem.children exists. I tested that code on groceries.html, and it returned the correct element. I also tested it on a few live web-pages, and it delivered.

I also added another function in the Element class. The function is called blur, and it works as a way to reverse the focus function. Why I added that function is because I am transitioning into working with Zendriver/Nodriver, and when focusing on select fields, sometimes buttons behave strangely when the focus function is not reversed. I did test the blur function I added, and it worked as I intended.

Let me know what you think.

Regards,
Hamza

@stephanlensky
Copy link
Member

Hi Hamza,

Sorry for the long delay, I was dealing with some things IRL. I just took a look at this again, and also merged in the latest changes to your fork for you.

Unfortunately it seems like some of the tests are still failing. Have you tried to run them locally?

You can do so like

$ ./scripts/test.sh
...
FAILED tests/bot_detection/test_browserscan.py::test_browserscan[headless0] - AssertionError: assert 'What is webdriver' == 'Normal'
FAILED tests/core/test_tab.py::test_find_finds_element_by_text[headless0] - TimeoutError: Time ran out while waiting for element with tagname: apples, attributes: None, text:None
FAILED tests/docs/tutorials/test_account_creation_tutorial.py::test_account_creation_tutorial_2[headless0] - Exception: could not find position for <script >

I am trying to spend some more time on this project again, so I will hopefully be able to respond more quickly in the future.

Copy link
Contributor

github-actions bot commented Aug 1, 2025

This pull request was automatically closed because it has been inactive for 7 days since being marked as stale.

@github-actions github-actions bot closed this Aug 1, 2025
@nathanfallet nathanfallet reopened this Sep 22, 2025
@hamzaaitbrik
Copy link
Contributor Author

Hello @nathanfallet, I'm still interested in merging this since it's a big issue of the web automation community; what do you think?

@nathanfallet
Copy link
Member

@hamzaaitbrik Yes this is still a great idea! Is it a WIP or is it ready for review? Since the PR is here for so long I don't know if you had more things to do on it or not 🤷‍♂️

@hamzaaitbrik
Copy link
Contributor Author

@nathanfallet I think it needs more work since I tried the newer version of zendriver with my modifications and it threw me a bunch of errors; no biggie, I can modify on those functions to make it work again.
The issue me and @stephanlensky faced is it failed tests, I mean...I'm running on Windows, and those tests seem to work only on Mac..? I'm not really sure. But I've been running that pull request on production for a few months now with no issues.
Like I said, I can get this going again, but can you guys test it on my behalf? Or at least make some OS-friendly tests that could run on Windows and Linux too.
Thanks,
Hamza

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

Successfully merging this pull request may close these issues.

3 participants