880855 - Defect - Back and Tab overlay buttons should be repositionable

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: Browser, defect, P2)

Description

[Kamil Jozwiak [:kjozwiak]]

Just wanted to mention this as a possible discussion regarding the overlay buttons that are located on both sides of the screen ("Back" & "New Tab" buttons). Currently, there's no way of retracting the buttons making it almost impossible to select something if one of the buttons is covering something of interest.

A perfect example is using Gmail. The "Back" overlay button will block the labels making it almost impossible to select the labels or move emails from the inbox to certain labels. I am sure there are other websites out there that the overlay buttons will be covering something of interest.

I think it would be a good idea to somehow make the overlays retractable. 

Yuan, just wanted to know your thoughts on this.

Comment 1

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

Great observation, Kamil.

We thought about this issue. And our current solution is to make the position of overlay buttons adjustable. The users should be able to move two buttons up or down along the edge by dragging either one.

Also, when there is full-screen media playing on a page, the overlay buttons should disappear.

CC'ing Frank, so we can get a Change bug into the Iteration as soon as we can.

Comment 2

[Jim Mathies [:jimm]]

I guess that doesn't fit, since it's epic. Somewhere we should have a story about the nav overly buttons that hopefully isn't closed yet.

Comment 3

[Jim Mathies [:jimm]]

Attachment: touch support v.1

This is the touch component which I'd like to land first. It looks really nice.

For mouse, I experimented with mouse tracking but found the motion to be janky. Our touch drag code in input.js does a really nice job of smoothing things out, and touch events in general are more fluid. I think if we want to do mouse drags we'll have to get the input.js drag code hooked back up to mouse movement.

I wonder though if we even want these buttons visible when using the mouse? I know we have nice transitions for hover setup up so I think the original intent was keep them visible when the mouse is active. If we want to try and get that working I'll see what I can do once I finish up some other work.

Comment 4

[Jim Mathies [:jimm]]

mbrubeck suggested we do a better job of hiding these when the mouse is active. That partially solves the problem, although I think users will still expect that they will be able to slide them around.

Comment 5

[Frank Yan (:fryn)]

Comment on attachment 762272 [details] [diff] [review]
touch support v.1

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

