Closed Bug 1209539 Opened 4 years ago Closed 4 years ago

Disable "Transfer" in import dialog if no items are checked

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 45
Tracking Status
firefox44 --- affected
firefox45 --- fixed

People

(Reporter: liuche, Assigned: liuche)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Currently we allow users to "Transfer" even if they don't have anything checked (bookmarks, history). We should disable the Transfer button in that case and only allow them to Cancel (and also not auto-advance if they hit Cancel).
Bug 1209539 - Disable "Transfer" in import dialog if no items are checked. r=sebastian
Attachment #8669979 - Flags: review?(s.kaspari)
Attached image Screenshot: Incorrect button styling (obsolete) —
Argh, these buttons are not styled correctly!

I'm using the container as the view root when inflating the view, but for some reason it's not inheriting the styles. I also tried setting a style for the buttons in xml, but there aren't isn't anything like a Theme.AlertDialog.Button style.

I'll look more tomorrow.
QA Contact: liuche
Assignee: nobody → liuche
QA Contact: liuche
Attachment #8669979 - Flags: review?(s.kaspari)
Comment on attachment 8669979 [details]
MozReview Request: Bug 1209539 - Disable "Transfer" in import dialog if no items are checked. r=sebastian

https://reviewboard.mozilla.org/r/21309/#review19227

It looks like the layout file is missing from the patch.
Comment on attachment 8669979 [details]
MozReview Request: Bug 1209539 - Disable "Transfer" in import dialog if no items are checked. r=sebastian

Bug 1209539 - Disable "Transfer" in import dialog if no items are checked. r=sebastian
Attachment #8669979 - Flags: review?(s.kaspari)
Comment on attachment 8669979 [details]
MozReview Request: Bug 1209539 - Disable "Transfer" in import dialog if no items are checked. r=sebastian

https://reviewboard.mozilla.org/r/21309/#review19319
Attachment #8669979 - Flags: review?(s.kaspari) → review+
Sebastian, I've been at a work week this week, so I haven't been able to take a look at this - but off the top of your head, can you think of a reason why this button styling isn't correct? From comment #2.
Flags: needinfo?(s.kaspari)
(In reply to Chenxia Liu [:liuche] from comment #6)
> Sebastian, I've been at a work week this week, so I haven't been able to
> take a look at this - but off the top of your head, can you think of a
> reason why this button styling isn't correct? From comment #2.

It looks like adding style="?android:attr/buttonBarButtonStyle" to the buttons does the trick.

For reference here is the default alert layout XML of Android:
https://github.com/android/platform_frameworks_base/blob/master/core/res/res/layout/alert_dialog.xml
Flags: needinfo?(s.kaspari)
Sweet, that's a good find. It looks like I need to do a little bit more work beyond this, because we need that extra divider and there's some extra padding under the button group.
Comment on attachment 8669979 [details]
MozReview Request: Bug 1209539 - Disable "Transfer" in import dialog if no items are checked. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21309/diff/2-3/
Comment on attachment 8669979 [details]
MozReview Request: Bug 1209539 - Disable "Transfer" in import dialog if no items are checked. r=sebastian

New version without custom layout nonsense. Pretty much the same as what I discovered in bug 1044401, with very similar code.
Attachment #8669979 - Flags: review+ → review?(s.kaspari)
Attachment #8669986 - Attachment is obsolete: true
Comment on attachment 8669979 [details]
MozReview Request: Bug 1209539 - Disable "Transfer" in import dialog if no items are checked. r=sebastian

https://reviewboard.mozilla.org/r/21309/#review22963

::: mobile/android/base/firstrun/ImportPanel.java:92
(Diff revision 3)
> +                dialog.show();

NIT/Question: We are creating the dialog here and then modifiying it. Do we need to do that because otherwise the views are not created yet? Just curious because the order of statements looks strange. :)

::: mobile/android/base/firstrun/ImportPanel.java:115
(Diff revision 3)
> +

NIT: Empty line
Attachment #8669979 - Flags: review?(s.kaspari) → review+
https://reviewboard.mozilla.org/r/21309/#review22963

> NIT/Question: We are creating the dialog here and then modifiying it. Do we need to do that because otherwise the views are not created yet? Just curious because the order of statements looks strange. :)

Yeah, the documentation [1] seems to imply the dialog will be created on an onCreate() call, but in practice, the views are null until show() is called [2]. I assume that calling show() the first time actually inflates the Views, and before that they don't exist.

[1] http://developer.android.com/reference/android/app/AlertDialog.html#getButton%28int%29
[2] http://stackoverflow.com/a/24872351
https://hg.mozilla.org/mozilla-central/rev/3423dce2511e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Depends on: 1232968
You need to log in before you can comment on or make changes to this bug.