Closed Bug 807418 Opened 12 years ago Closed 12 years ago

[Clock] No way to preview an alarm tone

Categories

(Firefox OS Graveyard :: Gaia::Clock, defect, P1)

Product:
Firefox OS Graveyard
Component:
Gaia::Clock
Platform:
All
Other
Type:
defect

Tracking

(blocking-basecamp:+)

Status:
VERIFIED FIXED
Milestone:
B2G C2 (20nov-10dec)
Project Flags:
blocking-basecamp +

People

(Reporter: padamczyk, Assigned: iliu)

References

Details

(Keywords: feature)

Attachments

(3 files, 1 obsolete file)

Screen I am referencing.
105.10 KB, image/png
Details
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/6572
355 bytes, text/html
alive
: review+
Details
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/6581
355 bytes, text/html
vingtetun
: review+
Details 
There should be a way to preview an alarm tone after the user has selected it. Perhaps all that is needed is a  (toggle like) button below the "snooze" label that says "preview" and plays the tone on loop. The button's label changes to "stop preview" and then when clicked the preview stops.
I totally agree with Patryk's suggestion.
User should want to choose and set their favorite ringtone of alarm with the feature.
Could we set blocking-basecamp to + ?
blocking-basecamp: --- → ?
Not blocker for shipping
blocking-basecamp: ? → -
This was discussed with product - is a P1 feature. If we ship alarms, users must be able to know what they sound like.
blocking-basecamp: - → +
Keywords: feature
Priority: -- → P1
Ian, I'll get you the proper wireframes for this feature next week sometime.
Assignee: iliu → padamczyk
(In reply to Patryk Adamczyk [:patryk] UX from comment #5)
> Ian, I'll get you the proper wireframes for this feature next week sometime.

That's really a risk at this point. We must finish new feature work like this.

What about just playing the sound when it's selected? That should be enough for this purpose, and requires no new UI.
Yes, that is fine, if it worked just like the ringtone selection. To reduce risk, visually its OKAY for v.1 if it looks inconsistent (no ui change). Just when you click on the alarm name in the choice dialog, it should preview the alarm.
Assignee: padamczyk → iliu
(In reply to Dietrich Ayala (:dietrich) from comment #6)
> (In reply to Patryk Adamczyk [:patryk] UX from comment #5)
> > Ian, I'll get you the proper wireframes for this feature next week sometime.
> 
> That's really a risk at this point. We must finish new feature work like
> this.
> 
> What about just playing the sound when it's selected? That should be enough
> for this purpose, and requires no new UI.

(A) For the above suggestion, it will need platform support to fire/receive the event.
Gaia can not receive the event when user are selecting on the value selector.
It will need some effort to implement the section in Gecko and Gaia. We should need to have discussion of supported event for <select> tag.

(B) If we don't want make the big change, I can just implement a customized value selector as system one. But it looks like a workaround.

(C) If we have another button to preview the alarm in edit-page, we don't need to have effort for (A), (B) now.
Milestoning for C2 (deadline of 12/10), as this meets the criteria of "remaining P1 bugs not already milestoned for C1".
Target Milestone: --- → B2G C2 (20nov-10dec)
Ian, option B is fine. We can get proper <select> support in V2.
Yes option B is perfectly fine for v.1
QA Contact: jshih
Hi Ian - any progress on this? Tomorrow is the last day we can land feature work before it requires driver review and evaluation for risk.
Hi Dietrich,
According Comment 9, is it in the milestone C2?
The longer we go on, the harder it will be to land new feature code like this. Regardless of milestone, we need to land it soon. What is your estimation for how long this will take?
Thanks for your reminder. I estimate to give the patch for the feature today.
But I'm sure that could be passed the reviewing task before C1. I'll give the patch soon.
(In reply to Ian Liu [:ianliu] from comment #15)
> Thanks for your reminder. I estimate to give the patch for the feature today.
> But I'm sure that could be passed the reviewing task before C1. I'll give
> the patch soon.
Sorry to miss a word.
I'm "not" sure that could be passed the reviewing task before C1.
Comment on attachment 683101 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/6495

The patch is relative with Value Selector and Audio channel.
So, I think Rudy and Alive are fit to be the reviewer.
Could you help me to review my pr?
Thank you.
Attachment #683101 - Flags: review?(rlu)
Attachment #683101 - Flags: review?(alive)
Comment on attachment 683101 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/6495

r- because this should use the onchange event of a select instead of copying the value_selector building blocks directly inside the application.
Attachment #683101 - Flags: review-
Status: NEW → ASSIGNED
Hi Vivien,

The onchange event will be fired after user select another option and click "OK" button.
In the case, value selector is hided by the "OK".
So, we can not have the preview alarm ringtone during showing value selector.

Could platform have "onclick" event from every options of the value selector?(Likes our Firefox desktop version.)
It will more precise to receive the event for previewing ringtone.
Even if user click on the same option, we can pause and resume the ringtone by the "onclick" event.
(In reply to Ian Liu [:ianliu] from comment #20)
> Hi Vivien,
> 
> The onchange event will be fired after user select another option and click
> "OK" button.

This is not the normal behavior, the onchange event should fired as soon the user choose something. Why is it not set on every time the user choose something? Sounds like a Gaia bug to me.

The behavior you describe sounds the behavior normally used for a checkbox/radio button. Not the one used for a select. (see http://help.dottoro.com/ljoiurdq.php)
Component: Gaia → Gaia::Clock
Hi Vivien, Ian,

For Gaia System's value selector, the onchange would occur after the user clicks [OK] button on the value selector to confirm the selection (and of course, when the selected option is really changed).

This design is implemented for the following 2 reasons,
 1. The user would not accidentally click an unwanted option to trigger onchange events when he/she scrolls the option list.
 2. Our value selector is a full-screen dialog, invoke onchange each time while keeping the value selector shown is kind-of weird to me.

--
Besides, even if Ian can change Clock App. to listen to onchange event, it will be an issue when the user clicks "the same ring tone", there will be no onchange to trigger the preview.

I would suggest we implement an "in-app" value selector as Ian did to make the UX better as described in Comment 8 (option B).

Thanks.
(In reply to Rudy Lu [:rudyl] from comment #22)
> Hi Vivien, Ian,
> 
> For Gaia System's value selector, the onchange would occur after the user
> clicks [OK] button on the value selector to confirm the selection (and of
> course, when the selected option is really changed).
> 
> This design is implemented for the following 2 reasons,
>  1. The user would not accidentally click an unwanted option to trigger
> onchange events when he/she scrolls the option list.

This is the behavior of <select size="xxx"> since ever (see above example) and the UI really looks like it. If the user click an option how can you decide what is accidental or not? If the panning algorithm creates error, let's fix it.

data:text/html,<select onchange="console.log(this.selectedIndex)" size="3"><option>foo</option><option>bar</option><option>foobar</option></select>

>  2. Our value selector is a full-screen dialog, invoke onchange each time
> while keeping the value selector shown is kind-of weird to me.
>

Why? Again this is the behavior for select that are displayed as list. In the worst case what about firing the onchange event on all changes for a select that has the 'size' attribute? That would allow web app authors to not have to re-implement a whole UI/js logic for a basic functionality.

 
> --
> Besides, even if Ian can change Clock App. to listen to onchange event, it
> will be an issue when the user clicks "the same ring tone", there will be no
> onchange to trigger the preview.

The ring tone is already selected and the user can click one and then an other in order to listen it again. Not a big issue.

> 
> I would suggest we implement an "in-app" value selector as Ian did to make
> the UX better as described in Comment 8 (option B).
> 

This would make life harder for web apps author and this will also result into a broken consistency if the app author does not use the last version of the building blocks.

> Thanks.
Comment on attachment 684340 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/6572

I suppose that we can receive "onchange" event when user are selecting on a showing value selector.
So, we can preview the alarm tone by the event.

Please test and review the pr with a revised patch of value selector.
I think Rudy will help on it to support firing "onchange" event when any option are selected immediately on a showing value selector.
Attachment #684340 - Flags: review?(rlu)
Attachment #684340 - Flags: review?(alive)
Comment on attachment 684340 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/6572

Please fix all coding style issues addressed in github comments and make sure this patch is safe to land before rudy's patch, or you should wait until that.

r=me. Thanks!
Attachment #684340 - Flags: review?(alive) → review+
Comment on attachment 684392 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/6581

This is the behavior change for value selector in Gaia system app.
Now we would trigger change event for the <select> element if the selected option is changed before the user clicks [OK] button.

---
Please be informed that this may introduce another issue for "select based navigation".
Take MDN as an example, 
https://developer.mozilla.org/en-US/

we have a <select> element at the bottom of the page for language selection.
If we select one of the languages with Gaia value selector, since the change would be triggered and cause the browser to reload.
We would find that we could receive a blur event to hide the value selector.
(Thanks to Tim to highlight this use case.)

Do you think we could just check in this behavior change first and then handle the above "select based navigation" as a follow-up?
Attachment #684392 - Flags: review?(21)
Comment on attachment 684392 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/6581

Mostly code dancing around. Sounds good to me.
Attachment #684392 - Flags: review?(21) → review+
Comment on attachment 683101 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/6495

Since it's a workaround solution, I close the pr according Vivien's suggestion.
Attachment #683101 - Attachment is obsolete: true
Attachment #683101 - Flags: review?(rlu)
Attachment #683101 - Flags: review?(alive)
(In reply to Alive Kuo [:alive] from comment #26)
> Comment on attachment 684340 [details]
> Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/6572
> 
> Please fix all coding style issues addressed in github comments and make
> sure this patch is safe to land before rudy's patch, or you should wait
> until that.
> 
> r=me. Thanks!
Alive,

I have refined the coding style according your comment.
If Rudy's patch(https://github.com/mozilla-b2g/gaia/pull/6581) is not merged, the preview feature is not working without "change" event when the value selector is showing.
But it will not block anything.
Thanks for your review effort.
(In reply to Vivien Nicolas (:vingtetun) from comment #23)
> (In reply to Rudy Lu [:rudyl] from comment #22) 
>  
> > --
> > Besides, even if Ian can change Clock App. to listen to onchange event, it
> > will be an issue when the user clicks "the same ring tone", there will be no
> > onchange to trigger the preview.
> 
> The ring tone is already selected and the user can click one and then an
> other in order to listen it again. Not a big issue.
> 
Hi Vivien, Rudy,

For the issue, I still think that we should listen "click" event for each <option> of the <select>. It will more precisely catch user's behavior in the long tern. But I'm not sure that the "click" event of <option> is a standard or not. We could have an example as following. It only work on Firefox browser. :)

Example: http://jsfiddle.net/L6kNE/3/
(In reply to Ian Liu [:ianliu] from comment #32)
> (In reply to Vivien Nicolas (:vingtetun) from comment #23)
> > (In reply to Rudy Lu [:rudyl] from comment #22) 
> >  
> > > --
> > > Besides, even if Ian can change Clock App. to listen to onchange event, it
> > > will be an issue when the user clicks "the same ring tone", there will be no
> > > onchange to trigger the preview.
> > 
> > The ring tone is already selected and the user can click one and then an
> > other in order to listen it again. Not a big issue.
> > 
> Hi Vivien, Rudy,
> 
> For the issue, I still think that we should listen "click" event for each
> <option> of the <select>. It will more precisely catch user's behavior in
> the long tern. But I'm not sure that the "click" event of <option> is a
> standard or not. We could have an example as following. It only work on
> Firefox browser. :)
> 
> Example: http://jsfiddle.net/L6kNE/3/

Could be a followup imo :)
This one of the few remaining pieces of feature work. We need to close it ASAP. Can you please have a meeting or talk on IRC to figure out the right fix, so this can be closed?

Ian, for your proposed fix, how long do you expect it to take?
Hi Dietrich,

We need to verify the side effect according Rudy's comment 28. In this case, it should handle the "mozbrowserlocationchange" event to do some action. We will land the feature soon after we finish the verification.

For comment 32, we could be follow in the feature. It will be an issue when the user clicks "the same ring tone", there will be no onchange to trigger the preview.
Hi Ian, Dietrich,

Sorry for the delay on value selector related changes.
I am going to send out the patch for review today.

Thanks.
Hi Vivien,

I've tried your suggestion to listen to "mozbrowserlocationchange" event, however it seems that in system app. I could not receive the event for the tabs in browser app.

My test code is located at:
https://github.com/RudyLu/gaia/commits/value_selector/change_event_forEachClick_locChangeTest

Do you have any other suggestions?
Or maybe we could land this patch first and fix the navigation issue in another bug.
Thanks.
(In reply to Rudy Lu [:rudyl] from comment #37)
> Hi Vivien,
> 
> I've tried your suggestion to listen to "mozbrowserlocationchange" event,
> however it seems that in system app. I could not receive the event for the
> tabs in browser app.
> 
> My test code is located at:
> https://github.com/RudyLu/gaia/commits/value_selector/
> change_event_forEachClick_locChangeTest
> 
> Do you have any other suggestions?
> Or maybe we could land this patch first and fix the navigation issue in
> another bug.
> Thanks.

As said on IRC let's fix the real platform issue for mozbrowserlocationchange.
The changes for value selector itself is merged as,
https://github.com/mozilla-b2g/gaia/pull/6581

Thanks.
Thanks everyone. What is the next step to close this?
Merged https://github.com/mozilla-b2g/gaia/pull/6572
We can close the issue now.

Thank you all.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Yay, thanks everyone!
verified on unagi
2012-11-28
gaia master : 47823b5932121b3a2c0b2410174f02b2b740cd3d
mozilla-beta : 003e1843e1d8
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: