1195231 - [Ringtones]The singer of a song is too close to next song 's name in Select Sound view if there are more than one song in My Ringtones.

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Ringtones, defect, P2)

Description

[Annabelle]

Attachment: Bug log: logcat_1551.txt

[1.Description]:

[Aries kk v2.5][Flame kk v2.5][Settings]The author of a song is too close to next song 's name, and  the second song can not diaplay singer in Select Sound view, if there are more than one song in My Ringtones.

Found time:15:51
See attachment:logcat_1551.txt & Aries_kk_2.5.3gp

[2.Testing Steps]: 
Premise: There are more than one song both with authors.
1.Launch Music and share 2 songs to Ringtones.
2.Get into Settings->sounds->Ringtones->My Ringtones
3.Observe the songs and singer.

[3.Expected Result]: 
3.The singer should be displayed within the seperating line, not too closed to the next song and every ringtone song should have its singer,and the circle that you can choose  does  display correctly.
[4.Actual Result]: 
3.The author of previous song is too close to the name of next song,  the second song does not have singer,and the circle that you can choose  does not display correctly.
[5.Reproduction build]: 
Device: Aries KK 2.5(Affected)
Build ID               20150814002719
Gaia Revision          39b121515ab8a8c3ea07f26d3ba1dd792e90217c
Gaia Date              2015-08-13 18:25:42
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/4e883591bb5dff021c108d3e30198a99547eed1e
Gecko Version          43.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20150813.235024
Firmware Date          Thu Aug 13 23:50:32 UTC 2015
Bootloader             s1

Device: Flame KK 2.5(Affected)
Build ID               20150816150205
Gaia Revision          47c91ffe7f500ca1aaa60de0aabf4d2429120733
Gaia Date              2015-08-14 18:55:02
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/0876695d1abdeb363a780bda8b6cc84f20ba51c9
Gecko Version          43.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150816.182145
Firmware Date          Sun Aug 16 18:21:56 EDT 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0

Device: Flame KK 2.2(Unaffected)
Build ID               20150816032505
Gaia Revision          335cd8e79c20f8d8e93a6efc9b97cc0ec17b5a46
Gaia Date              2015-08-14 19:06:41
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/82ec88fa015c
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150816.070759
Firmware Date          Sun Aug 16 07:08:10 EDT 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0

[6.Reproduction Frequency]: 
Always Recurrence,5/5

[7.TCID]: 
Free Test

Comment 1

[Annabelle]

Attachment: Bug video: Aries_kk_2.5.3gp

Comment 2

[William Hsu [:whsu]]

Bug can be reproduced on v2.5 Flame-KK and Aries-KK.
Request for UI enhancement.

Comment 3

[Jim Porter (:squib)]

[Blocking Requested - why for this release]:

Comment 4

[Jim Porter (:squib)]

Can we get a regression range on this? I'm guessing it's from bug 1186279.

Comment 5

[Hema Koka [:hema]]

Minor Ux issue, regression.

Comment 6

[Duane Dixon [:ddixon]]

B2G Inbound Regression Window

Last Working 

Device: Flame 2.5
BuildID: 20150806083401
Gaia: 00900a0f9e20694da0a770c4fb4c138c96dcc937
Gecko: 73c06f1bd68f
Version: 42.0a1 (2.5) 
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:42.0) Gecko/42.0 Firefox/42.0

First Broken

Device: Flame 2.5
BuildID: 20150806085259
Gaia: 7f387f859d48f9ad0761637c78447dc524747738
Gecko: 566ad956305d
Version: 42.0a1 (2.5) 
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:42.0) Gecko/42.0 Firefox/42.0

Last Working Gaia and First Broken Gecko
Issue DOES NOT occur here. 
Gaia: 00900a0f9e20694da0a770c4fb4c138c96dcc937
Gecko: 566ad956305d

Last Working Gecko and First Broken Gaia
Issue DOES occur here. 
Gaia: 7f387f859d48f9ad0761637c78447dc524747738
Gecko: 73c06f1bd68f

B2G Inbound Pushlog (Gaia): 
https://github.com/mozilla-b2g/gaia/compare/00900a0f9e20694da0a770c4fb4c138c96dcc937...7f387f859d48f9ad0761637c78447dc524747738

Possible Cause: 

Bug 1186279 - Convert ringtones switches to use web components

Comment 7

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

Kevin, the changes to Bug 1186279 seem to be related to this issue.  Can you take a look?

Comment 8

[GitHub Autolander Bot]

Attachment: [gaia] KevinGrandon:bug_1195231_ringtones_list_item_height > mozilla-b2g:master

