956946 - Gap below URL bar and menu

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Brian Nicholson (:bnicholson)]

Attachment: menu gap screenshot

There's a small gap between the action bar and the menu itself. All other Android apps align the menu directly under the action bar, which looks cleaner, and we should do the same to follow convention.

Though we've always had a gap here, we used to have an arrow between the menu button and the menu, so it made more sense before.

Screenshot attached.

Comment 1

[Brian Nicholson (:bnicholson)]

The menu margin/shadow is set by menu_panel_bg.9.png: http://mxr.mozilla.org/mozilla-central/find?text=&string=menu_panel_bg.9.png

Ian, I assume we want to flatten the menu out like the rest of our UI? If so, can you provide updated assets?

Comment 2

[Brian Nicholson (:bnicholson)]

Or maybe it's menu_popup_bg.9.png: http://mxr.mozilla.org/mozilla-central/find?text=&string=menu_popup_bg.9.png

I'm not sure what the difference is -- maybe we need to change both.

Comment 3

[Wesley Johnston (:wesj)]

It looks like the Android resources have this padding: http://androidxref.com/4.4.2_r1/xref/frameworks/base/core/res/res/drawable-xhdpi/menu_dropdown_panel_holo_light.9.png

Maybe we just need to offset this popup by the top padding?

Comment 4

[Brian Nicholson (:bnicholson)]

Hmm...I couldn't find any style where we set padding or margin, so I thought the spacing in the 9 png was creating the gap. That's weird that Android's 9 png has visible top padding/shadow, yet the menu is flush with the action bar...

Comment 5

[Brian Nicholson (:bnicholson)]

Eugen is taking a look at this, so assigning to him.

Comment 6

[Eugen Sawin [:esawin]]

Attachment: URL_bar_gap.patch

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8360782 [details] [diff] [review]
URL_bar_gap.patch

Passing to Brian for a better review.

I worry that menu_popup_offset is used for MenuPopup and ArrowPopup
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/menu/MenuPopup.java#33
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/ArrowPopup.java#49

I don't know that we want to change the ArrowPopup, since it still has the little arrow pointing up. MenuPopup used to have the little arrow too, so the same offset worked for both.

Maybe we should change menu_popup_offset, but add menu_popup_arrow_offset which has the old value and would be used in ArrowPopup?

Comment 8

[Brian Nicholson (:bnicholson)]

Comment on attachment 8360782 [details] [diff] [review]
URL_bar_gap.patch

I agree with Mark; menu_popup_offset is used by both MenuPopup and ArrowPopup, but we want to change only MenuPopup. Please create another dimens value as Mark said to keep these separate.

Comment 9

[Eugen Sawin [:esawin]]

Attachment: URL_bar_gap.patch

I have added menu_popup_arrow_offset as a separate dimens value for the ArrowPopup with the old value and updated the menu_popup_offset for MenuPopup to the new value to keep them separated.

Comment 10

[Brian Nicholson (:bnicholson)]

Comment on attachment 8361339 [details] [diff] [review]
URL_bar_gap.patch

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

Nice, this looks good.

Comment 11

[Eugen Sawin [:esawin]]

Attachment: URL_bar_gap.patch

Patch for check-in.

Comment 12

[Matthew N. [:MattN]]

Congratulations on getting your first patch landed!

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b290c1d5630f