-
Notifications
You must be signed in to change notification settings - Fork 95
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
Consider etaoin.api/displayed?
behavior
#444
Comments
Thanks so much @daveyarwood! Personally? No idea! And I don't know what that jwp comment means either: Line 2145 in e4d60c8
Not sure if this helps but here's what the w3c webdriver spec says on the topic. PR most welcome that includes tests. |
From the W3C spec you linked:
This definition is confusing with respect to I'm open to other perspectives on this, but I think it makes sense to take a pragmatic approach here and use the visibility and display attributes instead of the I'll work on a PR for this. |
Yeah I guess the fact that they didn't define it in their API is a sign that it was debatable or contentious? I don't know. Our docstring can describe what |
Hiya @daveyarwood, I'm learning that we might have been a bit hasty on this one. I'm ramping up on Etaoin by going over docs and docstrings and tying as much as I can. Here's something I happened to try: <ul>
<li><a href="#">link 1</a></li>
<li><a href="#" class="active">link 2 (active)</a></li>
<li><a href="https://clojure.org">link 3 (clojure.org)</a></li>
<li style="display:none"><a id="cantseeme" href="#">link 4 (not visible)</a></li>
</ul> With the current release of Etaoin (v0.4.6), I get what seems correct to me: (e/displayed? driver :cantseeme)
;; => false But with our agreed on change on master I see what seems incorrect to me: (e/displayed? driver :cantseeme)
;; => true Watcha think, should we re-open this issue? |
Ah, yeah, that seems like a bug. It looks like the issue there is that the element itself is not hidden by |
We can sleep on it for a bit and figure out what we want to do. |
I don't know if I will have time to work on this soon, but just off-hand, to suggest an approach: if there is a way to enumerate the ancestors of an element, we could recursively check whether each element is displayed until we reach the top and there are no more ancestors to check. This is probably not good from a performance perspective, but if it's acceptable performance-wise, the correctness might be worth it. |
Another idea: generally use the browser-provided |
Cool, if you are short on time, no pressure to work on it @daveyarwood, I'm sure we'll figure something out. |
I've done minor poking around the web. Here's what the Selenium folks say in their docs:
So this can get maybe complex. This selenium unit test can maybe give us some ideas of what we want to think about. Seems like they might cover:
I'm thinking that we should do the minimum that makes sense for us. |
I've marked this both a bug and a feature. |
I'm running into trouble now as a result of my change in #445. I have a UI test case where I click a button and I expect an element with a particular ID to become invisible. The trouble is: while that element does become invisible, it's because the In order to get my UI tests passing again, I'd love to do something about this if possible, even if it isn't an ideal long-term solution. I played around with this for a while just now by adding a test case for a nested inner div that should be effectively invisible because its parent is invisible: <div id="hidden-outer-div" style="display: none;">
<div id="hidden-inner-div">This is effectively hidden</div>
</div> (-> (e/invisible? {:id :hidden-outer-div}) is)
(-> (e/invisible? {:id :hidden-inner-div}) is) As expected, the If I revert my change, then the hidden But I was able to get all of the If you're on board with this as a short term fix, I'd be happy to raise a PR! Let me know your thoughts. |
Heya @daveyarwood thanks for following up! After looking at Selenium's approach, I've learned that the definition of "displayed" can include a lot of perspectives! I think for us, for now, we can stick to our current approach of CSS controlled visibility via This means we would not (at least initially) include:
What are our options for implementing the parent elements check?
Whatever approach we choose, we should make sure it works across all of our supported browsers: chrome, firefox, edge and safari. |
I agree. The browser native I haven't started looking into approaches for checking the ancestors of an element. I think that would be a better fix for the immediate issue, but in the short term, are you open to a "band-aid" level fix like the one I mentioned in my last comment? I have the fix ready and could make a PR soon. |
@daveyarwood I think I'd rather we take a stab at deciding what behaviour we think makes good general sense and implement that. If we get it right, users of Etaoin would not have to adapt to changes we make in this area. |
Alright, that sounds reasonable to me 👍 Unfortunately, I don't think I have the bandwidth to work on a more robust fix myself, but I'm happy to continue the conversation here about approaches if that would be helpful. |
Yeah, your continued feedback is very much appreciated! |
Juuust in case you might be interested in my hotfix 🙂 , I've pushed it to a branch, and you can see the diff here: master...daveyarwood:etaoin:pragmatic-displayed-impl-v2 The change is pretty small -- just reintroducing the browser native At the moment, I'm having to switch the repo we use at work to point at my personal fork instead of this repo, so that we can get our UI test cases to pass again. If we could get this change into etaoin master, then I could at least get us pointed at the proper Etaoin repo again, which would be helpful for us. Better still will be when we have a robust Thoughts? |
Thanks @daveyarwood, if we were confident your hotfix was on the long-term path we wanted to take, I'd be all for it. I'll take a look at this issue sometime soon. |
I'm attempting to learn what WebDriver implementations do today wrt displayed-ness. Can confirm that as of this writing, It seems that (Far as I can tell, I do see a note in the Selenium source expressly stating that
Why an expressly hidden I'm also poking around to see differences between WebDriver implementation behaviors. And I'm sure others have grappled with this issue, so far I noticed: |
I've just verified that Selenium itself considers <select id="visibility">
<option value="regular" class="regular">Regular</option>
<option value="disabled" disabled="disabled" class="disabled">Disabled</option>
<option value="hidden" style="visibility: hidden;" class="hidden">Hidden</option>
<option value="invisible" style="display: none;" class="invisible">Invisible</option>
</select> (source) My hacky little Java app to verify (the package com.dlread.seleniumplay;
import org.openqa.selenium.By;
import org.openqa.selenium.WebDriver;
import org.openqa.selenium.chrome.ChromeDriver;
public class Play {
public static void main (String args[]) {
WebDriver driver = new ChromeDriver();
driver.get("file:///Users/lee/proj/oss/SeleniumHQ/selenium/common/src/web/selectPage.html");
System.out.println ("regular displayed: " + driver.findElement(By.cssSelector("[class='regular']")).isDisplayed());
System.out.println ("hidden displayed: " + driver.findElement(By.cssSelector("[class='hidden']")).isDisplayed());
System.out.println ("invisible displayed: " + driver.findElement(By.cssSelector("[class='invisible']")).isDisplayed());
System.out.println ("transparent displayed: " + driver.findElement(By.id("transparent")).isDisplayed());
driver.close();
}
} When run, outputs:
I still have not dug up the rationale for this behavior, but I'm sure my aha moment is coming. |
I've asked the question on the selenium user group: Hopefully, someone will help us out with an explanation. |
No answers to my question on the Selenium User's Group yet. I've done some more digging and my (perhaps flawed) understanding is this:
So what shall we do? The W3C WebDriver spec recommends (but does not require) that a WebDriver implement displayed-ness by calling the Selenium's JavaScript atom (without ignoring opaqueness styling BTW) and expose the result on the Where are we at now?Etaoin implemented a naive @daveyarwood noticed the oddity with the We then realized that the safari implementation was more naive than we'd like. Where do we go?[rejected] Option 1: Do nothingLeave our naive displayed behavior change as is. Option 2: Reverse new naive displayed implementationContinue to delegate to the Option 3: Automatically tweak new naive displayed implementation to special case option elements.Call I think this is your most recent proposal, @daveyarwood. Option 4: Call the Selenium atomDo what the W3C WebDriver spec recommends WebDriver implementations do, and call the Selenium JavaScript atom to determine displayed-ness. If I've understood correctly, this is what Selenium does. (To look into: other WebDriver implementation operations might rely internally on displayed-ness. We do want to remain a lightweight wrapper atop W3C WebDriver. If we have one definition of displayed-ness and a WebDriver implementation has a different definition, this could get confusing. I expect that Selenium wraps displayed-ness checks in all the right places for consistency?). But this does not resolve the raised issue. Sub-options Option ? |
Heya @daveyarwood, I've been letting this issue block a release. I'll likely roll our current changes to |
Sounds good to me. I'm all for not blocking a release! |
etaoin.api/displayed?
returns incorrect results for Chrome and Firefoxetaoin.api/displayed?
behavior
etaoin.api/displayed?
behavioretaoin.api/displayed?
behavior
We have learned that displayed-ness is more complex than we had originally imagined. It requires more research and maybe more hammock time. Effectively rolls back clj-commons#455 for issue clj-commons#444.
We have learned that displayed-ness is more complex than we had originally imagined. It requires more research and maybe more hammock time. Effectively rolls back PR clj-commons#445 for issue clj-commons#444.
I noticed that
displayed?
returns true for an invisibleoption
element with styledisplay: none
.I looked at the code a little bit, and it seems like the issue might be that the
displayed
value is not correct.I noticed in the code that there is a custom implementation for Safari because Safari does not have a native
displayed
implementation. I tried using that implementation for Chrome and Firefox and it works as expected.I might be lacking the full context here, but it seems like this custom implementation is more reliable, and Etaoin should just use that for all browsers. If you're interested, I could put together a PR to do that. But I'm also curious if you have any other thoughts about this.
The text was updated successfully, but these errors were encountered: