Closed Bug 864684 Opened 6 years ago Closed 6 years ago

Prevent two way scrolling if it is not necessary

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: leo.bugzilla.gecko, Assigned: jeffhwang)

Details

(Whiteboard: [fixed-in-birch])

Attachments

(1 file, 2 obsolete files)

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.
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 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.
Attachment #740713 - Flags: review?(21)
Assignee: nobody → faraojh
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.
Attachment #740713 - Flags: review?(21)
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.
Attachment #741112 - Flags: review?(21)
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) {
Attachment #741112 - Flags: review?(21)
Attachment #740713 - Attachment is obsolete: true
Attachment #741112 - Attachment is obsolete: true
Attachment #741587 - Flags: review?(21)
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:
Attachment #741587 - Flags: approval-mozilla-b2g18?
(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.
Flags: needinfo?(faraojh)
Summary: Prevent two way scrolling if it is not neccessary → Prevent two way scrolling if it is not necessary
https://hg.mozilla.org/mozilla-central/rev/e3cecee622e3
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Attachment #741587 - Flags: approval-mozilla-b2g18?
Flags: needinfo?(faraojh)
You need to log in before you can comment on or make changes to this bug.