807990 - Reader mode: scrolling contents "a screenful at a time" is missing.

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Reader View, defect, P2)

Description

[Edward B]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/17.0 Firefox/17.0
Build ID: 20120927042010

Steps to reproduce:

Scrolled down the page of contents on display.  
Then tried different "gestures" to indicate move "forward" and backwards a screen-full at a time
+ tapping on right margin area of the display
+ swiping downward on the rhs edge of the screen
+ tapping near the bottom of the display
+ pressing the "down" volume button


Actual results:

Tapping the margins did nothing.  
Tapping near the bottom causes the reader mode menu to appear.
Scrolling down the page kept kinetically scrolling like a web-page.
Pressing the volume button caused the volume level to change...


Expected results:

a screen-full of content should have "scrolled" by, then stop, so that reading can commence from the previous screen-full of content.

Comment 1

[Ian Barlow (:ibarlow)]

While this was out of scope for our initial release, this is certainly an interaction we would like to explore -- whether it's a double tap on the screen, tilting the device to scroll, using the volume controls, or some combination of the above.

Comment 2

[Lucas Rocha (:lucasr)]

Ian, it seems to me that double-tap anywhere for "next page" is the best option here. Agree?

Comment 3

[Kartikaya Gupta (email:[e-mail])]

In addition to double-tap, maybe also hijack volume up/down for page up/down?

Comment 4

[Ian Barlow (:ibarlow)]

(In reply to Lucas Rocha (:lucasr) from comment #2)
> Ian, it seems to me that double-tap anywhere for "next page" is the best
> option here. Agree?

Yes, I'd like to try this. We'll have to do a little more fine tuning to our gestures in Reader though, since this could interfere with displaying the toolbar. I would suggest:

** Double tap (or tap) to scroll down a page height (or part of a page height) -- this will require some tuning to get the distance and speed right
** Hide the toolbar when moving your finger up, and reveal it again when moving your finger down. This will make for a consistent hide / show interaction that we are implementing for the title bar in bug 716403

> In addition to double-tap, maybe also hijack volume up/down for page up/down?

I would certainly like to try it. It may be a thing we need to pref off by default in Settings, but allow people to turn it on. I can imagine a case where someone is listening to music while reading, and becoming confused about why their volume control stopped working properly.

Comment 5

[Edward B]

(in reply to Ian Barlow (:ibarlow) comment #4)

>> In addition to double-tap, maybe also hijack volume up/down for page up/down?
>I would certainly like to try it. It may be a thing we need to pref off by default in 
>Settings, but allow people to turn it on. I can imagine a case where someone is listening to 
>music while reading, and becoming confused about why their volume control stopped working 
>properly.

Well, for this to work appropriately, there should be some audible pings distinct to Firefox for Android.  these pings will get the user's attention so (s)he can look at the notification toolbar and be informed that firefox has now hijaacked the volume controls for reading-mode scrolling purposes.  "To deactivate, go into settings --> ... " etc..

Comment 6

[Lucas Rocha (:lucasr)]

For whoever decides to take this bug:

You'll probably have to add extra event handlers to handle double taps in the Reader Mode page:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutReader.js#50
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutReader.js#155

Then it's just about adding a new method that starts an animation that scrolls the content by the window height minus some padding (to keep reading context). I guess that would be something using requestAnimationFrame().

Comment 7

[Edman Anjos]

Lucas, I would like to take this bug

Comment 8

[Lucas Rocha (:lucasr)]

(In reply to Edman Anjos from comment #7)
> Lucas, I would like to take this bug

Great! Have you got your development environment up and running? Join us on IRC (irc.mozilla.org, #mobile channel) so that we can get you started.

Comment 9

[Lucas Rocha (:lucasr)]

Edman, do you still plan to work on this bug? Need any help to get started?

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Hey there Edman, are you still intending to take a look at this bug? Just checking in!  Thanks!

Comment 11

[Lucas Rocha (:lucasr)]

Assigning this to capella for the UI hackathon.

Comment 12

[Varun Varshney]

Hello , is there anyone working proactively on this bug ? I'm a beginner and would like to try my hand on this. I have got my development environment up and running and have built fennec succesfully.

Comment 13

[Mark Capella [:capella]]

I was volunteered to look at it a while back but haven't really done anything ... you can feel free to run with it!

Comment 14

[Varun Varshney]

Okay then , I would like to take this bug. Can someone please provide me assisstance ?

Comment 15

[Varun Varshney]

There is an eBook reader app call Moon+ Reader. In that app :-

1) tapping on right margin scrolls to the next page.
2) tapping on left margin scrolls scrolls up to the previous page.
3) tapping in the center of the screen brings up the menu and title bar.
4) swiping up/down on the left left margin controls the brightness.

