Last Comment Bug 833079 - Make the python abstraction layer check if the element is visible before acting on it
: Make the python abstraction layer check if the element is visible before acti...
Status: RESOLVED FIXED
:
Product: Testing
Classification: Components
Component: Marionette (show other bugs)
: Trunk
: All All
-- normal (vote)
: ---
Assigned To: Malini Das [:mdas] - Away, not checking bugmail
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-01-21 13:15 PST by Malini Das [:mdas] - Away, not checking bugmail
Modified: 2013-05-01 18:40 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
wontfix
fixed


Attachments
patch v1.0 (2.60 KB, patch)
2013-01-21 13:43 PST, Malini Das [:mdas] - Away, not checking bugmail
no flags Details | Diff | Splinter Review
patch v1.1 (4.63 KB, patch)
2013-01-21 14:03 PST, Malini Das [:mdas] - Away, not checking bugmail
jgriffin: review+
Details | Diff | Splinter Review
patch v1.2 (4.95 KB, patch)
2013-01-22 08:45 PST, Malini Das [:mdas] - Away, not checking bugmail
malini: review+
Details | Diff | Splinter Review

Description User image Malini Das [:mdas] - Away, not checking bugmail 2013-01-21 13:15:03 PST
If you tap() a hidden element, it will still try to dispatch the event, when we should be throwing an ElementNotVisible error.
Comment 1 User image David Burns :automatedtester 2013-01-21 13:33:43 PST
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.
Comment 2 User image Malini Das [:mdas] - Away, not checking bugmail 2013-01-21 13:43:53 PST
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.
Comment 3 User image Malini Das [:mdas] - Away, not checking bugmail 2013-01-21 14:03:34 PST
Created attachment 704680 [details] [diff] [review]
patch v1.1

More robust changes to errors.py
Comment 4 User image Malini Das [:mdas] - Away, not checking bugmail 2013-01-21 14:08:18 PST
(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.
Comment 5 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2013-01-21 15:41:29 PST
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 6 User image Jonathan Griffin (:jgriffin) 2013-01-21 16:51:00 PST
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.
Comment 7 User image Malini Das [:mdas] - Away, not checking bugmail 2013-01-22 07:45:17 PST
(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.
Comment 8 User image Malini Das [:mdas] - Away, not checking bugmail 2013-01-22 08:04:50 PST
(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.
Comment 9 User image Malini Das [:mdas] - Away, not checking bugmail 2013-01-22 08:27:59 PST
(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.
Comment 10 User image Malini Das [:mdas] - Away, not checking bugmail 2013-01-22 08:45:22 PST
Created attachment 704907 [details] [diff] [review]
patch v1.2

I forgot to up the python package version *again*. Moving r+ forward.
Comment 11 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2013-01-22 08:51:03 PST
(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.
Comment 12 User image Malini Das [:mdas] - Away, not checking bugmail 2013-01-22 13:15:16 PST
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
Comment 13 User image Ryan VanderMeulen [:RyanVM] 2013-01-23 08:43:32 PST
https://hg.mozilla.org/mozilla-central/rev/742adca7ab38
Comment 14 User image Malini Das [:mdas] - Away, not checking bugmail 2013-01-23 12:27:48 PST
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
Comment 15 User image Malini Das [:mdas] - Away, not checking bugmail 2013-01-23 12:36:24 PST
Not going to land in b2g18. This isn't urgently needed in b2g18, so I won't pursue getting the tef+ needed to land.
Comment 16 User image Malini Das [:mdas] - Away, not checking bugmail 2013-04-02 11:43:41 PDT
https://hg.mozilla.org/releases/mozilla-b2g18/pushloghtml?changeset=71f7f42dee13
Comment 17 User image Jonathan Griffin (:jgriffin) 2013-05-01 18:40:18 PDT
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/400c689f0a58

Note You need to log in before you can comment on or make changes to this bug.