"Edit bookmark" dialog needs some padding clean up

RESOLVED WONTFIX

Status

()

Firefox for Android
General
RESOLVED WONTFIX
3 years ago
2 years ago

People

(Reporter: antlam, Assigned: ahunt, Mentored)

Tracking

(Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=java])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Created attachment 8591105 [details]
Screenshot

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
Attachment #8591105 - Attachment description: RN4id2v6ED3ho2VJ3D9uuGZrMpXdMUKkDtOzRP5WbFc11NLU8VmZJcZjmmCiBBmQBaxVD13HpRi4aAvvQ27qsG555M6U_IJjwsbn63MWW4Pe5xp7TvrjPLD4dpZ454Xie31Hg6QUWCjpBtcRWG7Ly-wJJB5XHEs0JQ6MRHxEojKWpZ9VXoUo9S46yJE7wkUrp3O_84_dfSLM9LLpvM1VAjs_Oryk0cD1D_.png → Screenshot
(Reporter)

Updated

2 years ago
Blocks: 1157964

Updated

2 years ago
Mentor: michael.l.comella@gmail.com, margaret.leibovic@gmail.com
Whiteboard: [lang=java]
(Assignee)

Comment 1

2 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
(Assignee)

Comment 2

2 years ago
Created attachment 8696066 [details]
MozReview Request: Bug 1153441 - Improve padding for "Edit Bookmark" dialog r=mcomella

Bug 1153441 - Improve padding for "Edit Bookmark" dialog r=mcomella
Attachment #8696066 - Flags: review?(michael.l.comella)

Updated

2 years ago
Assignee: nobody → ahunt
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.
Attachment #8696066 - Flags: review?(michael.l.comella)
(Assignee)

Comment 4

2 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.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
(Assignee)

Updated

2 years ago
Attachment #8696066 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.