(In reply to Jim Mathies [:jimm] from comment #3)
> Created attachment 762272 [details] [diff] [review]
> touch support v.1

This is fantastic! :D

> For mouse, I experimented with mouse tracking but found the motion to be
> janky. Our touch drag code in input.js does a really nice job of smoothing
> things out, and touch events in general are more fluid. I think if we want
> to do mouse drags we'll have to get the input.js drag code hooked back up to
> mouse movement.

I see. If it possible to hook it up to mouse movement only for the overlay buttons, or perhaps directly trigger code in input.js inside mouse event listeners on the overlay buttons?


(In reply to Jim Mathies [:jimm] from comment #3)
> I wonder though if we even want these buttons visible when using the mouse?

I think these buttons are actually more useful when using a mouse/trackpad than when using a trackpad. If we improved our back/forward swiping to be more fluid and intuitive, the overlay buttons wouldn't be as useful for touch, but with a mouse, you can't swipe.

::: browser/metro/base/content/NavButtonSlider.js
@@ +86,5 @@
> +  _update: function (aClientY) {
> +    if (this._topStop > aClientY || this._bottomStop < aClientY)
> +      return;
> +    this._yPos = aClientY;
> +    this._setPosition();

A future improvement for this is to have the buttons snap smoothly to their original position (50%), so it's easy to reset.

Comment 6

[Jim Mathies [:jimm]]

> > For mouse, I experimented with mouse tracking but found the motion to be
> > janky. Our touch drag code in input.js does a really nice job of smoothing
> > things out, and touch events in general are more fluid. I think if we want
> > to do mouse drags we'll have to get the input.js drag code hooked back up to
> > mouse movement.
> 
> I see. If it possible to hook it up to mouse movement only for the overlay
> buttons, or perhaps directly trigger code in input.js inside mouse event
> listeners on the overlay buttons?

I'll take a shot at this. I'd rather not mess with input.js.

Comment 7

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #6)
> > > For mouse, I experimented with mouse tracking but found the motion to be
> > > janky. Our touch drag code in input.js does a really nice job of smoothing
> > > things out, and touch events in general are more fluid. I think if we want
> > > to do mouse drags we'll have to get the input.js drag code hooked back up to
> > > mouse movement.
> > 
> > I see. If it possible to hook it up to mouse movement only for the overlay
> > buttons, or perhaps directly trigger code in input.js inside mouse event
> > listeners on the overlay buttons?
> 
> I'll take a shot at this. I'd rather not mess with input.js.

What I might be able to do is keep this contained in in NavButtonSlider with mouse input and borrow a copy of KineticController to smooth things out.

Comment 8

[Jim Mathies [:jimm]]

Attachment: mouse support v.1

This is built on top of 'touch support v.1'.

Comment 9

[Frank Yan (:fryn)]

Comment on attachment 764333 [details] [diff] [review]
mouse support v.1

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

Most of it looks good. Just some comments to address below.
I'll test out the patch locally in a bit.

::: browser/metro/base/content/NavButtonSlider.js
@@ +6,5 @@
>   * Handles nav overlay button positioning.
>   */
>  
> +// minimum amount of movement using the mouse after which we cancel the button click handlers
> +const kOnClickMargin = 20;

I wonder if there is a canonical algorithm for determining this sort of thing. mbrubeck might know. In the meantime, I don't have significantly better ideas off the top of my head. The movement threshold that our platform uses before triggering a dragstart listener is 3px, if I recall correctly, but that's definitely too small for low-precision dragging with a finger.

@@ +89,5 @@
>  
> +  _getPosition: function _getPosition() {
> +    let strTop = window.getComputedStyle(this._back, null).top;
> +    strTop.replace("px", "");
> +    this._yPos = parseInt(strTop, 10);

* parseInt('8px') returns 8.
* parseInt('013') now returns 13, not 11.
* the second argument of getComputedStyle is optional
So you can just do the following:
this._yPos = parseInt(getComputedStyle(this._back).top);

@@ +132,5 @@
> +        this._mouseY = aEvent.clientY;
> +        break;
> +      case "mouseup":
> +        this._capture = false;
> +        aEvent.originalTarget.setCapture(false);

It's cool that you're using setCapture :D, but that's not how the setCapture API works:
https://developer.mozilla.org/en-US/docs/Web/API/element.setCapture
The argument for setCapture actually specifies retargeting.
Please check if you actually want retargeting (in mousedown).
You don't need to call releaseCapture. It will happen automatically on mouseup.

Comment 10

[Jim Mathies [:jimm]]

(In reply to Frank Yan (:fryn) from comment #9)
> I wonder if there is a canonical algorithm for determining this sort of
> thing. mbrubeck might know. In the meantime, I don't have significantly
> better ideas off the top of my head. The movement threshold that our
> platform uses before triggering a dragstart listener is 3px, if I recall
> correctly, but that's definitely too small for low-precision dragging with a
> finger.

This only applies to mouse input, so I'll use 3px.

> So you can just do the following:
> this._yPos = parseInt(getComputedStyle(this._back).top);

Thanks, much cleaner.

> 
> @@ +132,5 @@
> > +        this._mouseY = aEvent.clientY;
> > +        break;
> > +      case "mouseup":
> > +        this._capture = false;
> > +        aEvent.originalTarget.setCapture(false);
> 
> It's cool that you're using setCapture :D, but that's not how the setCapture
> API works:
> https://developer.mozilla.org/en-US/docs/Web/API/element.setCapture
> The argument for setCapture actually specifies retargeting.
> Please check if you actually want retargeting (in mousedown).
> You don't need to call releaseCapture. It will happen automatically on
> mouseup.

Ok, I see. I'm guessing either will work (no descendents?), hence maybe false is fine. I'll test a bit.

Comment 11

[Jim Mathies [:jimm]]

Attachment: mouse support v.2

Comment 12

[Frank Yan (:fryn)]

Comment on attachment 764360 [details] [diff] [review]
mouse support v.2

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

Almost there! :)

::: browser/metro/base/content/NavButtonSlider.js
@@ +138,5 @@
> +        if (!this._capture) {
> +          return;
> +        }
> +        let dy = aEvent.clientY - this._mouseY;
> +        this._mouseMoveAmount += dy;

I think this line should be replaced with:
if (aEvent.mozInputSource != Ci.nsIDOMMouseEvent.MOZ_SOURCE_TOUCH &&
    !this._moveStarted &&
    Math.abs(dy) < kOnClickMargin)
  return;
this._mouseMoveStarted = true;

and change the check in `case "click"` to:
if (this._mouseMoveStarted)
  return;

and insert the following in `case "mousedown"`:
this._mouseMoveStarted = false;

No more need for `_mouseMoveAmount`.

That's because the behavior we want is:
For mouse events, require displacement of kOnClickMargin before moving the button. If the button is moved, don't invoke its action.

Comment 13

[Frank Yan (:fryn)]

I suppose the check in `case "click"` will need to retain the mozInputSource condition.

Comment 14

[Frank Yan (:fryn)]

Or you modify the condition in `mousemove` to:
if (aEvent.mozInputSource != Ci.nsIDOMMouseEvent.MOZ_SOURCE_TOUCH) {
  if (!this._mouseMoveStarted && Math.abs(dy) < kOnClickMargin)
    return;
  this._mouseMoveStarted = true;
}
and not need the input source check in `click`.

Comment 15

[Jim Mathies [:jimm]]

(In reply to Frank Yan (:fryn) from comment #14)
> Or you modify the condition in `mousemove` to:
> if (aEvent.mozInputSource != Ci.nsIDOMMouseEvent.MOZ_SOURCE_TOUCH) {
>   if (!this._mouseMoveStarted && Math.abs(dy) < kOnClickMargin)
>     return;
>   this._mouseMoveStarted = true;
> }
> and not need the input source check in `click`.

We don't get mousemoves from touch so I had to keep this check in the click handler. Other than this though I think I've implemented what you wanted and everything seems to be working.

Comment 16

[Jim Mathies [:jimm]]

Attachment: mouse support v.3

Comment 17

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #15)
> (In reply to Frank Yan (:fryn) from comment #14)
> > Or you modify the condition in `mousemove` to:
> > if (aEvent.mozInputSource != Ci.nsIDOMMouseEvent.MOZ_SOURCE_TOUCH) {
> >   if (!this._mouseMoveStarted && Math.abs(dy) < kOnClickMargin)
> >     return;
> >   this._mouseMoveStarted = true;
> > }
> > and not need the input source check in `click`.
> 
> We don't get mousemoves from touch so I had to keep this check in the click
> handler. Other than this though I think I've implemented what you wanted and
> everything seems to be working.

Actually, I may not need this check at all, need to test without it.

Comment 18

[Jim Mathies [:jimm]]

Attachment: mouse support v.3

Since _mouseMoveStarted only gets set when we get mousemoves, and we never get those for touch, we can skip that test completely.

Comment 19

[Frank Yan (:fryn)]

Comment on attachment 764807 [details] [diff] [review]
mouse support v.3

Thanks! :)

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1cfbe0f0c74d
https://hg.mozilla.org/mozilla-central/rev/870691ed6060

Comment 21

[Mihaela Velimiroviciu (:mihaelav)]

Build identifier: Mozilla/5.0 (Windows NT 6.2; rv:25.0) Gecko/20130625 Firefox/25.0

Verified (on desktop) that the position of the back and new tab overlay buttons can be adjusted: buttons can be moved up and down. Also, buttons disappear when a video plays in full screen mode.

This still needs verification for mobile devices.

Comment 22

[Manuela Muntean [Away]]

Build identifier: Mozilla/5.0 (Windows NT 6.2; rv:25.0) Gecko/20130716 Firefox/25.0

On desktop, verified that the back and new tab overlay buttons can be moved up and down. Also, buttons disappear when a video (HTML 5) plays in full screen mode.

This still needs verification for mobile devices.

Comment 23

[Eddi]

I hope that even though this has been resolved, that I can still add input.  I didn't know that the buttons were adjustable and I have a feeling that many users may not figure this out either.  I only realized when I went to submit a bug/suggestion but found this thread.

Would it be possible to move or somehow 'nudge' the new tab and back button when the mouse hovers over it slightly so that the user can either click on it or grab what's underneath this.  In my head it seems like makes sense, but the idea may need someone else's thoughts to be make it awesome.

Comment 24

[Samvedana (:Samvedana)]

User Agent: Mozilla/5.0 (Windows NT 6.2; Win64; x64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130813030205
Built from http://hg.mozilla.org/mozilla-central/rev/c146d402a55f

WFM
Tested on windows 8 using latest nightly. Back and Tab overlay buttons are repositionable.

Comment 25

[Samvedana (:Samvedana)]

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130816030205
Built from http://hg.mozilla.org/mozilla-central/rev/1ed5a88cd4d0

WFM
Tested on windows 8 using latest nightly for iteration-12.  Back and Tab overlay buttons are repositionable.