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

"Get BoundingBox" fails if element is not visible, wo proper error. #3651

Open
Andre-A-AtGithub opened this issue Jun 12, 2024 · 17 comments · May be fixed by #3821
Open

"Get BoundingBox" fails if element is not visible, wo proper error. #3651

Andre-A-AtGithub opened this issue Jun 12, 2024 · 17 comments · May be fixed by #3821
Labels
bug Something isn't working priority: high

Comments

@Andre-A-AtGithub
Copy link

Andre-A-AtGithub commented Jun 12, 2024

Describe the bug
Using FOR loop to iterate "Elements" "Get BoundingBox" gives error.

This worked before when using "older" versions. After updating to latest version of Python, Robot framework, Browser, etc this issue arose. See below overview of exact "old" and "new" versions:

Python: 3.9 → 3.12
Robotframework: 4.1.1 → 7.0
Robotframework-browser: 8.0.2 → 18.5.1

For more information see Robot framework Slack: https://robotframework.slack.com/archives/C015KB1QSDN/p1717657032495999

Print screen of debug log
image

To Reproduce
Environment:
This mainly happens in the test auto test framework I use, it is for me not certain if this also occurs in other setups.

Steps to reproduce the behavior:

  1. Run "Get Elements" in a way it gives more than 1 results
  2. Use FOR loop to iterate the elements to find the boundingbox of the elements
  3. When the list of elements contains more than 1 element in the list than there is a error: TypeError: 'NoneType' object is not iterable". I also added a image of the error.

Relevant code in our test framework:
${elements}= Get Elements ${selector}
FOR ${element} IN @{elements}
${box}= Get BoundingBox ${element}
IF ${box}
Append To List ${bounded_elements} ${element}
END
END
RETURN ${bounded_elements}

Expected behavior
That it iterates all elements that are in the list

Screenshots
If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information):

  • OS: Windows 10/11 (both give same issue)
  • Browser: Chrome
  • Version: 125.0.6422.26 (developer build) (64-bits)

Additional context
Add any other context about the problem here.

@aaltat aaltat added bug Something isn't working priority: high labels Jun 15, 2024
@aaltat
Copy link
Member

aaltat commented Jun 15, 2024

@allcontributors please add @Andre-A-AtGithub for bugs

Copy link
Contributor

@aaltat

I've put up a pull request to add @Andre-A-AtGithub! 🎉

@robinmackaij
Copy link

I'm assisting Andre with the analysis of this issue and this is my current understanding of what's happening:

