899002 - Use one button dialog for CE audio warning

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Settings, defect)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

+++ This bug was initially created as a clone of Bug #891687 +++

v1.2 scope, see the original bug and attachment 782457 [details].

The current dialog shows two buttons (cancel/continue) and is text only, mostly due to schedule pressure on 1.1.  For 1.2 the story is to implement according to the spec UX provided in bug 863267 (also attached).
To fulfill the requirement, we should at least move to a one button dialog.  The asset is not part of the CE requirement, but let's follow the UX design if possible.As a user, I would like to be warned if audio from my headphones are dangerously loud

Comment 1

[Greg Weng [:snowmantw][:gweng][:λ]]

With Gary's helps, I've found a possible way to solve this issue. And I'm used to record details in a individual document (personal notes), here it is:

https://docs.google.com/document/d/1jQ7RB74qP337utnN2zs_fR4EOEFFOXh5UkUUpWOJcrA/edit?usp=sharing

Comment 2

[Greg Weng [:snowmantw][:gweng][:λ]]

This bug is almost solved, but it still needs a icon described in the attachment 782457 [details], page 8.

Comment 3

[Greg Weng [:snowmantw][:gweng][:λ]]

This bug is almost solved, but it still needs an icon described in the attachment 782457 [details], page 8.

Comment 4

[Greg Weng [:snowmantw][:gweng][:λ]]

Attachment: Patch

This patch solved this issue by

1. Changing the original CustomDialog to ModalDialog, which has only one button.
2. Hacking the ModalDialog's appearance because the original one doesn't support icon.
3. Restore the hack after user close it.

Comment 5

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Greg Weng [:snowmantw] from comment #4)
> Created attachment 789464 [details]
> Patch
> 
> This patch solved this issue by
> 
> 1. Changing the original CustomDialog to ModalDialog, which has only one
> button.
> 2. Hacking the ModalDialog's appearance because the original one doesn't
> support icon.

It's really a bad idea to me. We shouldn't directly control a module from another one, especially touching the DOM element which isn't managed under sound manager.

For example, sound manager shows the current volume settings by adding/removing class to some div under sound overlay. It's bad if another module directly overwrites the class name.

This violates the direction we're moving toward(Module decoupling).

IMO the correct way is:
Design a new dialog class, for more more general purpose. (Include icon as config).

Currently Modal Dialog refactor is in the scope of new window management system, so I couldn't judge that now is a good timing to write the general dialog, but it'd be a good practice.

Or else, at least, you could modify custom dialog to accept icon parameter. But be careful because it's shared among apps.

> 3. Restore the hack after user close it.

Comment 6

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 789464 [details]
Patch

Don't want to give a r- to your first patch but only cancel the review. 
Please see my last comment. Come to me if you have problems.
Thanks.

Comment 7

[Greg Weng [:snowmantw][:gweng][:λ]]

I've update my patch as follow:

1. Allow title and message in CustomDialog displaying with optional icons and other possible options, while the function signature remains the same.

2. Modify code in sound manager to use the new CustomDialog.

Instead of changing the function's arguments list and cause possible bugs for its callees, I chose to not modify it and allow passing new arguments with different type to achieve this.

Comment 8

[(inactive after 6/18) Alive Kuo [:alive]]

^ Status update or review request?

Comment 9

[Greg Weng [:snowmantw][:gweng][:λ]]

(In reply to Alive Kuo [:alive] from comment #8)
> ^ Status update or review request?

Sorry. I just set up the review flag again.

Comment 10

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 789464 [details]
Patch

r=me if you change per github comments.
Let's feedback from l10n for string change and feedback from etinne because he invents the custom dialog.

Comment 11

[Greg Weng [:snowmantw][:gweng][:λ]]

Attachment: VolumeWarningDialog2.png

Comment 12

[Greg Weng [:snowmantw][:gweng][:λ]]

The last design I received shows the title is capitalized like "Volume Warning", but the title inside original draft and the "ceWarningtitle" entry in the locale file is "Volume warning". So I attached the picture I received and need info Neo.

Comment 13

[Etienne Segonzac (:etienne)]

Comment on attachment 789464 [details]
Patch

Pretty sure I didn't invent the custom dialog, but the code looks good!

Comment 14

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Etienne Segonzac (:etienne) from comment #13)
> Comment on attachment 789464 [details]
> Patch
> 
> Pretty sure I didn't invent the custom dialog, but the code looks good!

git log said so :/ anyway I think this patch is OK to go.

Comment 15

[Etienne Segonzac (:etienne)]

(In reply to Alive Kuo [:alive] from comment #14)
> (In reply to Etienne Segonzac (:etienne) from comment #13)
> > Pretty sure I didn't invent the custom dialog, but the code looks good!
> 
> git log said so :/ 

That's because I stole it ;)

Comment 16

[Axel Hecht [:Pike]]

I don't see string changes so far?

Comment 17

[Francesco Lodolo [:flod]]

The only thing I saw disappeared in the middle (capitalization change)

-ceWarningtitle=Volume warning
+ceWarningtitle=Volume Warning

Comment 18

[Casey Yee]

(In reply to Greg Weng [:snowmantw] from comment #3)
> This bug is almost solved, but it still needs an icon described in the
> attachment 782457 [details], page 8.

Flagging visual design for support

Comment 19

[Greg Weng [:snowmantw][:gweng][:λ]]

(In reply to Casey Yee [:cyee] from comment #18)
> (In reply to Greg Weng [:snowmantw] from comment #3)
> > This bug is almost solved, but it still needs an icon described in the
> > attachment 782457 [details], page 8.
> 
> Flagging visual design for support

It's a legacy ni comment. The icon problem had been solved and I forgot to reply that.
The last ni is for the capitalization issue.

Comment 20

[Greg Weng [:snowmantw][:gweng][:λ]]

Master:
https://github.com/mozilla-b2g/gaia/commit/f0bed3a7ed918c86bb0a49b53dba568d6fa80c87

Comment 21

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I don't think we need to land this to v1.1 or v1.1hd.

Patryk, could you clarify the capitalization change mentioned in comment 12?

Comment 22

[Patryk Adamczyk [:patryk] UX]

Peter, can you answer comment 21?

Comment 23

[Peter La]

Hi Tim/Patryk,

Please follow what was in the original UX spec.  So "Volume warning", not "Volume Warning".