search

mozilla / geckodriver

WebDriver for Firefox
https://firefox-source-docs.mozilla.org/testing/geckodriver/
Mozilla Public License 2.0
7.12k stars 1.52k forks source link

<element hidden> that gets revealed due to display: flex are incorrectly reported as hidden #864

Closed APshenkin closed 6 years ago

APshenkin commented 7 years ago

System

Testcase

Fill login/password fields https://mail.rambler.ru/

These fields have attribute hidden=""

When try to clear field everything is ok, but if we try to send keys to it, we get exeption

Stacktrace

Session ID: e92d757f-e636-1d49-83d1-85d961b0696a
    at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
    at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
    at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
    at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
    at org.openqa.selenium.remote.http.W3CHttpResponseCodec.createException(W3CHttpResponseCodec.java:150)
    at org.openqa.selenium.remote.http.W3CHttpResponseCodec.decode(W3CHttpResponseCodec.java:115)
    at org.openqa.selenium.remote.http.W3CHttpResponseCodec.decode(W3CHttpResponseCodec.java:45)
    at org.openqa.selenium.remote.HttpCommandExecutor.execute(HttpCommandExecutor.java:164)
    at org.openqa.selenium.remote.service.DriverCommandExecutor.execute(DriverCommandExecutor.java:82)
    at org.openqa.selenium.remote.RemoteWebDriver.execute(RemoteWebDriver.java:637)
    at org.openqa.selenium.remote.RemoteWebElement.execute(RemoteWebElement.java:272)
    at org.openqa.selenium.remote.RemoteWebElement.sendKeys(RemoteWebElement.java:96)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:498)
    at org.openqa.selenium.support.events.EventFiringWebDriver$EventFiringWebElement$1.invoke(EventFiringWebDriver.java:332)
    at com.sun.proxy.$Proxy5.sendKeys(Unknown Source)
    at org.openqa.selenium.support.events.EventFiringWebDriver$EventFiringWebElement.sendKeys(EventFiringWebDriver.java:355)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:498)
    at org.openqa.selenium.remote.server.KnownElements$1.invoke(KnownElements.java:64)
    at com.sun.proxy.$Proxy6.sendKeys(Unknown Source)
    at org.openqa.selenium.remote.server.handler.SendKeys.call(SendKeys.java:50)
    at org.openqa.selenium.remote.server.handler.SendKeys.call(SendKeys.java:29)
    at java.util.concurrent.FutureTask.run(FutureTask.java:266)
    at org.openqa.selenium.remote.server.DefaultSession$1.run(DefaultSession.java:176)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
    at java.lang.Thread.run(Thread.java:745)
14:39:16.419 WARN - Exception: Element is not visible
twalpole commented 7 years ago

The hidden attribute is a boolean attribute, which means the value of it doesn't matter. If the attribute exists on the element the element should not be shown on the screen UNLESS the display style is changed. https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/hidden - "Note: Changing the value of the CSS display property on an element with the hidden attribute overrides the behavior. For instance, elements styled display: flex will be displayed despite the hidden attribute's presence."

I believe the issue here is the atom used by marionette for testing whether or not an element is displayed skips the "UNLESS" part of that definition - https://hg.mozilla.org/mozilla-central/file/tip/testing/marionette/atom.js#l149 . Of course, the second issue is why that website would use the hidden attribute on elements when it's not doing anything.

Note: The 'hidden' attribute check was actually removed from the selenium atom (which I believe is where the marionette one came from) quite some time ago - https://github.com/SeleniumHQ/selenium/commit/5b60eb950acc9b265513c6ad411f12f675aef8b4

APshenkin commented 7 years ago

FYI, I can fill these fields with chromedriver and edgedriver properly. Yes, I also think that this is a bug on website, but I believe that geckodriver have to fill fields like these.

whimboo commented 7 years ago

@andreastt would this be related to our planned update of the Selenium atoms in Marionette?

andreastt commented 7 years ago

We should not be using the Selenium atom for determining whether <input type=hidden> is interactable or not. According to the specification we should look at the element’s keyboard interactability, which is defined thus:

A keyboard-interactable element is any element that has a focusable area, is a body element, or is the document element.

I remember trying to read up on exactly what this means a while back, and I remember thinking it was a very complicated algorithm to implement. Further investigation is needed to make the Element Send Keys command in Marionette conformant to the specification.

In any case, I’m quite sure that <input type=hidden> is not supposed to be covered by that, and certainly there are no special provisions in the specification that cover hidden input fields. To set the value of such a field you need to use the Execute Script command.

