1009133 - [music] The "play" icon in the header appears even when nothing is playing

Status: VERIFIED FIXED

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

Description

[Jim Porter (:squib)]

I think this is fallout from some building blocks changes. The "hidden" attribute on the play icon is getting ignored because some CSS explicitly sets "display: block;" on the element, and that CSS has higher specificity. We should fix this.

The easy way is just to add some CSS to make the "hidden" attribute hide the button, but maybe now would be a good time to update the header to use web components. (Or maybe not, depending on our plans with music2). Thoughts?

Comment 1

[Jason Smith [:jsmith]]

Can we get STR that you used to reproduce this?

Comment 2

[Jim Porter (:squib)]

Ok.

1) Open the music app.

Comment 3

[Jason Smith [:jsmith]]

Two things:

1. Can we get a screenshot?
2. Can we confirm this is not present on 1.4?

Comment 4

[Joshua Mitchell (Inactive)]

This does NOT repro on the 1.4 Buri

1.4 Environmental Variables:
Device: Buri 1.4 MOZ
BuildID: 20140508000201
Gaia: 4ce973ef0732b0d52cb043210db598aa176b2ce9
Gecko: 16ab7f6b18f8
Version: 30.0
Firmware Version: v1.2-device.cfg


This does NOT repro on 1.4 Open_C

1.4 Environmental Variables:
Device: Open_C 1.4 MOZ
BuildID: 20140512000204
Gaia: 17fb44880e95bc7ae363a609d811bf5a9a067b5b
Gecko: ec24f847e7c0
Version: 30.0
Firmware Version: FFOS_US_EBAY_P821A10V1.0.0B06_LOG_D

Comment 5

[Jim Porter (:squib)]

Attachment: The issue

Here's a screenshot. 

I also tried with the latest 1.4 build, and everything looks ok there.

Comment 6

[Dominic Kuo [:dkuo]]

(In reply to Jim Porter (:squib) from comment #0)
> I think this is fallout from some building blocks changes. The "hidden"
> attribute on the play icon is getting ignored because some CSS explicitly
> sets "display: block;" on the element, and that CSS has higher specificity.
> We should fix this.
> 
> The easy way is just to add some CSS to make the "hidden" attribute hide the
> button, but maybe now would be a good time to update the header to use web
> components. (Or maybe not, depending on our plans with music2). Thoughts?

I feel this is more like an issue on the css style of the building blocks, or gecko?, if |hidden| attribute is set to true I would expected the element is invisible, but apparently it's not for now, maybe we should read the spec to see which attribute overrides then decide what fix we should use.

Comment 7

[Jim Porter (:squib)]

Looks like this issue got lost. We need to fix this for 2.0, since it looks really bad!

Comment 8

[Dominic Kuo [:dkuo]]

(In reply to Jim Porter (:squib) from comment #7)
> Looks like this issue got lost. We need to fix this for 2.0, since it looks
> really bad!

Yes! totally forgot this and we should definitely fix this, taking it.

Comment 9

[Dominic Kuo [:dkuo]]

Attachment: patch

Comment 10

[Sri Kasetti]

We should fix this, bad user experience

Comment 11

[Nawneet Kumar]

I checked with Dominic's patch and this issue is not present.

Comment 12

[Dominic Kuo [:dkuo]]

Comment on attachment 8449534 [details] [review]
patch

Jim,

This is a simple patch with tests but my approach needed to add !important to the player icon's style, which couldn't pass the csslint and I have to modify the build/csslint/xfail.list to ignore it. I am still thinking what's an alternative way to fix this but those feels like workaround so I have wrapped it first for you to review, do you think it's acceptable? or probably you have some suggestions and I would love to hear them, thanks!

Comment 13

[Jim Porter (:squib)]

Comment on attachment 8449534 [details] [review]
patch

I commented on the pull request. I think we should try to avoid setting the style attribute directly in JS if we can...

Comment 14

[Dominic Kuo [:dkuo]]

(In reply to Jim Porter (:squib) from comment #13)
> Comment on attachment 8449534 [details] [review]
> patch
> 
> I commented on the pull request. I think we should try to avoid setting the
> style attribute directly in JS if we can...

Thanks Jim, that's also what I thought at first and has hesitated to use this approach, but looks like your way is easier and doesn't need JS involved(also fixed the case in pick activity), so let's use it :)

Comment 15

[Dominic Kuo [:dkuo]]

Comment on attachment 8449534 [details] [review]
patch

Issues are addressed, mind to review again? thanks.

Comment 16

[Jim Porter (:squib)]

Comment on attachment 8449534 [details] [review]
patch

Cool, looks good! (Note that I didn't actually run with this patch.)

Comment 17

[Dominic Kuo [:dkuo]]

Thanks Jim! I have checked it works before landing it.

master: 5886383cd18a79cbe4f4c5b1fe6129b2c9c7a27c

Comment 18

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/5ee3216a5cc54abda3a573378fbbd9990412fc17

Comment 19

[Coler]

This issue has been verified successfully on Flame2.0&2.1.
Reproducing rate: 0/5
See attachment: Verify_Flame_Music.mp4

Flame2.0 build version:
Gaia-Rev        8d1e868864c8a8f1e037685f0656d1da70d08c06
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/c756bd8bf3c3
Build-ID        20141201000201
Version         32.0

Flame2.1 build version:
Gaia-Rev        ccb49abe412c978a4045f0c75abff534372716c4
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/18fb67530b22
Build-ID        20141201001201
Version         34.0

Comment 20

[Coler]

Attachment: Verify_Flame_Music.MP4

Comment 21

[Hubert Figuiere [:hub]]

Is there a reason why a test was disabled at https://github.com/mozilla-b2g/gaia/pull/21294/files#diff-c6184496bf60ffae2f21ac71bd7a648aR93 ?

Comment 22

[Hubert Figuiere [:hub]]

Jim replied. It is just local disable vs file disable that changed in this bug - because the new test is in the file. Original disabling due to bug 1032037. Case closed. Thanks.