1066830 - Hide theme menu if there is no certified theme app

Status: VERIFIED FIXED

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

Description

[Evelyn Hung]

[Blocking Requested - why for this release]:

We don't have certified theme app now, so the UI will display a blank page when you navigate into "Themes" in Settings which makes it looks like broken. We should hide the menu in this case.

Comment 1

[Olle Klang]

I don't believe we should hide the menu, It's really a step backwards, instead we should add a default theme.

Comment 2

[Olle Klang]

Attachment: GitHub pull-request

Comment 3

[[:fabrice] Fabrice Desré]

Hiding the menu when there is no theme is the correct solution. Whether we want a default theme that lives in its own app is a different topic.

And honestly, the PR attached there looks very risky for a late landing. We need to find a simpler solution there.

Comment 4

[Tony Chung [:tchung]]

(In reply to Fabrice Desré [:fabrice] from comment #3)
> Hiding the menu when there is no theme is the correct solution. Whether we
> want a default theme that lives in its own app is a different topic.
> 
> And honestly, the PR attached there looks very risky for a late landing. We
> need to find a simpler solution there.

I agree.  we really should get this 2.1+'d and uplifted.  Showing the user a blank themes option that does nothing is bad UI and confusing feature.   If any other partner takes 2.1 and doesnt use themes, they will see this ridiculous setting that does nothing.

Comment 5

[Tony Chung [:tchung]]

changing title back to original, since that seems to be the suggested plan of action.

Comment 6

[howie [:howie]]

Triage: blocking, regression and bad user experience. Hi Olle, are you going to work on this? Thanks.

Comment 7

[Olle Klang]

Yes, I'll provide a solution.

Comment 8

[Olle Klang]

Attachment: gi

Comment 9

[Olle Klang]

Attachment: GitHub pull-request

We could go for a similar solution used for the homescreen section. That solution is slow because we would need to call appsMgmt.getAll and look for installed themes. Users would notice a lag and the themes section would show up several seconds after all other sections. 

I don't like that solution and we would probably revert it immediately on our side as it is not acceptable from an UX perspective.

Instead I propose the attached solution where we simply hide the section with css and let it be up to the themes to unhide it as changing css is just what themes are good at. This is simple and we could probably not do it faster. 

I'll attach a patch with test themes to try it out.

Comment 10

[Olle Klang]

Attachment: 0001-Test-themes-and-settings-for-testing-the-themes-sett.patch

Test themes for testing the themes settings section.

Comment 11

[[:fabrice] Fabrice Desré]

(In reply to Olle Klang from comment #9)
> Created attachment 8490010 [details] [review]
> GitHub pull-request
> 
> We could go for a similar solution used for the homescreen section. That
> solution is slow because we would need to call appsMgmt.getAll and look for
> installed themes. Users would notice a lag and the themes section would show
> up several seconds after all other sections. 

getAll() is not "several seconds" slow. And since we already call it for the homescreen section we can just piggyback on this call, right? no need to call it twice.


> Instead I propose the attached solution where we simply hide the section
> with css and let it be up to the themes to unhide it as changing css is just
> what themes are good at. This is simple and we could probably not do it
> faster. 

The issue with that is that a malicious theme could keep the section hidden to prevent the user from switching to another theme. Not great...

Comment 12

[Olle Klang]

(In reply to Fabrice Desré [:fabrice] from comment #11)
> getAll() is not "several seconds" slow. And since we already call it for the
> homescreen section we can just piggyback on this call, right? no need to
> call it twice.

No getAll is not that slow, but the homescreens section (not homescreen, sorry I was unclear) does exacly what we would do and initially caused a startup delay of 200 ms, see bug 958318. To solve that, a timeout of several seconds was added and that's what I was referring to. Now if I was to use a similar approach I would either cause a similar problem described in bug 958318 or we would have to accept a lag of several seconds. With that in mind I'm still voting for the proposed solution. 

> The issue with that is that a malicious theme could keep the section hidden
> to prevent the user from switching to another theme. Not great...

True, but that issue is already present in the framework and will be there even if we don't use this solution. A malicious theme could do much worse things than that. That's one of the reasons we only support certified themes right?

Comment 13

[[:fabrice] Fabrice Desré]

getAll() performance and cost has improved a lot since bug 958318 landed. I really recommend you to test with a much shorter timeout.

Comment 14

[Olle Klang]

(In reply to Fabrice Desré [:fabrice] from comment #13)
> getAll() performance and cost has improved a lot since bug 958318 landed. I
> really recommend you to test with a much shorter timeout.

I've updated the PR. It's not as fast as I would like it to be but if we can compromise on enabling/disabling the section instead of showing/hiding it I'm OK with it.

What do you think?

Comment 15

[[:fabrice] Fabrice Desré]

(In reply to Olle Klang from comment #14)
> (In reply to Fabrice Desré [:fabrice] from comment #13)
> > getAll() performance and cost has improved a lot since bug 958318 landed. I
> > really recommend you to test with a much shorter timeout.
> 
> I've updated the PR. It's not as fast as I would like it to be but if we can
> compromise on enabling/disabling the section instead of showing/hiding it
> I'm OK with it.
> 
> What do you think?

I'm not a UX guy but I'm quite sure they will prefer to have the same behavior for themes as for the homescreen session. How fast is it?

Comment 16

[howie [:howie]]

ni Rob for UX opinion and review, thanks.

Comment 17

[Olle Klang]

Attachment: hidden_shown.png

Comment 18

[Olle Klang]

Attachment: enable_disable.png

Comment 19

[Olle Klang]

(In reply to Fabrice Desré [:fabrice] from comment #15) 
> I'm not a UX guy but I'm quite sure they will prefer to have the same
> behavior for themes as for the homescreen session. How fast is it?

Using a minimum timeout of 50 ms, the time from "DOMContentLoaded" until the setting is displayed is ~2150 ms. If I measure from tryEnableThemeSection is called until the setting is displayed I get ~ 710 ms.

I've uploaded screenshots for UX to decide.

Comment 20

[howie [:howie]]

Hi Maria, we need UX opinion here, thanks!

Comment 21

[Maria Sandberg [:mushi]]

Hi there,

I'd rather hide the menu all together until there is functionality that can be used from within the settings menu. Having it disabled implies that there is something wrong or something the user needs to do to enable it which I imagine will cause confusion. 

Having functionality that can be used from the themes menu to me means having at least two themes (preferably more) that the user can switch between and that in some noticeable way changes the look of the system.

Comment 22

[howie [:howie]]

Hi Olle, based on the UX feedback, let's go for the hidden solution then, thank you.

Comment 23

[Olle Klang]

Comment on attachment 8490010 [details] [review]
GitHub pull-request

Updated PR to only show the themes section if two or more themes are installed.

Comment 24

[[:fabrice] Fabrice Desré]

Comment on attachment 8490010 [details] [review]
GitHub pull-request

redirecting to a settings peer.

Comment 25

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8490010 [details] [review]
GitHub pull-request

EJ, could you help review the patch? Thanks.

Comment 26

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8490010 [details] [review]
GitHub pull-request

Hi Olle,

basically this patch looks nice. But after discussing with Arthur offline, we are trying to make our codebase more structure and for new check-in panels, we all need to follow AMD styles if possible and add unit tests for it.

I left some comments on Github and please go check it ! By the way, Due to the tree close happened recently, you have to re-open your PR after tree is open and remember to rebase to latest codebase.

Feel free to set r? on me when related fixed are done. THanks :)

Comment 27

[howie [:howie]]

Hi Olle, are you working on EJ's feedback? Thanks.

Comment 28

[Olle Klang]

(In reply to howie [:howie] from comment #27)
> Hi Olle, are you working on EJ's feedback? Thanks.

I'll try to find time for it this week.

Comment 29

[howie [:howie]]

Hi Olle, seems like this one can't be done before 2.1 FC Oct. 10th, please help to put ETA on the whiteboard, e.g. [ETA:10/17], thanks.

Comment 30

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

It seems Olle is missing ... Let me take this bug.

Comment 31

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Attachment: patch on master

Arthur, I just made a patch for this bug and please give me some feedbacks about it.

If there is no big problem for the patch, I will start to add tests and jsdocs ! Thanks

Comment 32

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Attachment: patch on v2.1

Same patch taken from master with some tweaks because there are conflicts between master and v2.1

Comment 33

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Tests are all added for master & 2.1, please check them ! THanks

Comment 34

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Tests are all added for master & 2.1, please check them ! THanks

Comment 35

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8505234 [details] [review]
patch on master

Looks good to me! Thanks.

Comment 36

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8505234 [details] [review]
patch on master

Great, thanks Arthur. I think we are ready for review now.

Comment 37

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8505235 [details] [review]
patch on v2.1

This one too !

Comment 38

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8505234 [details] [review]
patch on master

The code and tests look good to me. r=me with jsdocs added, thanks!

Comment 39

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8505235 [details] [review]
patch on v2.1

r=me with jsdocs added, thanks.

Comment 40

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Merged into gaia/master : https://github.com/mozilla-b2g/gaia/commit/0ca9aec276129343955ad23ffc4c37365a192d6a

Comment 41

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8505235 [details] [review]
patch on v2.1

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined: Users would see useless theme panel and can't change to different theme.
[Testing completed]: yes
[Risk to taking this patch] (and alternatives if risky): lwo
[String changes made]: no

Comment 42

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Merged into Gaia/v2.1 : https://github.com/mozilla-b2g/gaia/commit/c86825e4ee8cf1d8a31b9e2241d06b2710dc6519

Thanks all.

Comment 43

[Yeojin Chung [:YeojinC]]

This issue is verified fixed on Flame 2.2 and 2.1.

"Theme" menu does not appear on the Settings page.

Flame 2.2 

Device: Flame 2.2 Master (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141021040206
Gaia: 457a54fc3200b80e4f5e1cd3acaa062309230732
Gecko: 29fbfc1b31aa
Gonk: 05aa7b98d3f891b334031dc710d48d0d6b82ec1d
Version: 36.0a1 (2.2 Master)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Flame 2.1 

Device: Flame 2.1 (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141021001201
Gaia: e458f5804c0851eb4e93c9eb143fe044988cecda
Gecko: ee86921a986f
Gonk: 05aa7b98d3f891b334031dc710d48d0d6b82ec1d
Version: 34.0 (2.1)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0