In the past (when this framework / these keywords were created), Get Elements returned a list of Playwright ElementHandles (https://playwright.dev/docs/api/class-elementhandle). This has changed; now, Get Elements returns a list of Playwright Locators (https://playwright.dev/docs/api/class-locator).

This change seems to be (partially?) incompatible with the cascaded locator syntax (>>), especially with the * prefix.

What we see happening now is that where in the past we got something like <some_guid> >> text=some_text we now get <the locator that was used for Get Elements >> nth=# >> text=some_text.

Note the nth=# that Playwright injects.

It really goes off-track when * is involved. Here, ${element} is an element from an Get Elements result where the locator was something like *css=.Pane >> text=some_text
${element} >> css=.SubPane now results in css=.Pane >> text=some_text >> css=.SubPane >> nth=#
So instead of trying to get the nth of the matched .Pane elements, it now tries to get the nth of the .SubPane.

I hope this makes sense, it's hard to provide an example showing the issue, especially since such a sample would require different Playwright / Browser versions.

If the problem is clear, would there be any suggestions how we could repair the framework? Note there's quite a bit of use of the * option and mixed locator strategies. From the Playwright documentation I gather that getting an ElementHandle is still possible. Could this be made possible with the Browser keywords?

@aaltat
Copy link
Member

aaltat commented Oct 3, 2024

@Snooz82 I think you did drive this change and I don’t anymore remember why it was done. Could you explain and even better if you have suggestions how to fix it.

@Snooz82
Copy link
Member

Snooz82 commented Oct 3, 2024

the real origin was that back in the days playwright did actually keep a handle on the real element. similar to Selenium. BUT you may know the problems with Seleniums „Element Stale Exception“ that if that element changes, it may not be accessible.

so they changed it so, that the Locator elements basically just store the selector.

so if you do in playwright page.getByRole('button', { name='Login' }) you would also just get an object that contains the selector. not the element itself.

Playwright docs:

The method returns an element locator that can be used to perform actions on this page / frame. Locator is resolved to the element immediately before performing an action, so a series of actions on the same locator can in fact be performed on different DOM elements. That would happen if the DOM structure between those actions has changed

so when we do Get Elements we actually just do Get Element Count and then return a list of the original selector with the appendix >> nth=i while i is the index of the element.

@Snooz82
Copy link
Member

Snooz82 commented Oct 3, 2024

the * selection does not work with the cascading selectors! that is independent if you get the guid or not, because this guid thing was still just resolved to a selector. we could try if we could do Get Element with a parent and do that with playwright API to see if that solves the issue with the *

@Snooz82
Copy link
Member

Snooz82 commented Oct 3, 2024

So this Playwright code:

await page.locator('*css=.box >> "ABS"').locator('input')._selector

gives the following selector:

*css=.box >> "ABS" >> input

So it will not be the input element that is a child of the .box element, but the .box element itself.

SO:
We can not do anything about the * thing not working.
In these cases you have to use xpath ../.. and so...

@Snooz82
Copy link
Member

Snooz82 commented Oct 3, 2024

And i think the overall problem here is, that you have an element that is not visible and therefore Get Bounding Box does fail with this error.

I can without any problems iterate over many elements like this:

${e}    Get Elements    input
FOR     ${el}     IN     @{e}
    ${t}    Get Attribute    ${el}     attribute=type
    Log To Console     ${t}
    ${b}    Get Element States    ${el}
    Log To Console     ${b}
END

As long as all of them are visible.
But once one element is hidden Get Bounding Box does fail.
I think we can fix that, that you get a proper Error Message instead of this.

@Snooz82
Copy link
Member

Snooz82 commented Oct 3, 2024

Get BoundingBox does fail with weird error:

async function getBoundingBox(request2, state2) {
  const selector = request2.getSelector();
  const strictMode = request2.getStrict();
  const locator = await findLocator(state2, selector, strictMode, void 0, true);
  const boundingBox = await locator.boundingBox();
  return jsonResponse(JSON.stringify(boundingBox), "Got bounding box successfully.");
}

So locator.boundingBox(); does return null when element is not visible.
Thats why it fails.

If you are ok with it, i would change that issue to fix the keyword.

@Snooz82 Snooz82 changed the title Using FOR loop to iterate "Elements" "Get BoundingBox" gives error "Get BoundingBox" fails if element is not visible, wo proper error. Oct 4, 2024
@robinmackaij
Copy link

Get BoundingBox does fail with weird error:

async function getBoundingBox(request2, state2) {
  const selector = request2.getSelector();
  const strictMode = request2.getStrict();
  const locator = await findLocator(state2, selector, strictMode, void 0, true);
  const boundingBox = await locator.boundingBox();
  return jsonResponse(JSON.stringify(boundingBox), "Got bounding box successfully.");
}

So locator.boundingBox(); does return null when element is not visible. Thats why it fails.

If you are ok with it, i would change that issue to fix the keyword.

If instead of raising an error, it would return None, that'd resolve the original issue.

I only have access to the codebase if I'm in office with Andre, I'll check the use of * in combination with loops; I wrote these keywords a bit over 3 years ago and they worked back then. Andre called on me since things broke after recent Playwright / Browser updates. I don't have all the details yet, I just know it did work back then. The information you provided does give me a better idea of what to look for though.

@Andre-A-AtGithub
Copy link
Author

First of all I really appreciate the effort you all put in this. With the new info I am now able to reproduce this issue fully, I can confirm it is specifically with a element that is hidden. I used this test to reproduce it, it uses a dutch webshop.

*** Test Cases ***
Open browser and do stuff
@{elements} Create List
@{bounded_elements} Create List
New Browser chromium headless=No
New Page https://www.megekko.nl
Click css=#CybotCookiebotDialogBodyLevelButtonLevelOptinAllowAll
${elements}= Get Elements css=.hdr_catMenuMobile
FOR ${element} IN @{elements}
${box}= Get BoundingBox ${element}
IF ${box}
Append To List ${bounded_elements} ${element}
Log ${box}
END
END
Log ${bounded_elements}

@Snooz82
Copy link
Member

Snooz82 commented Oct 4, 2024

i would not like to return None.

If you call Get BoundingBox you normally expect a dictionary or number and not None.

i am fine adding an additional argument that enables „not failing“
something like:
allow_hidden=True which then returns None if not visible.

but by default i would still Fail with a proper error message.

@robinmackaij
Copy link

i would not like to return None.

If you call Get BoundingBox you normally expect a dictionary or number and not None.

i am fine adding an additional argument that enables „not failing“ something like: allow_hidden=True which then returns None if not visible.

but by default i would still Fail with a proper error message.

I don't think a None would be unexpected here, since locator.boundingBox() effectively does the same (returning null).

An empty dict would also work for me and would make the interface consistent.

An optional named argument to suppress any errors and return None instead is also fine. I'd be careful with the name allow_hidden though; there may be other conditions that will result in Playwright returning a null. Perhaps suppress_errors with default False.

@Snooz82
Copy link
Member

Snooz82 commented Oct 5, 2024

playwright states that null is returned if hidden.

element not found would be an error that we won’t suppress.

if you return an empty dict, users may access the x key at some point and fail much later.

we made these mistakes earlier and i won’t repeat.

we either return a valid position and size or fail.
regarding the name i am open to proposals but suppress_errors is too wide.

@robinmackaij
Copy link

playwright states that null is returned if hidden.

Looking back at the trace from the log we see that the problem is in getters.py in get_boundingbox: parsed = DotDict(data).

The data comes from getBoundingBox from the Playwright side, that can return null as a JSON string. So to me it seems like parsed = DotDict(data) is not taking the possible null / None value of data into account.

Since Playwright states that null is the returned value for getBoundingBox for hidden elements, I'd say the bug is actually that Browser's Get BoundingBox does not return None anymore for hidden elements (as it has done in the past). In other words: a regression issue.

element not found would be an error that we won’t suppress.

Of course, that would fail on Playwright's side in const locator = await findLocator(state2, selector, strictMode, void 0, true);.

if you return an empty dict, users may access the x key at some point and fail much later.

we made these mistakes earlier and i won’t repeat.

I can see that.

we either return a valid position and size or fail.

What my issue is with that approach is that it's different from Playwright's approach. Playwright returns a dict with the coordinates or null (in case of hidden elements).

Of course "valid value or fail" would be justified if all the keywords in Browser library do check for null responses from Playwright and raise / fail in that case. Behavior should be consistent throughout the library.

regarding the name i am open to proposals but suppress_errors is too wide.

allow_none_result perhaps? Should be clear also on the Python side (pseudocode):

if data is None and self.allow_none_result:
    return None
parsed = DoctDict(data)
...

@Snooz82
Copy link
Member

Snooz82 commented Oct 5, 2024

i do agree that this is a regression. before we did anything with the result, it would have worked.

we do not have the policy to behave same as Playwright. So no need to return None.

in Robot Framework you typically do expect to get always the same. we did for example had keywords where we sometimes returned just a single value and sometimes a list of values, which make the code unnecessarily complex when using the keywords.

allow_none_results is imho too technical.

@Snooz82
Copy link
Member

Snooz82 commented Oct 5, 2024

@robinmackaij Playwright itself is imho not really consistent.
Examples:

  • inputValue throws if the selected element is not an input, textarea or select
  • isChecked throws if the element is not a checkbox or radio input

So as i would expect, if that call is not really applicable to the selected element the keyword shall fail.
As for getting the location and size of an invisible element.

Snooz82 added a commit that referenced this issue Oct 6, 2024
Snooz82 added a commit that referenced this issue Oct 6, 2024
@Snooz82 Snooz82 linked a pull request Oct 6, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority: high
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants