Closed Bug 901781 Opened 7 years ago Closed 7 years ago

enable visibility tests on tbpl

Categories

(Testing :: Marionette, defect)

x86
macOS
defect
Not set

Tracking

(firefox24 wontfix, firefox25 wontfix, firefox26 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

RESOLVED FIXED
mozilla26
Tracking Status
firefox24 --- wontfix
firefox25 --- wontfix
firefox26 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed

People

(Reporter: automatedtester, Assigned: automatedtester)

References

Details

Attachments

(1 file, 3 obsolete files)

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

Let's enable it!
Assignee: nobody → dburns
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?
This does handle the case. I have added tests to show it working since each element is off the viewport for half the element.
Attached patch bug901781-v2.patch (obsolete) — Splinter Review
Attachment #787188 - Attachment is obsolete: true
Attachment #787188 - Flags: review?(mdas)
Attachment #788406 - Flags: review?(mdas)
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
Attachment #788406 - Flags: review?(mdas) → review+
Attached patch bug901781-v3.patch (obsolete) — Splinter Review
Just noticed that the ini part of the patch was dropped so redoing try, carrying forward the r+ for the update of algorithm
Attachment #788406 - Attachment is obsolete: true
Attachment #788426 - Flags: review+
carrying r+ forward
Attachment #788426 - Attachment is obsolete: true
Attachment #792728 - Flags: review+
please uplift to b2g trees awesome sheriffs!
Keywords: checkin-needed
Keywords: checkin-needed
Whiteboard: [checkin-needed-b2g18]
https://hg.mozilla.org/mozilla-central/rev/342c800534ab
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.