as per bug 1058909 comment 63.

This is on a 60dp toolbar height (ignore that for now, just using this to show the options for Forward button size)

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.

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.

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).

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?

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.

(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.