Closed Bug 956946 Opened 6 years ago Closed 6 years ago

Gap below URL bar and menu

Categories

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

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 29
Tracking Status
fennec 29+ ---

People

(Reporter: bnicholson, Assigned: esawin)

Details

(Whiteboard: [mentor=bnicholson][lang=java])

Attachments

(2 files, 2 obsolete files)

Attached image 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.
tracking-fennec: ? → 29+
Whiteboard: [mentor=bnicholson][lang=java]
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?
Flags: needinfo?(ibarlow)
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.
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?
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...
Flags: needinfo?(ibarlow)
Eugen is taking a look at this, so assigning to him.
Assignee: nobody → esawin
Attached patch URL_bar_gap.patch (obsolete) — Splinter Review
Attachment #8360782 - Flags: review?(mark.finkle)
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?
Attachment #8360782 - Flags: review?(mark.finkle) → review?(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.
Attachment #8360782 - Flags: review?(bnicholson) → review-
Attached patch URL_bar_gap.patch (obsolete) — Splinter Review
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.
Attachment #8360782 - Attachment is obsolete: true
Attachment #8361339 - Flags: review?(bnicholson)
Comment on attachment 8361339 [details] [diff] [review]
URL_bar_gap.patch

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

Nice, this looks good.
Attachment #8361339 - Flags: review?(bnicholson) → review+
Patch for check-in.
Attachment #8361339 - Attachment is obsolete: true
Attachment #8361377 - Flags: review+
Congratulations on getting your first patch landed!

https://hg.mozilla.org/integration/fx-team/rev/b290c1d5630f
Status: NEW → ASSIGNED
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b290c1d5630f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
You need to log in before you can comment on or make changes to this bug.