Closed Bug 846867 Opened 11 years ago Closed 11 years ago

checkVisible() should call scrollIntoView() only if the element is not yet in the viewport

Categories

(Remote Protocol :: Marionette, defect)

x86
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(firefox20 wontfix, firefox21 wontfix, firefox22 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

RESOLVED FIXED
mozilla22
Tracking Status
firefox20 --- wontfix
firefox21 --- wontfix
firefox22 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: mdas, Assigned: annyang121)

References

Details

(Whiteboard: [NPOTB])

Attachments

(1 file, 1 obsolete file)

Due to Bug 846834 and Bug 796422, we should avoid calling scrollIntoView() unless necessary in b2g. It doesn't seem necessary to call scrollIntoView() on an element that's already in the viewport, so we should only call it if the element is not in the viewport. Right now, we call it regardless.
Attached patch changeScrollFeature (obsolete) — Splinter Review
Attachment #720095 - Flags: review?(mdas)
Comment on attachment 720095 [details] [diff] [review]
changeScrollFeature

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

::: testing/marionette/marionette-listener.js
@@ +729,5 @@
>      if (!visible) {
>        return false;
>      }
> +    let inViewport = elementInViewport(el);
> +    if (!inViewport) {

you can just do

if (elementInViewport(el)) {

@@ +737,5 @@
> +      }
> +      inViewport = elementInViewport(el);
> +      if (!inViewport) {
> +        return false;
> +      }

you can change if () here too.

Also, this if check should be moved into the above block. If scrollIntoView isn't callable, then we don't need to check if the element is in the viewport, since we know it is not.
Attachment #720095 - Flags: review?(mdas) → review-
Attached patch styleChangeSplinter Review
Attachment #720095 - Attachment is obsolete: true
Attachment #720116 - Flags: review?(mdas)
Attachment #720116 - Flags: review?(mdas) → review+
Comment on attachment 720116 [details] [diff] [review]
styleChange

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: no direct user impact
Testing completed: 
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none

Provided that Bug 839943 lands, we'll need this patch since it steps around an existing issue in B2G and lets the action chains run smoothly.
Attachment #720116 - Flags: approval-mozilla-b2g18?
https://hg.mozilla.org/mozilla-central/rev/63c0a866a2ae
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Whiteboard: [NPOTB]
Attachment #720116 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: