901781 - enable visibility tests on tbpl

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[David Burns :automatedtester]

The visibility tests are not enabled and this is bad...

Let's enable it!

Comment 1

[David Burns :automatedtester]

Attachment: bug901781.patch

Try pushes

https://tbpl.mozilla.org/?tree=Try&rev=c25bd7b5ed5c
https://tbpl.mozilla.org/?tree=Try&rev=f259821c1ed2

Comment 2

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 787188 [details] [diff] [review]
bug901781.patch

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

::: testing/marionette/marionette-listener.js
@@ +631,5 @@
> +                  right:(curWindow.pageXOffset + curWindow.innerWidth)};
> +  return (viewPort.left <= rect.right + curWindow.pageXOffset &&
> +          rect.left + curWindow.pageXOffset <= viewPort.right &&
> +          viewPort.top <= rect.bottom + curWindow.pageYOffset &&
> +          rect.top + curWindow.pageYOffset <= viewPort.bottom);

This algorithm returns true only if the element is *fully* in the viewport. If the element is mostly (say top, left and right) are in the viewport but say some of its bottom pixels are out of the viewport, this algorithm will consider it as out of the viewport, whereas the previous algorithm was stronger and would detect that it is still within the viewport. 

Has the definition of 'in the viewport' changed, or is this a mistake?

Comment 3

[David Burns :automatedtester]

This does handle the case. I have added tests to show it working since each element is off the viewport for half the element.

Comment 4

[David Burns :automatedtester]

Attachment: bug901781-v2.patch

Comment 5

[David Burns :automatedtester]

try 

https://tbpl.mozilla.org/?tree=Try&rev=b38438f5003d
https://tbpl.mozilla.org/?tree=Try&rev=820d2a0ffa4a

Comment 6

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 788406 [details] [diff] [review]
bug901781-v2.patch

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

I must have misread it because this makes tons of sense, but a .rej file was committed. You can use By if you want in the tests

Comment 7

[David Burns :automatedtester]

just realised that the ini update has been dropped redoing try

https://tbpl.mozilla.org/?tree=Try&rev=d9c37e1c25bf
https://tbpl.mozilla.org/?tree=Try&rev=8554d389e3e6

Comment 8

[David Burns :automatedtester]

Attachment: bug901781-v3.patch

Just noticed that the ini part of the patch was dropped so redoing try, carrying forward the r+ for the update of algorithm

Comment 9

[David Burns :automatedtester]

landed in https://hg.mozilla.org/integration/mozilla-inbound/rev/342c800534ab after rebasing

Comment 10

[David Burns :automatedtester]

Attachment: bug901781-v4.patch

carrying r+ forward

Comment 11

[David Burns :automatedtester]

please uplift to b2g trees awesome sheriffs!

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/342c800534ab

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/5d90dd226ff3

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/5d90dd226ff3