Closed Bug 1072464 Opened 5 years ago Closed 5 years ago

Discuss new tablet forward button size

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 36

People

(Reporter: mcomella, Assigned: mcomella)

References

Details

Attachments

(4 files, 2 obsolete files)

Assignee: nobody → michael.l.comella
Status: NEW → ASSIGNED
Attached image prev_tabletui_vert_mock8_forward.png (obsolete) —
This is on a 60dp toolbar height (ignore that for now, just using this to show the options for Forward button size)
Attached image Forward button small (obsolete) —
This is the other size, less tap-able, but also less "in the way"
A third go.

I originally sized and spaced it the same as the 3 buttons on the right hand side for tap-ability but I've now used the visual hit area of the back button to space the forward here.
Attachment #8494690 - Attachment description: prev_tabletui_vert_mock7_forward.png → Forward button small
Looking at the different options, I think the best option would be something between the 'small' option and the third one :-) In other words, a slightly smaller version of the third option. Thoughts?
I'd say either small, or slightly larger (but smaller than option three) as Lucas suggests in comment 4.
Played with this some more and I have a lot of concern about the forward button being way too frustrating to press if it's too small..

I think this is a good, balanced first go for V1, let's try this and we can iterate on it later. But, this way the visuals are still balanced and it's not too small still.
Attachment #8494688 - Attachment is obsolete: true
Attachment #8494690 - Attachment is obsolete: true
Flags: needinfo?(michael.l.comella)
This simplifies the animation code a bit by removing the whatever was going on
with the left margin of the forward button.

Additionally, this implementation is to antlam's spec, with the exception of
the forward button being centered in its space: paddingLeft on an ImageView
doesn't seem to work properly (at least if I calculated things correctly) so I
just eyeballed this value to center the arrow. I'm a little concerned about how
consistent it will be across devices (because I don't know why it was
not working properly in the first place).
Attachment #8505064 - Flags: review?(lucasr.at.mozilla)
Attached image Screenshot of patch
Attachment #8505065 - Flags: feedback?(alam)
Flags: needinfo?(michael.l.comella)
Note that there is inconsistent behavior when the back/forward buttons are tapped during animation, so I filed bug 1082863, though this issue can conceivably also be fixed by bug 960746.
(In reply to Michael Comella (:mcomella) from comment #9)
> Note that there is inconsistent behavior when the back/forward buttons are
> tapped during animation, so I filed bug 1082863, though this issue can
> conceivably also be fixed by bug 960746.

Is this a pre-existing issue or something new caused by these changes?
Attachment #8505065 - Flags: feedback?(alam) → feedback+
Comment on attachment 8505064 [details] [diff] [review]
Implement new tablet forward button dimensions and clean up animation code

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

Gosh, how I hate this forward button animation code :-/ I declare bankruptcy. We need to rewrite this at some point. IIUC, you're reversing the animation logic, right?

::: mobile/android/base/toolbar/BrowserToolbarNewTablet.java
@@ +30,5 @@
> +
> +        // The forward button is initially expanded (in the layout file)
> +        // so translate it for start of the expansion animation; future
> +        // iterations translate it to this position when hiding and will already be set up.
> +        ViewHelper.setTranslationX(forwardButton, -forwardButtonTranslationWidth);

Not sure I follow. AFAIK, the forward button is initially hidden, no?

This only works because API level >= 11 for tablets btw. In pre-Honeycomb releases, the touch events were only handled in the untransformed bounds.
Attachment #8505064 - Flags: review?(lucasr.at.mozilla) → review+
(In reply to Lucas Rocha (:lucasr) from comment #10)
> Is this a pre-existing issue or something new caused by these changes?

Looks like it is introduced by these changes - I think the removal of margin checking (and its `return`) is the issue.

(In reply to Lucas Rocha (:lucasr) from comment #11)
> Gosh, how I hate this forward button animation code :-/ I declare
> bankruptcy. We need to rewrite this at some point.

I thought to - but my approach ended up being fairly similar to the existing code and is what you see in these patches.

> IIUC, you're reversing the animation logic, right?

Not sure what you mean by reversing. Basically, the forward button is set up in its displayed state in XML and in the constructor we offset the translation. We translate it to its original position to show it again.

Because the text field size changes, we have to change the left margin on the display and edit layouts - so we do a combination of changing these margins to show/hide and translating as part of the animation. 

> ::: mobile/android/base/toolbar/BrowserToolbarNewTablet.java
> @@ +30,5 @@
> > +
> > +        // The forward button is initially expanded (in the layout file)
> > +        // so translate it for start of the expansion animation; future
> > +        // iterations translate it to this position when hiding and will already be set up.
> > +        ViewHelper.setTranslationX(forwardButton, -forwardButtonTranslationWidth);
> 
> Not sure I follow. AFAIK, the forward button is initially hidden, no?

alpha == 0, but it is located in its displayed position - we translate here to put it in its hidden position initially, so we are ready for the show animation state.
https://hg.mozilla.org/mozilla-central/rev/1c6a933fb75c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Duplicate of this bug: 1084313
You need to log in before you can comment on or make changes to this bug.