864684 - Prevent two way scrolling if it is not necessary

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[leo.bugzilla.gecko]

If the browser content is scrollable, every touch move event makes two way scrolling(scrollTop, scrollLeft) in BrowserElementPanning.js even though the content only can be scrolled in one way(vertically or horizontally).
So, prevent two way scrolling if it is not neccessary.

Comment 1

[Jinho Hwang [:jeffhwang]]

Attachment: Prevent two way scrolling if it is not neccessary

Comment 2

[Jinho Hwang [:jeffhwang]]

Gregor. I attached the patch as we talked during the work week in Madrid. 
Unfortunately, I couldn't remember his name correctly(we discussed this with). 
Ruben or Ruven.. 
So, Could you please add Ruben(please, correct me if I spell his name wrong) as cc on this? 
I'd like him to review the patch casue JS is new world for me so there might be better code for this.

Comment 3

[Gregor Wagner [:gwagner]]

Comment on attachment 740713 [details] [diff] [review]
Prevent two way scrolling if it is not neccessary

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

Thanks Jinho! We talked to Vivien about it.

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 740713 [details] [diff] [review]
Prevent two way scrolling if it is not neccessary

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

::: dom/browser-element/BrowserElementPanning.js
@@ +14,5 @@
>  Cu.import("resource://gre/modules/Geometry.jsm");
>  
>  var global = this;
> +var yScrollable;
> +var xScrollable;

Those should likely be part of the ContentPanning singleton instead of globals.

@@ +176,5 @@
> +       if(this.target.scrollHeight <= this.target.clientHeight)
> +          yScrollable = false;
> +       if(this.target.scrollWidth <= this.target.clientWidth)
> +          xScrollable = false;
> +    }

let target = this.target;
if (target) { 
  yScrollable = target.scrollHeight > target.clientHeight;
  xScrollable = target.scrollWidth > target.clientWidth;
}

Btw is it really possible for |targeT| to be null ?
If not the check is not needed.

@@ +386,5 @@
>  
>      function doScroll(node, delta) {
>        if (node instanceof Ci.nsIDOMHTMLElement) {
>          oldX = node.scrollLeft, oldY = node.scrollTop;
> +        if(xScrollable) {

if (xScrollable) {

@@ +390,5 @@
> +        if(xScrollable) {
> +           node.scrollLeft += delta.x;
> +           newX = node.scrollLeft;
> +        }
> +        if(yScrollable){

if (yScrollable) {

@@ +393,5 @@
> +        }
> +        if(yScrollable){
> +           node.scrollTop += delta.y;
> +           newY = node.scrollTop;
> +        }           

nit: extra white spaces after the bracket.

Comment 5

[Jinho Hwang [:jeffhwang]]

Attachment: edited according to Vivien's comments.

Comment 6

[Jinho Hwang [:jeffhwang]]

Thanks Vivien for the review. 
And I apologize for my fool memory.. couldn't remember your name :( hope you understand.

I upload the new patch file edited according to your comments. 
Could you check it again? and if it is okay with you, pls push it to the git repository. 
I think I don't have the right to access the repository yet.

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 741112 [details] [diff] [review]
edited according to Vivien's comments.

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

Close! Let's reask a review once you have fixed the 2 nits.

::: dom/browser-element/BrowserElementPanning.js
@@ +376,5 @@
>  
>      function doScroll(node, delta) {
>        if (node instanceof Ci.nsIDOMHTMLElement) {
>          oldX = node.scrollLeft, oldY = node.scrollTop;
> +        if(xScrollable) {

nit: if (xScrollable) {

@@ +380,5 @@
> +        if(xScrollable) {
> +           node.scrollLeft += delta.x;
> +           newX = node.scrollLeft;
> +        }
> +        if(yScrollable){

nit: if (yScrollable) {

Comment 8

[Jinho Hwang [:jeffhwang]]

Attachment: patch 1 : prevent unnecessary two way scrolling

Comment 9

[Jinho Hwang [:jeffhwang]]

Comment on attachment 741587 [details] [diff] [review]
patch 1 : prevent unnecessary two way scrolling

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

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

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to JinhoHwang from comment #9)
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): 
> User impact if declined: 
> Testing completed: 
> Risk to taking this patch (and alternatives if risky): 
> String or UUID changes made by this patch:

Please fill out the form request so we have information with which to base an assessment of uplift viability.  Also, this isn't on master yet & tested so we'll be waiting for that info as well.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/e3cecee622e3

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e3cecee622e3