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 | 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 | 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 | Review

Description 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 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 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 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 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 Henrik Skupin (:whimboo) 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 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 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 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 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 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 Henrik Skupin (:whimboo) 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 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 Ryan VanderMeulen [:RyanVM] 2013-01-23 08:43:32 PST
https://hg.mozilla.org/mozilla-central/rev/742adca7ab38
Comment 14 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 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 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 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.