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)
Tracking
(firefox20 wontfix, firefox21 wontfix, firefox22 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)
RESOLVED
FIXED
mozilla22
People
(Reporter: mdas, Assigned: annyang121)
References
Details
(Whiteboard: [NPOTB])
Attachments
(1 file, 1 obsolete file)
1.36 KB,
patch
|
mdas
:
review+
akeybl
:
approval-mozilla-b2g18+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•11 years ago
|
||
Attachment #720095 -
Flags: review?(mdas)
Reporter | ||
Comment 2•11 years ago
|
||
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-
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #720095 -
Attachment is obsolete: true
Attachment #720116 -
Flags: review?(mdas)
Reporter | ||
Updated•11 years ago
|
Attachment #720116 -
Flags: review?(mdas) → review+
Reporter | ||
Comment 4•11 years ago
|
||
landing on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/63c0a866a2ae
Reporter | ||
Comment 5•11 years ago
|
||
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?
Comment 6•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/63c0a866a2ae
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Updated•11 years ago
|
Whiteboard: [NPOTB]
Updated•11 years ago
|
Attachment #720116 -
Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Comment 7•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18/rev/5ba282182901
status-b2g18:
--- → fixed
status-b2g18-v1.0.0:
--- → wontfix
status-b2g18-v1.0.1:
--- → wontfix
status-firefox20:
--- → wontfix
status-firefox21:
--- → wontfix
status-firefox22:
--- → fixed
Updated•1 year ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•