Make the python abstraction layer check if the element is visible before acting on it

RESOLVED FIXED

Status

Testing
Marionette
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mdas, Assigned: mdas)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

If you tap() a hidden element, it will still try to dispatch the event, when we should be throwing an ElementNotVisible error.
A quick win and to be honest the best solution until we have done the proper event dispatching is to call element.is_displayed() before each of the execute_script() calls.
Created attachment 704674 [details] [diff] [review]
patch v1.0

So I'd like to add the status/messages that are used in Selenium to errors.py as defaults for each particular Exception, unless you prefer otherwise. I think it'll be cleaner this way. I'm just adding the one ElementNotVisible one for now since that's the only one that's needed for this patch, but I can create a bug to deal with that. It'll take some manual labor to find all the strings needed from Selenium.
Assignee: nobody → mdas
Attachment #704674 - Flags: review?(jgriffin)
Created attachment 704680 [details] [diff] [review]
patch v1.1

More robust changes to errors.py
Attachment #704674 - Attachment is obsolete: true
Attachment #704674 - Flags: review?(jgriffin)
Attachment #704680 - Flags: review?(jgriffin)
(In reply to David Burns :automatedtester from comment #1)
> A quick win and to be honest the best solution until we have done the proper
> event dispatching is to call element.is_displayed() before each of the
> execute_script() calls.

Whoa didn't see your comment there, heh. Fortunately, that's what this patch is doing, but I have to make sure it's integrated server-side for Bug 750271. I've added a note on that bug about it.
What happens if the element is out of the view port and you would have to scroll down first? There is a `scrollIntoView()` method around for this purpose.
Comment on attachment 704680 [details] [diff] [review]
patch v1.1

Review of attachment 704680 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm, maybe Yiming can help with the rest of the error codes.
Attachment #704680 - Flags: review?(jgriffin) → review+
(In reply to Henrik Skupin (:whimboo) from comment #5)
> What happens if the element is out of the view port and you would have to
> scroll down first? There is a `scrollIntoView()` method around for this
> purpose.

According to dburns, is_displayed will return if it is out of the viewport, but still scrollable. It won't necessarily scroll it into view though.
(In reply to Malini Das [:mdas] from comment #7)
> (In reply to Henrik Skupin (:whimboo) from comment #5)
> > What happens if the element is out of the view port and you would have to
> > scroll down first? There is a `scrollIntoView()` method around for this
> > purpose.
> 
> According to dburns, is_displayed will return if it is out of the viewport,
> but still scrollable. It won't necessarily scroll it into view though.

And according to https://dvcs.w3.org/hg/webdriver/raw-file/tip/webdriver-spec.html#clicking, we should scroll to the element before clicking on it, so I'm going to use the same idea here and scroll to the element before we do any actions on it.
(In reply to Malini Das [:mdas] from comment #8)
> And according to
> https://dvcs.w3.org/hg/webdriver/raw-file/tip/webdriver-spec.html#clicking,
> we should scroll to the element before clicking on it, so I'm going to use
> the same idea here and scroll to the element before we do any actions on it.

Scratch that, it's not as easy as I'd like. I've opened https://bugzilla.mozilla.org/show_bug.cgi?id=833370 to address this. In the meantime, we can rely on is_displayed to check if the element is visible, even if it has to be scrolled to.
Created attachment 704907 [details] [diff] [review]
patch v1.2

I forgot to up the python package version *again*. Moving r+ forward.
Attachment #704680 - Attachment is obsolete: true
Attachment #704907 - Flags: review+
(In reply to Malini Das [:mdas] from comment #9)
> https://bugzilla.mozilla.org/show_bug.cgi?id=833370 to address this. In the
> meantime, we can rely on is_displayed to check if the element is visible,
> even if it has to be scrolled to.

Just keep in mind that we could see failures when acting on elements which are visible but outside of the viewport.
Status: NEW → ASSIGNED
landed on m-i:

https://hg.mozilla.org/integration/mozilla-inbound/rev/742adca7ab38

will push to pypi and b2g18 if it lands on m-c
Whiteboard: [leave-open]
https://hg.mozilla.org/mozilla-central/rev/742adca7ab38
uploaded to pypi, but I haven't landed on b2g18 yet due to some conflicts. I'd like to update all the necessary marionette stuff on b2g18 before landing my patch
Not going to land in b2g18. This isn't urgently needed in b2g18, so I won't pursue getting the tef+ needed to land.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [leave-open]
https://hg.mozilla.org/releases/mozilla-b2g18/pushloghtml?changeset=71f7f42dee13
status-b2g18: --- → fixed
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/400c689f0a58
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → fixed
You need to log in before you can comment on or make changes to this bug.