Anthony Lam (:antlam) Find me, @antlam on Slack! Reporter
	Description • 9 years ago

We inherit the right theme, but i imagine we hacked together some of the padding for pre-L and so we should clean that up accordingly (use L padding specs for dialogs)

Found here: http://www.google.com/design/spec/components/dialogs.html#dialogs-specs

	Andrzej Hunt :ahunt Assignee
	Comment 1 • 9 years ago

As far as I can tell we're actually inheriting the themes correctly (or will definitely be when [1] lands).

However the padding for items in an AlertDialog is hardcoded in Android's layout files [2] - for this specific dialog we insert a custom layout as the dialog content using AlertDialog.Builder.setView [3], which is inserted into customPanel in Android's alert_dialog.xml - customPanel has no left/right padding set, which is why our dialog content fills the full width.

nalexander suggested retrieving the correct padding dynamically, which means we shouldn't need to care if the paddings are changed in future (patch in progress) - this could be reused for any AlertDialog that contains custom content.

The layout currently used by AlertDialog.Builder doesn't even conform to Google's style guide: the total title padding is ~19dp, and the default TextView is offset another 3px from the title. I think it's more sensible to copy the TextView padding for consistency with most other dialogs on the system (they also exhibit this offset) - trying to copy the title padding would probably also be more brittle (the Title is inside a layout which uses a margin, whereas the TextView just uses two sets of padding).

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1220315
[2] https://github.com/android/platform_frameworks_base/blob/master/core/res/res/layout/alert_dialog.xml

	Andrzej Hunt :ahunt Assignee
	Comment 2 • 9 years ago

Bug 1153441 - Improve padding for "Edit Bookmark" dialog r=mcomella

	Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]
	Comment 3 • 9 years ago

Comment on attachment 8696066 [details]
MozReview Request: Bug 1153441 - Improve padding for "Edit Bookmark" dialog r=mcomella

https://reviewboard.mozilla.org/r/27273/#review24839

::: mobile/android/base/DialogUtils.java:6
(Diff revision 1)
> +package org.mozilla.gecko;

This should be in `org.mozilla.gecko.util`. Don't forget to update moz.build!

::: mobile/android/base/DialogUtils.java:16
(Diff revision 1)
> +    public static void alignCustomPanel(final AlertDialog dialog, final Context context) {

nit: add some javadoc to explain why we need this method.

::: mobile/android/base/DialogUtils.java:25
(Diff revision 1)
> +        final int scrollViewID = context.getResources().getIdentifier("scrollView", "id", "android");

This seems pretty fragile because it could change between android versions, manufacturer hardware, etc. – I'd rather set and hardcode our own custom values that mirror the Android standard.

What do you think?

Holding r+ due to the fragility issue.

	Andrzej Hunt :ahunt Assignee
	Comment 4 • 9 years ago

Bug 1232439 will involve significant changes to the entire dialog, and will also make this bug obsolete, so I think it's probably best to abandon this bug.