And as usual , swiping up/down on any other part of the screen scrolls the text up/down according to the speed of the swipe.

Is this behaviour suitable for the fennec reader too ?

Comment 16

[Varun Varshney]

Attachment: Screenshot of how the screen can be divided.

Comment 17

[Lucas Rocha (:lucasr)]

Varun, I'd just start with just the double-tap to scroll behaviour. Single tap should probably be kept as is i.e. toggle the options bar.

I'd probably stick the simplest possible interaction: double-tap anywhere to scroll down to next page. Scroll up is not that common to warrant its own shortcut.

Ian, thoughts?

Comment 18

[Ian Barlow (:ibarlow)]

Agreed, let's keep it simple by using double tap to advance.

Comment 19

[Lucas Rocha (:lucasr)]

Varun, do you need any further information to work on this bug?

Comment 20

[Varun Varshney]

Lucas , I'm a bit confused right now. How to add the handlers for the double-tap event and where to write the actual function for the double-tap to work ?

Comment 21

[Lucas Rocha (:lucasr)]

(In reply to Varun Varshney from comment #20)
> Lucas , I'm a bit confused right now. How to add the handlers for the
> double-tap event and where to write the actual function for the double-tap
> to work ?

I assume you already have a development environment up and running. You'll probably have to add one more event handler at:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutReader.js#50
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutReader.js#176

Then do a scroll animation in response to double taps.

Comment 22

[Kartikaya Gupta (email:[e-mail])]

Note that the Java code already sends an event to Gecko for double-taps, here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/JavaPanZoomController.java#1281

You should just need to listen for it here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutReader.js#31

and handle it here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutReader.js#176

Comment 23

[Varun Varshney]

So I add something like this to listen for the double-tap:

Services.obs.addObserver(this, "Gesture:DoubleTap", false);  ?

Comment 24

[Kartikaya Gupta (email:[e-mail])]

Yup, that looks right.

Comment 25

[Awad Mackie]

Attachment: double_tap_forward.patch

Hi, I'd like to pick this up if no-ones working on it anymore?

Attached a quick first attempt, not quite sure if what I've done with the viewport is right. On my test device (Nexus S) I can't get a DoubleTap event without setting the ZoomConstraints somehow. Any advice?

Comment 26

[Peter Retzer (:pretzer)]

Comment on attachment 8420461 [details] [diff] [review]
double_tap_forward.patch

Hi Awad, thanks for your work! When you upload a patch to Bugzilla you should make sure to request feedback or review. You do that by stetting one of those flags to '?' on the upload screen and entering a requestee (if you set the review flag you get a dropdown of suggested people). Without setting a flag patches can easily slip through without anyone noticing them.

Comment 27

[Lucas Rocha (:lucasr)]

Comment on attachment 8420461 [details] [diff] [review]
double_tap_forward.patch

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

Looking pretty good, it would be nice if the animated the scroll to the new position instead of just snapping. Maybe give it a try?

Comment 28

[Awad Mackie]

Attachment: double_tap_forward.patch

Added an animation, gave it interruptible 500ms to run for now.

Also, realised that some context after a scroll would be nice, so arbitrarily decided on 50px.

Comment 29

[Lucas Rocha (:lucasr)]

Comment on attachment 8424218 [details] [diff] [review]
double_tap_forward.patch

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

Looking good, still needs some tweaks. I tried the patch locally, I think the animation could be a little faster. I wonder why the animation doesn't feel very smooth given that you seem to be doing all the right things here (i.e. using requestAnimationFrame() and all).

::: mobile/android/chrome/content/aboutReader.js
@@ +239,5 @@
>      switch (aEvent.type) {
>        case "touchstart":
> +        if (this._scrollAnimateID) {
> +          this._win.cancelAnimationFrame(this._scrollAnimateID);
> +          this._scrollAnimateID = false;

Given that this is an ID, it shouldn't really take a boolean here. Assign it to 0 or simply delete the attribute here.

@@ +280,5 @@
> +    // Arbitary choice of innerHeight - 50 to give some context after scroll
> +    let startTimestamp = null,
> +      startY = this._win.scrollY,
> +      finalY = Math.min(startY + (this._win.innerHeight - 50), this._win.scrollMaxY),
> +      YOffset = finalY - this._win.scrollY;

nits:
We don't usually declare variables like this btw (with the comma separator).
YOffset -> yOffset
finalY -> endY
Replace this._win.scrollY with startY in the YOffset assignment.

@@ +282,5 @@
> +      startY = this._win.scrollY,
> +      finalY = Math.min(startY + (this._win.innerHeight - 50), this._win.scrollMaxY),
> +      YOffset = finalY - this._win.scrollY;
> +
> +    let boundAnimateScrollStep = (function animateScrollStep(currentTimestamp) {

nit: Parens are not needed here.

@@ +283,5 @@
> +      finalY = Math.min(startY + (this._win.innerHeight - 50), this._win.scrollMaxY),
> +      YOffset = finalY - this._win.scrollY;
> +
> +    let boundAnimateScrollStep = (function animateScrollStep(currentTimestamp) {
> +      if (startTimestamp === null) startTimestamp = currentTimestamp;

nit: Always brace if's. So:

if (...) {
   ...
}

@@ +286,5 @@
> +    let boundAnimateScrollStep = (function animateScrollStep(currentTimestamp) {
> +      if (startTimestamp === null) startTimestamp = currentTimestamp;
> +      let elapsed = currentTimestamp - startTimestamp;
> +      let progress = elapsed / duration;
> +      let newY = startY + Math.floor(progress * YOffset);

You should probably ensure newY doesn't go beyond finalY here i.e. use something like Math.min() here.

@@ +289,5 @@
> +      let progress = elapsed / duration;
> +      let newY = startY + Math.floor(progress * YOffset);
> +
> +      this._win.scrollTo(this._win.scrollX, newY);
> +      if ((elapsed < duration) && (newY < finalY) && this._scrollAnimateID)

Do you actually need to check time *and* newY here?

nit: always brace if's, no need for the extra parens in the expression.

Comment 30

[Awad Mackie]

Attachment: double_tap_forward.patch

Fixed the nits and a few other issues.

Check for newY before setting the callback again was to catch the case where newY went beyond finalY because of delayed callback, but that's caught by limiting progress instead now.

Also added a check to prevent any callbacks when at the bottom of the document already.

I've reduced the duration to 300ms instead and did a bit of testing regarding jankiness. It's less noticeable at this speed, but it seems to correspond to requestAnimationFrame being called less times (5/6 vs 11/12).

Noted this happening as soon as reader mode is opened and after a user scroll, other double-taps seem to be fine. Did some quick tests and if the bottom toolbar isn't showing at all its consistently called more times, but still not as much as a 'normal' double-tap.

Any idea if this is normal requestAnimationFrame behaviour or there's some kind of clamping in progress after certain kinds of user interaction?

Comment 31

[Lucas Rocha (:lucasr)]

Comment on attachment 8427406 [details] [diff] [review]
double_tap_forward.patch

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

This is looking pretty good. I tried the patch locally the new animation time does feel better. I'd like to try using an easing function on the animation but we can do this in a separate bug. Interested? :-)

Just fix the nits and request review. Should be ready to land then.

::: mobile/android/chrome/content/aboutReader.js
@@ +286,5 @@
> +    if (yOffset === 0) {
> +      return;
> +    }
> +
> +    let boundAnimateScrollStep = function animateScrollStep(currentTimestamp) {

nit: I'd go with just animateScrollStep() here. Just do:

let animateScrollStep = function(currentTimestamp) {
   ...
}.bind(this);

@@ +289,5 @@
> +
> +    let boundAnimateScrollStep = function animateScrollStep(currentTimestamp) {
> +      if (startTimestamp === null) {
> +        startTimestamp = currentTimestamp;
> +      }

nit: add empty line here.

Comment 32

[Awad Mackie]

Attachment: double_tap_forward.patch

Ah, forgot about that, naming anonymous functions is a pet peeve of mine.

Yep, happy to look at easing once this lands!

Comment 33

[Lucas Rocha (:lucasr)]

Comment on attachment 8427993 [details] [diff] [review]
double_tap_forward.patch

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

Great, thanks. Please add the checkin-needed tag.

Comment 34

[Lucas Rocha (:lucasr)]

Actually, before you add the checkin-needed flag, could you please a link to a local build so that ibarlow can have a look at it before landing?

Comment 35

[Awad Mackie]

Hope the apk is all you need: https://dl.dropboxusercontent.com/u/8037977/fennec-32.0a1.en-US.android-arm.apk

Comment 36

[Ian Barlow (:ibarlow)]

Thanks for the build! Couple comments. 

The scroll feels too fast, too linear, and not smooth enough. I might slow it down a bit and add some easing in and easing out to the animation curve.

I like the double tap to advance, though while using it I noticed that I immediately found myself trying to double tap in the upper part of the page to scroll back up. Is this something that can be worked into this bug? Or at least have do a test build to see how it feels?

Comment 37

[Awad Mackie]

Yep, I'm fine doing that in here. Will return with a patch and build!

Comment 38

[Awad Mackie]

Attachment: double_tap_forward.patch

Added some simple easing in/out, and put the duration back up to 500ms. Also added scrolling up by topping on the top half of the screen. 

Build available here again: https://dl.dropboxusercontent.com/u/8037977/fennec-32.0a1.en-US.android-arm.apk

Comment 39

[Lucas Rocha (:lucasr)]

Comment on attachment 8435361 [details] [diff] [review]
double_tap_forward.patch

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

(In reply to Awad Mackie from comment #38)
> Created attachment 8435361 [details] [diff] [review]
> double_tap_forward.patch
> 
> Added some simple easing in/out, and put the duration back up to 500ms. Also
> added scrolling up by topping on the top half of the screen. 

This is looking great, thanks! The only issue I found is that the toolbar shows up when you double-tap to go up. We should only avoid toggling the toolbar when scrolling from a double-tap. Ian, thoughts?

Comment 40

[Ian Barlow (:ibarlow)]

(In reply to Awad Mackie from comment #38)

> Build available here again:
> https://dl.dropboxusercontent.com/u/8037977/fennec-32.0a1.en-US.android-arm.
> apk

Oops, sorry for the delay in looking at this build. This looks awesome!

(In reply to Lucas Rocha (:lucasr) from comment #39)

> This is looking great, thanks! The only issue I found is that the toolbar
> shows up when you double-tap to go up. We should only avoid toggling the
> toolbar when scrolling from a double-tap. Ian, thoughts?

Yep that would be my only comment too. 

How does the scrolling feel to you guys? It seems a *little* bit jankier when I double tap than when I am just scrolling with my thumb, but I can't really figure out why. 

Overall this looks great though :)

Comment 41

[Lucas Rocha (:lucasr)]

(In reply to Ian Barlow (:ibarlow) from comment #40)
> (In reply to Awad Mackie from comment #38)
> 
> > Build available here again:
> > https://dl.dropboxusercontent.com/u/8037977/fennec-32.0a1.en-US.android-arm.
> > apk
> 
> Oops, sorry for the delay in looking at this build. This looks awesome!
> 
> (In reply to Lucas Rocha (:lucasr) from comment #39)
> 
> > This is looking great, thanks! The only issue I found is that the toolbar
> > shows up when you double-tap to go up. We should only avoid toggling the
> > toolbar when scrolling from a double-tap. Ian, thoughts?
> 
> Yep that would be my only comment too. 
> 
> How does the scrolling feel to you guys? It seems a *little* bit jankier
> when I double tap than when I am just scrolling with my thumb, but I can't
> really figure out why. 

Yeah, it's a bit janky, not sure why. Chris, this patch scrolls the page programmatically and feels a lot less smooth than thumb scrolling. Any ideas why?

Comment 42

[Wesley Johnston (:wesj)]

This isn't using async pan zoom. You can probably just use ZoomHelper.zoomToRect() with some appropriate values to do the whole animation for you:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/ZoomHelper.js#87

Comment 43

[Wesley Johnston (:wesj)]

I haven't looked at this patch, but I was actually thinking the other day it would be nice if we scrolled a page length for you on normal web pages as well (assuming you didn't tap on something clickable). I wonder if that's worth trying?

Comment 44

[Ian Barlow (:ibarlow)]

(In reply to Wesley Johnston (:wesj) from comment #43)
> I haven't looked at this patch, but I was actually thinking the other day it
> would be nice if we scrolled a page length for you on normal web pages as
> well (assuming you didn't tap on something clickable). I wonder if that's
> worth trying?

We would want to take a look at the interaction -- we already use double tap for zooming normal pages. At one point we also discussed other options though, like tilting your device, or using the volume controls for page up/down

Comment 45

[Chris Lord [:cwiiis]]

(In reply to Wesley Johnston (:wesj) from comment #42)
> This isn't using async pan zoom. You can probably just use
> ZoomHelper.zoomToRect() with some appropriate values to do the whole
> animation for you:
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> ZoomHelper.js#87

This. You may also be interested in bug 1010538, which would help you for this.

Comment 46

[Awad Mackie]

Attachment: double_tap_forward.patch

Thanks! I've swapped over to using zoomToRect. 

Bug 1010538 for CSS-OM View and the DOM methods bug, https://bugzilla.mozilla.org/show_bug.cgi?id=1022818 could be useful but they seem to still be in development.

Build available here: https://dl.dropboxusercontent.com/u/8037977/fennec-33.0a1.en-US.android-arm.apk

Comment 47

[Awad Mackie]

Attachment: double_tap_forward.patch

Forgot the toolbar toggle on scroll up, new patch.

Build: https://dl.dropboxusercontent.com/u/8037977/fennec-33.0a1.en-US.android-arm.apk

Comment 48

[Ian Barlow (:ibarlow)]

(In reply to Awad Mackie from comment #47)
> Created attachment 8443907 [details] [diff] [review]
> double_tap_forward.patch
> 
> Forgot the toolbar toggle on scroll up, new patch.
> 
> Build:
> https://dl.dropboxusercontent.com/u/8037977/fennec-33.0a1.en-US.android-arm.
> apk

Nice, the scrolling feels smoother here than it did before. 

I did notice one other thing as I was using this build though. If I load an article in Reader Mode and then double tap to advance the text, the title bar doesn't hide when the page scrolls. I can hide it when I scroll manually, and then double tapping to read leaves the title bar hidden which is great. But it's just that initial double-tap-to-scroll *and* hide the title bar that seems to be missing.

Comment 49

[Awad Mackie]

Attachment: double_tap_forward.patch

> I did notice one other thing as I was using this build though. If I load an
> article in Reader Mode and then double tap to advance the text, the title
> bar doesn't hide when the page scrolls. I can hide it when I scroll
> manually, and then double tapping to read leaves the title bar hidden which
> is great. But it's just that initial double-tap-to-scroll *and* hide the
> title bar that seems to be missing.

Yep, makes sense. Patch and build: https://dl.dropboxusercontent.com/u/8037977/fennec-33.0a1.en-US.android-arm.apk

Comment 50

[Ian Barlow (:ibarlow)]

Looks great, thanks Awad!

Comment 51

[Lucas Rocha (:lucasr)]

Comment on attachment 8444822 [details] [diff] [review]
double_tap_forward.patch

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

Looking pretty good. I'd like to discuss the toolbar visibility changes in a separate bug if possible (so that we can land the double-tap feature sooner). Can file a bug like 'Hide toolbar when double-tapping to scroll in reader mode'.

Chris, do the changes to viewport code in browser.js look good?

::: mobile/android/base/GeckoApp.java
@@ +656,5 @@
>              startService(new Intent(
>                      UpdateServiceHelper.ACTION_APPLY_UPDATE, null, this, UpdateService.class));
> +
> +        } else if ("BrowserToolbar:Visibility".equals(event)) {
> +            setBrowserToolbarVisible(message.getBoolean("visible"));

This should only exist in BrowserApp.

::: mobile/android/chrome/content/aboutReader.js
@@ +226,5 @@
>        }
> +
> +      case "Gesture:DoubleTap": {
> +        let args = JSON.parse(aData);
> +        let scrollBy = null;

nit: no need to init to null

@@ +281,5 @@
>    },
>  
> +  _scrollPage: function Reader_scrollPage(scrollByPixels) {
> +    let viewport = BrowserApp.selectedTab.getViewport();
> +    let newY     = Math.min(Math.max(viewport.cssY + scrollByPixels, viewport.cssPageTop), viewport.cssPageBottom);

nit: remove extra spaces, we don't usually align multiple assignment like this anyway.

@@ +282,5 @@
>  
> +  _scrollPage: function Reader_scrollPage(scrollByPixels) {
> +    let viewport = BrowserApp.selectedTab.getViewport();
> +    let newY     = Math.min(Math.max(viewport.cssY + scrollByPixels, viewport.cssPageTop), viewport.cssPageBottom);
> +    let newRect  = new Rect(viewport.cssX, newY, viewport.cssWidth, viewport.cssHeight);

nit: remove extra space before '='

::: mobile/android/chrome/content/browser.js
@@ +4824,4 @@
>      let data = JSON.parse(aData);
>      let element = ElementTouchHelper.anyElementFromPoint(data.x, data.y);
>  
> +    if (BrowserApp.selectedBrowser.contentWindow.location.href.startsWith("about:reader")) {

I'd like Chris to have a look at this change.

Comment 52

[Chris Lord [:cwiiis]]

Comment on attachment 8444822 [details] [diff] [review]
double_tap_forward.patch

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

These changes are confusing to me, and they definitely shouldn't be introduced with no documentation as to what they're doing and why.

::: mobile/android/chrome/content/browser.js
@@ +4824,4 @@
>      let data = JSON.parse(aData);
>      let element = ElementTouchHelper.anyElementFromPoint(data.x, data.y);
>  
> +    if (BrowserApp.selectedBrowser.contentWindow.location.href.startsWith("about:reader")) {

So you're force-enabling double-tap for about:ready, but then disabling it actually doing anything? I'm confused.

@@ +6158,5 @@
>      // disable it in updateViewportSize.
>      let allowDoubleTapZoom = allowZoom;
>  
> +    if (aWindow.location.href.startsWith("about:reader")) {
> +      allowDoubleTapZoom = true;

Why are you doing this instead of just changing the meta viewport tag in the content?

Comment 53

[Awad Mackie]

Attachment: double_tap_forward.patch

Fixed nits and removed toolbar visibility changes

Comment 54

[Awad Mackie]

(In reply to Chris Lord [:cwiiis] from comment #52)
> So you're force-enabling double-tap for about:ready, but then disabling it
> actually doing anything? I'm confused.

I'm jumping out of browser.js' DoubleTap handling because it interferes with my using zoomToRect to scroll the page, is there a better way to do this?

> Why are you doing this instead of just changing the meta viewport tag in the
> content?

updateViewportSize checks the dimensions and disables double tap if specified in the content, http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#4433 

Actually the reason for all this is because JavaPanZoomController doesn't send Gesture:DoubleTap unless allowDoubleTapZoom is set (http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/JavaPanZoomController.java#1376), does it make more sense to remove that constraint? Not sure if that'll break anything along the way.

Comment 55

[Chris Lord [:cwiiis]]

Apologies for my initial curtness, I was short on time and didn't realise you were a new contributor. Thanks for your work, and it's a good start - Here's hoping for much more to come :)

(In reply to Awad Mackie from comment #54)
> (In reply to Chris Lord [:cwiiis] from comment #52)
> > So you're force-enabling double-tap for about:ready, but then disabling it
> > actually doing anything? I'm confused.
> 
> I'm jumping out of browser.js' DoubleTap handling because it interferes with
> my using zoomToRect to scroll the page, is there a better way to do this?

Yes, we only enable double-tap to zoom if the page is formatted in a way that it may need it. To disable it, you can specify in the page's meta viewport that zooming should not be allowed:
https://developer.mozilla.org/en/docs/Mozilla/Mobile/Viewport_meta_tag
In short, a tag in the head like this will do the same in a neater way:
<meta name="viewport" content="user-scalable=no" />

> updateViewportSize checks the dimensions and disables double tap if
> specified in the content,
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js#4433 
> 
> Actually the reason for all this is because JavaPanZoomController doesn't
> send Gesture:DoubleTap unless allowDoubleTapZoom is set
> (http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/
> JavaPanZoomController.java#1376), does it make more sense to remove that
> constraint? Not sure if that'll break anything along the way.

I'm uncertain as well, but perhaps it's worth trying?

Rather than using this gesture though, could you not listen to the 'click' event on the page and look at the detail for the click count?
https://developer.mozilla.org/en-US/docs/Web/Events/click

There is actually a 'dblclick' event too, but I have a feeling this isn't synthesised when using touch events. Might be worth a try though.

Comment 56

[Awad Mackie]

Attachment: double_tap_forward.patch

(In reply to Chris Lord [:cwiiis] from comment #55)
> Apologies for my initial curtness, I was short on time and didn't realise
> you were a new contributor. Thanks for your work, and it's a good start -
> Here's hoping for much more to come :)

Don't worry about it!

> In short, a tag in the head like this will do the same in a neater way:
> <meta name="viewport" content="user-scalable=no" />

> I'm uncertain as well, but perhaps it's worth trying?
> 
> Rather than using this gesture though, could you not listen to the 'click'
> event on the page and look at the detail for the click count?
> https://developer.mozilla.org/en-US/docs/Web/Events/click
> 
> There is actually a 'dblclick' event too, but I have a feeling this isn't
> synthesised when using touch events. Might be worth a try though.

Turns out I get neither event for a double tap, I've removed the restriction in JavaPanZoomController and it doesn't seem to be breaking anything. I'm now checking in browser.js' DoubleTap handler if double tap zoom is allowed. aboutReader's html already has user-scalable turned off so the new handler should be the only thing that does anything.

Does that make more sense?

Comment 57

[Chris Lord [:cwiiis]]

Comment on attachment 8449828 [details] [diff] [review]
double_tap_forward.patch

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

While I'm not totally convinced this can't be done without this native event, I think putting the check further down the chain makes sense and there's no reason for built-in features not to use stuff like this if it's more convenient. Looks good to me :)

Comment 58

[Awad Mackie]

Comment on attachment 8449828 [details] [diff] [review]
double_tap_forward.patch

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

Thanks!

Comment 59

[Lucas Rocha (:lucasr)]

Comment on attachment 8449828 [details] [diff] [review]
double_tap_forward.patch

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

Awesome. I assume Cwiiis is ok with the change in JavaPanZoomController.

Comment 60

[Chris Lord [:cwiiis]]

Comment on attachment 8449828 [details] [diff] [review]
double_tap_forward.patch

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

Adding an r+ to be explicit that I think the change to JavaPanZoomController is fine. This doesn't count as review for the other parts, but you have Lucas for that :)

Comment 61

[Carsten Book [:Tomcat]]

Hi,
could you provide a Try link. Suggestions for what to run if you haven't
yet can be found here:
https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices

Comment 62

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8449828 [details] [diff] [review]
double_tap_forward.patch

Also, please use the checkin-needed bug keyword rather than setting checkin? on the patch. It plays more nicely with our bug marking tools. Thanks :)

Comment 63

[Lucas Rocha (:lucasr)]

https://tbpl.mozilla.org/?tree=Try&rev=9238b9aa34dc

Comment 64

[travneff]

Just realized that this feature is related to reader mode.
Will it be available for usual browsing as well? It's very valuable there, as for me.

It might be a question what to do with zoom feature for this case. It could be resolved with volume keys used as discussed above, I think.

Comment 65

[Lucas Rocha (:lucasr)]

Try run looks good to me. Adding checkin-needed.

Comment 66

[Lucas Rocha (:lucasr)]

(In reply to travneff from comment #64)
> Just realized that this feature is related to reader mode.
> Will it be available for usual browsing as well? It's very valuable there,
> as for me.
> 
> It might be a question what to do with zoom feature for this case. It could
> be resolved with volume keys used as discussed above, I think.

Double-tap to scroll might conflict with what web devs do in their pages. You could probably write an add-on that does that though :-)

Comment 67

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/193712d05469

Comment 68

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/193712d05469

Comment 69

[Awad Mackie]

Thanks all, I've created a new bug for the browser toolbar part, https://bugzilla.mozilla.org/show_bug.cgi?id=1044665

Comment 70

[Teodora Vermesan (:TeoVermesan)]

double-tap anywhere to scroll down to next page. 

Device: Samsung Galaxy Nexus 
OS: Android 4.2.1

When performing double tap at the top of the page, the page is scrolled up and double tap at the bottom of the page will scroll the page down, so verified fixed on:
Build: Firefox for Android 34.0a1 (2014-08-07)