twalpole commented 7 years ago

@andreastt This issue is not for <input type=hidden> -- this is for any input element with a hidden attribute - https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/hidden . Please reopen and read my above comment.

andreastt commented 7 years ago

Element Send Keys in Marionette is currently not conforming to the specification at all. I would not like to spend time working on Selenium compatibility as it is not the goal of geckodriver.

twalpole commented 7 years ago

@andreastt This really isn't about Send Keys, the underlying issue is that geckodriver/marionette reports elements with a hidden attribute as non-displayed even when they are displayed.

andreastt commented 7 years ago

@twalpole I see. That is totally not obvious from the either the description or the issue title. I’ve updated the title to reflect that <p hidden style="display: block">foo</p> is meant to be reported as displayed.

I suspect, however, that this will get resolved when we move to implementing the keyboard-interactable definition in WebDriver (for Element Send Keys) and the pointer-interactable definition for Element Click (tracked in https://bugzilla.mozilla.org/show_bug.cgi?id=1321516).

a1dutch commented 7 years ago

we have been hit by this too with hidden style="display: block", any idea on a fix date?

akostadinov commented 7 years ago

FYI https://bugzilla.mozilla.org/show_bug.cgi?id=1321516 was resolved but didn't resolve this issue. At least my test described in #935 still doesn't work with 0.19.0 and moz:webdriverClick capability.

-- Firefox 57 beta4

andreastt commented 7 years ago

https://bugzilla.mozilla.org/show_bug.cgi?id=1321516 hasn’t been fixed yet, and it is related to clicking, not sending keys which this issue describes. Keyboard interactability is not exactly the same as pointer interactability, because there are a few additional scenarios by which you can reach an element by keyboard that you can’t by mouse.

akostadinov commented 7 years ago

bug 1321516 is complete according to the assignee when moz:webdriverClick is enabled in 0.19.0+. At least this is how I'm reading the comments.

I just tried to say that bug 1321516 doesn't seem expected to help so better not put our bets on it. Instead I hope somebody can pick up the hard task of correctly determine visibility of elements described here.

Please correct me if I'm wrong.

andreastt commented 7 years ago

What I mean is that pointer-interactability is not going to help in the case of keyboard input. That is still largely unfinished and unimplemented.

twalpole commented 7 years ago

I believe the reason https://bugzilla.mozilla.org/show_bug.cgi?id=1321516 doesn't help is because it still assumes the presence of a "hidden" attribute on an element means the element is non-visible - which it doesn't since it can be overridden by setting a non-default value for CSS display - https://hg.mozilla.org/mozilla-central/file/8e818b5e9b6b/testing/marionette/accessibility.js#l282 which is called from https://hg.mozilla.org/mozilla-central/file/8e818b5e9b6b/testing/marionette/accessibility.js#l305 which is called from https://hg.mozilla.org/mozilla-central/file/8e818b5e9b6b/testing/marionette/interaction.js#l245

I gathered this from just a quick reading of what I believe is the relevant code, so I may have missed something

whimboo commented 6 years ago

I don't understand this issue. Is the problem here related to the input#form_password element on https://mail.rambler.ru/? When I open the page with Firefox the input field is NOT hidden, and I can perfectly enter text. Why should Marionette fail?

twalpole commented 6 years ago

@whimboo It shouldn't fail, but it definitely used to. It failed because geckodriver/firefox assumed an element with a hidden attribute (which the password field has) was non-visible. That assumption is incorrect if the default display style has been overridden. It's possible the atom updates in the latest Firefox release fixed the issue, I haven't gone back and checked yet.

twalpole commented 6 years ago

@whimboo It is still broken with FF 57 - does appear to be fixed with FF 59.0a1

whimboo commented 6 years ago

Ah, I see. I run it locally and it worked with Nightly, so I was wondering. But that means that my patch which updated the Selenium atoms for Firefox 58 has fixed it (see https://bugzilla.mozilla.org/show_bug.cgi?id=1375660), and that also with webdriver compat clicks it passes too (which are enabled by default in 58).

Can you please try yourself again with 58?

twalpole commented 6 years ago

@whimboo Appears fixed with FF 58.0b4 too

whimboo commented 6 years ago

Great! It means we can close this issue. Thanks to everyone involved.

lock[bot] commented 5 years ago

This issue has been automatically locked since there has not been any recent activity after it was closed. If you have run into an issue you think is related, please open a new issue.