Comment 9

[Kevin Grandon :kgrandon]

Comment on attachment 8650862 [details] [review]
[gaia] KevinGrandon:bug_1195231_ringtones_list_item_height > mozilla-b2g:master

Jim - could you review this when you get a chance?

This is a workaround for multi-line gaia components. We're going to be patching the shared list styles in the future to handle this, but this should work for the time being.

Comment 10

[Hema Koka [:hema]]

Kevin, since you are on it, assigning to you. thanks!

Comment 11

[Jim Porter (:squib)]

Attachment: UI with the patch

Unfortunately, this still doesn't work. Here's what it looks like in the picker...

Comment 12

[Jim Porter (:squib)]

Attachment: Correct UI (from the manager)

...and here's what it should look like (except for the different icons).

Comment 13

[Kevin Grandon :kgrandon]

Comment on attachment 8650862 [details] [review]
[gaia] KevinGrandon:bug_1195231_ringtones_list_item_height > mozilla-b2g:master

Jim - thank you for the screenshots, they were very useful. Please take another look if you have a chance.

Since we are switching over to a component model, I've changed the details object to be passed into the radio control, as this is what we do with the other form controls elsewhere in the OS (in settings for example). We're also using a web component which does not typically  truncate the label contents. Since this is what is expected elsewhere in the OS and I am fairly sure that we don't want to differ in this implementation here, but let me know if you want me to flag a UX person for approval.

Comment 14

[Jim Porter (:squib)]

I definitely think we should be truncating these items, since the default titles can be quite long (they're whatever is in the "title" field for a song). Let's ask UX though.

Tif: Should the ringtones' names be truncated with ellipses at the end, or should we wrap the text and show it all? We currently do the former, which I think is the right decision, but apparently we're moving away from that elsewhere in the OS.

Comment 15

[Tiffanie Shakespeare [:tif]]

I'm not aware of our components changing as Kevin mentions. I would prefer truncation as well, but of course we should be following component guidelines here. 

So I'm going to switch the NI to przemek since he's working on those. He's on PTO right now but should be able to answer next week.

Thanks!

Comment 16

[Przemek Abratowski [:przemek] UX]

The video shows that the 2 line ringtone list item is not correct for sure. Hung is currently looking at a list component guideline now so I'll forward this to him. We never had lists as components and we're trying to add them now.

Comment 17

[Hung Nguyen]

The latest screen that Jim posted is correct (Correct UI (from the manager)). There should be 2 lines of text and the text should truncate if the string becomes too long. 

Wilson - can we use the new list component for this case?

Comment 18

[Wilson Page [:wilsonpage]]

Sure, it would be good to have more apps trying it out. Jim, you can use Bower to install the list [1] into the Ringtones app. It's early days so docs are a little flakey. You're best bet it to look at the examples [2] to see how to use <gaia-fast-list> web-component.

[1] https://github.com/gaia-components/fast-list
[2] https://github.com/gaia-components/fast-list/tree/master/examples/gaia-fast-list

Comment 19

[Wilson Page [:wilsonpage]]

Alternatively you can install it into /shared/ and use it like we're using gaia-header.

1. cd gaia/shared
2. bower install gaia-components/fast-list --save
3. Add necessary assets to <head>

Comment 20

[Wilson Page [:wilsonpage]]

Just a heads up that I'll be splitting the gaia-fast-list web-component from the fast-list repo this week. So the component you're after will soon live within github.com/gaia-components/gaia-fast-list.

Comment 21

[Jim Porter (:squib)]

In that case, let's just back out the original patch for now. It sounds like the original patch wasn't enough to do what we need, so I think it would be easier in the long run to just start over. Kevin, you know more about the current state of the radio button building block; could you back out your patch from bug 1186279 for now?

Comment 22

[Kevin Grandon :kgrandon]

We can backout, but I don't think that's the right move. I also disagree about the truncation call, as this doesn't work for all of our apps (thus going against some of the wins of using a component).

For now though, we should be able to add truncation quite easily by passing in an attribute (we shouldn't need to use the fast list component for this). I'll look at providing a quick fix for this soon.

Comment 23

[Jim Porter (:squib)]

This has been broken for a month now, and while it's just a UI bug, I'd rather have the tree remain in a fully-working state rather than wait for patches to fix regressions.

From the above, it also sounds like the "right" solution is probably to use the list component to handle truncation, and then the radio component doesn't need to worry about it. If that's the case, then the original patch is probably going in the wrong direction anyway, and would need to be undone once we convert to using the list component. With that in mind, I think a backout makes the most sense.

(In reply to Kevin Grandon :kgrandon from comment #22)
> I also disagree about the truncation call, as this doesn't work for all of
> our apps (thus going against some of the wins of using a component).

I don't think this is contradictory to the goals of components. Generally speaking, if a web component can't handle being used in multiple ways, then we've either 1) made an incomplete web component, or 2) picked the wrong thing to componentize.

Comment 24

[Kevin Grandon :kgrandon]

The list component won't be able to handle truncation, but likely a future <gaia-label> component will be able to. For now, I've added the ability for a <gaia-radio> component to truncate by adding a class="truncate" onto the element, which feels like a reasonable API.

Jim - the pull requested has been updated, please take a look at the latest code when you get a chance.

Comment 25

[Jim Porter (:squib)]

Comment on attachment 8650862 [details] [review]
[gaia] KevinGrandon:bug_1195231_ringtones_list_item_height > mozilla-b2g:master

Sorry for taking so long to review this! I've been swamped with Music reviews lately.

Unfortunately, it seems that the list item is too tall when it has a subtitle. I also just noticed that for the non-subtitle case, the text isn't vertically centered. This happens before the patch in this bug as well. Finally, there doesn't seem to be enough padding between the text and the radio button, since the ellipsis can get awfully close.

I'm going to be out for the next couple of days, but I'd like to suggest again that we back the original patch out. It might also be useful to get ui-review on any future patches, since the UX folks have a good eye for visual changes. (I should have been a bit more careful in my initial review, but lesson learned I suppose.)

Comment 26

[Kevin Grandon :kgrandon]

>  I'd like to suggest again that we back the original patch out

I can do that, but it's more risky to do as previous code is outdated and has been removed (that code also didn't have RTL fixes that the new component has).

Comment 27

[Kevin Grandon :kgrandon]

Attachment: New-Screenshot-Ringtones-adjusted-radio-sizing.png

I've uploaded a new patch, and here is a screenshot of the implementation. The changes implemented here are taken from the system app (which will eventually be implemented in the gaia-list component.

I'm looking for a UX review before opening up the patch for review again. I'm not sure who should do this, perhaps Tiffanie as I saw you on some previous ringtones bugs. Also adding Hung as he's experienced with components. Note: this implementation currently matches the system-wide v1 components in gaia. We can update styling if needed, but it would update across the OS. Thanks!

Comment 28

[Kevin Grandon :kgrandon]

Comment on attachment 8650862 [details] [review]
[gaia] KevinGrandon:bug_1195231_ringtones_list_item_height > mozilla-b2g:master

This is ready for re-review, but waiting for a ui-review before sending it over.

Comment 29

[Hung Nguyen]

Comment on attachment 8670592 [details]
New-Screenshot-Ringtones-adjusted-radio-sizing.png

Looks good to go. Thanks Kevin.

Comment 30

[Tiffanie Shakespeare [:tif]]

Comment on attachment 8670592 [details]
New-Screenshot-Ringtones-adjusted-radio-sizing.png

whoops didn't hit send :)

Looks good - thanks Kevin!

Comment 31

[Tiffanie Shakespeare [:tif]]

Comment on attachment 8670592 [details]
New-Screenshot-Ringtones-adjusted-radio-sizing.png

sigh - see previous comment...

Comment 32

[Kevin Grandon :kgrandon]

Comment on attachment 8650862 [details] [review]
[gaia] KevinGrandon:bug_1195231_ringtones_list_item_height > mozilla-b2g:master

Jim - we've got a double ui-review now for the screenshot (https://bug1195231.bmoattachments.org/attachment.cgi?id=8670592).

Could you please take another look if you got a chance. The gaia-radio work can be split out into another bug if needed, just want to make sure that you are happy with this approach. Once we have a gaia-list component implemented, we'll remove that extra CSS from the ringtones app. Thanks for working on this with me.

Comment 33

[Jim Porter (:squib)]

Comment on attachment 8650862 [details] [review]
[gaia] KevinGrandon:bug_1195231_ringtones_list_item_height > mozilla-b2g:master

The two-line version of the list item is still 1.1rem too tall. It should be exactly 6rem tall like the one-line version. I also notice that the font size for the main item is 1.9rem, when it used to be 1.8rem, as per the spec in bug 960329. There might be a more-complete visual spec, but I'm having trouble finding it.

Comment 34

[Jim Porter (:squib)]

Another thing I noticed: when you tap on a ringtone in the picker, it should be highlighting the entire row. However, the highlight is offset and looks bad. See the spec in bug 1020845 (the 4th attachment is most relevant).

Sorry for not being as thorough reviewing the initial patch; I've been swamped with reviews due to the NGA Music work and probably just tried to get it off my plate as quickly as possible.

Comment 35

[Kevin Grandon :kgrandon]

Thanks for taking a look.

(In reply to Jim Porter (:squib) from comment #33)
> The two-line version of the list item is still 1.1rem too tall. It should be
> exactly 6rem tall like the one-line version. I also notice that the font
> size for the main item is 1.9rem, when it used to be 1.8rem, as per the spec
> in bug 960329. There might be a more-complete visual spec, but I'm having
> trouble finding it.

The styles are correct for the new web components, and that's what we're switching to. Custom styles per-app should be avoided as that defeats the purpose of web components. The components have already undergone a visual review, and Hung as ui-reviewed them already.

(In reply to Jim Porter (:squib) from comment #34)
> Another thing I noticed: when you tap on a ringtone in the picker, it should
> be highlighting the entire row. However, the highlight is offset and looks
> bad. See the spec in bug 1020845 (the 4th attachment is most relevant).

I'll take a look at the hightlighting - there's probably an issue there due to the list styles.

Comment 36

[Jim Porter (:squib)]

(In reply to Kevin Grandon :kgrandon from comment #35)
> The styles are correct for the new web components, and that's what we're
> switching to. Custom styles per-app should be avoided as that defeats the
> purpose of web components. The components have already undergone a visual
> review, and Hung as ui-reviewed them already.

Nevertheless, the overall height of the two-line item should match that of the one-line item (unless we have an actual spec saying otherwise).

Also, if we're moving to 1.9rem for the font size, we should probably update the ringtone manager's styles so that they're consistent.

Comment 37

[Kevin Grandon :kgrandon]

Comment on attachment 8650862 [details] [review]
[gaia] KevinGrandon:bug_1195231_ringtones_list_item_height > mozilla-b2g:master

Hi Jim, thanks for taking a deep look here. I've made some updates and I think we're close now, please take another look if you can.

(In reply to Jim Porter (:squib) from comment #36)
> Nevertheless, the overall height of the two-line item should match that of
> the one-line item (unless we have an actual spec saying otherwise).

This will likely change in the future (already different in the settings app), but for now I've restored the previous behavior to not change the height. All items should remain the same size now.

> Also, if we're moving to 1.9rem for the font size, we should probably update
> the ringtone manager's styles so that they're consistent.

We should move to 1.9rem, but for the time being I've just added an override of the label to be 1.8, and we can re-evaluate when there's more time, or when we go to the gaia-list component.

Also, the highlighting should be fixed now.

Comment 38

[Jim Porter (:squib)]

Comment on attachment 8650862 [details] [review]
[gaia] KevinGrandon:bug_1195231_ringtones_list_item_height > mozilla-b2g:master

Ok, everything looks good to me. Thanks for fixing this!

Comment 39

[Kevin Grandon :kgrandon]

Thank you very much for the review!

In master: https://github.com/mozilla-b2g/gaia/commit/b1b593aca34282803b6eb72eb499c31df5f333e2

Comment 40

[Carrie.Yang Leave from Mozilla]

This bug has been verified as pass on latest build of Flame KK v2.5 and Aries KK v2.5 by the STR in comment 0.
Actual results: In Manage Tones page and Select Sound page, the space between items are appropriate. The singer is displayed within the seperating line, not too closed to the next song, and the circle that you can choose does display correctly.
See attachment: Verified_Aries_v2.5_ringtones.png.
Reproduce rate: 0/10.

Device: Flame KK v2.5 (Pass)
Build ID               20151025150204
Gaia Revision          1c6628ed1e40575e5ec3669ab6ef389d4ebeea65
Gaia Date              2015-10-23 17:01:43
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/d53a52b39a95dced722cca90ac74529b66dd5253
Gecko Version          44.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20151025.201952
Firmware Date          Sun Oct 25 20:20:05 EDT 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0

Device: Aries KK v2.5 (Pass)
Build ID               20151025085903
Gaia Revision          1c6628ed1e40575e5ec3669ab6ef389d4ebeea65
Gaia Date              2015-10-23 17:01:43
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/d53a52b39a95dced722cca90ac74529b66dd5253
Gecko Version          44.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20151025.081846
Firmware Date          Sun Oct 25 08:18:54 UTC 2015
Bootloader             s1

Comment 41

[Carrie.Yang Leave from Mozilla]

Attachment: Verified_Aries_v2.5_ringtones.png