809308 - Add an option to the download prompt to download only the system update

Status: RESOLVED FIXED

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

Description

[Etienne Segonzac (:etienne)]

We want to let the user decide to download only the system update on the download prompt.

UX needs to be defined.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This is a requirement from product. Though if this turns out to be too hard, or takes too long to land, then I would say that we should reconsider blocking.

Comment 2

[Chris Lee [:clee]]

This is critical for users to be able to stay up to date on the latest version. 

We do not want to block OS updates if a user doesn't want to update a 3rd party application.  

Josh, can you provide the link to the UX spec for this?  Thanks.

Comment 3

[Jason Smith [:jsmith]]

I don't understand why this doesn't fit in the system component. I thought it did?

Comment 4

[Etienne Segonzac (:etienne)]

(In reply to Jason Smith [:jsmith] from comment #3)
> I don't understand why this doesn't fit in the system component. I thought
> it did?

It does.
Not sure what happened.

Comment 5

[Alex Keybl [:akeybl]]

Milestoning for C2 (deadline of 12/10), as this meets the criteria of "remaining P1 bugs not already milestoned for C1".

Comment 6

[Etienne Segonzac (:etienne)]

Attachment: Spec

final spec for this screen

Comment 7

[Etienne Segonzac (:etienne)]

Attachment: Patch proposal

Had to fix a small bug along the way to enable me to test this correctly (as in manual tests). Hope you won't mind.

Comment 8

[Julien Wajsberg [:julienw]]

Comment on attachment 684511 [details] [diff] [review]
Patch proposal

Review of attachment 684511 [details] [diff] [review]:
-----------------------------------------------------------------

I suspect something's missing: if nothing is selected, "Download" should be disabled.

Good job !

::: apps/system/js/update_manager.js
@@ +172,2 @@
>        var listItem = document.createElement('li');
>        listItem.textContent = updatable.name;

I'd really put the name inside the label :

- immediate benefit: the user can tap the name of the application to check the box. Big utilisability win !
- free future accessibility benefit (when we'll have a screen reader)

However we can open another bug if that's too much CSS work. (I'd rather not because I'm sure it would be done like in 1 year)

@@ +172,4 @@
>        var listItem = document.createElement('li');
>        listItem.textContent = updatable.name;
>  
> +      // The user can choose not to update an app

OMG a comment !

(that's ok, you'll be fine)

@@ +172,5 @@
>        var listItem = document.createElement('li');
>        listItem.textContent = updatable.name;
>  
> +      // The user can choose not to update an app
> +      if (updatable.app) {

why not use instanceof AppUpdatable ? seems better than using internal properties from AppUpdatable.

Better: why not put this inside AppUpdatable and SystemUpdatable, and you'd just have to call "renderDownloadBox" (or something like that) on the Updatable implementation?

@@ +180,5 @@
> +        checkbox.type = 'checkbox';
> +        checkbox.dataset.position = index;
> +        checkbox.checked = true;
> +
> +        var span = document.createElement('span');

what is this |span| for ?

@@ +192,5 @@
> +        label.textContent = _('required');
> +        label.classList.add('required');
> +
> +        listItem.appendChild(label);
> +      }

you can put the "createElement" and "appendChild" calls out of the |if| block.

::: apps/system/locales/system.en-US.properties
@@ +95,4 @@
>  updatesAvailableMessage[many]={{ n }} updates available. <span>Tap to download.</span>
>  updatesAvailableMessage[other]={{ n }} updates available. <span>Tap to download.</span>
>  updates={[ plural(n) ]}
> +updates[one]={{ n }} Update available

I'd put "One update available"

@@ +98,5 @@
> +updates[one]={{ n }} Update available
> +updates[two]={{ n }} Updates available
> +updates[few]={{ n }} Updates available
> +updates[many]={{ n }} Updates available
> +updates[other]={{ n }} Updates available

I'd say we should remove the capital letter in Updates.

@@ +103,2 @@
>  systemUpdate=System update
> +required=Required

you can remove the capital since you're capitalizing everything in CSS anyway

::: apps/system/style/update_manager/update_manager.css
@@ +116,5 @@
>    pointer-events: auto;
>  }
>  
> +#updates-download-dialog h1 {
> +  font-size: 2.2rem;

why do we need specifically this for this specific dialog ?

@@ +167,5 @@
> +  float: right;
> +
> +  line-height: 5rem;
> +  font-size: 1.3rem;
> +  text-transform: uppercase;

Maybe move the transform to the next rule.

::: apps/system/test/unit/update_manager_test.js
@@ +637,5 @@
> +        appUpdatable.name = 'Angry birds';
> +        appUpdatable.size = '423459';
> +
> +        UpdateManager.updatesQueue = [appUpdatable,
> +                                      systemUpdatable];

why can't you use addToUpdatesQueue ?

@@ +639,5 @@
> +
> +        UpdateManager.updatesQueue = [appUpdatable,
> +                                      systemUpdatable];
> +
> +        UpdateManager.containerClicked();

nitpicking here, but couldn't you try to use "click()" on the container DOM node ?

@@ +740,4 @@
>              var item = UpdateManager.downloadDialogList.children[1];
> +            var expectedMarkup = ['Angry birds',
> +                                  '<label>',
> +                                  '<input data-position="1" type="checkbox">',

I'm wondering if this is right.. because depending on what gecko does, attributes could be swapped for example.

unless this is well specified and well implemented in gecko.

And I don't think using DOM methods would be less readable actually.

Comment 9

[Etienne Segonzac (:etienne)]

(In reply to Julien Wajsberg [:julienw] [:everlong] from comment #8)
> I'd really put the name inside the label :
> 
> - immediate benefit: the user can tap the name of the application to check
> the box. Big utilisability win !
> - free future accessibility benefit (when we'll have a screen reader)
> 
> However we can open another bug if that's too much CSS work. (I'd rather not
> because I'm sure it would be done like in 1 year)

I gave it a fair shot but I'm afraid it can not be done without modifying the swiches.css building block, which is way outside the scope here.

> 
> @@ +180,5 @@
> > +        checkbox.type = 'checkbox';
> > +        checkbox.dataset.position = index;
> > +        checkbox.checked = true;
> > +
> > +        var span = document.createElement('span');
> 
> what is this |span| for ?

For the building block :)

> 
> ::: apps/system/style/update_manager/update_manager.css
> @@ +116,5 @@
> >    pointer-events: auto;
> >  }
> >  
> > +#updates-download-dialog h1 {
> > +  font-size: 2.2rem;
> 
> why do we need specifically this for this specific dialog ?

It doesn't have the same design, the design decision for the bigger font probably comes from the fact that the content is scrollable here.

Comment 10

[Etienne Segonzac (:etienne)]

Attachment: Patch v2

Comment 11

[Etienne Segonzac (:etienne)]

Attachment: Diff from patch v1

Comment 12

[Julien Wajsberg [:julienw]]

Comment on attachment 684700 [details] [diff] [review]
Patch v2

Review of attachment 684700 [details] [diff] [review]:
-----------------------------------------------------------------

::: apps/system/js/update_manager.js
@@ +173,5 @@
>        listItem.textContent = updatable.name;
>  
> +      // The user can choose not to update an app
> +      var checkContainer = document.createElement('label');
> +      if (updatable instanceof AppUpdatable) {

I still don't like that and you know it, but I won't hold back this patch for this.

@@ +177,5 @@
> +      if (updatable instanceof AppUpdatable) {
> +        var checkbox = document.createElement('input');
> +        checkbox.type = 'checkbox';
> +        checkbox.dataset.position = index;
> +        checkbox.checked = true;

You didn't see my global comment so I put it here instead:

I suspect something's missing here: if nothing is selected, "Download" should be disabled.
So we need to bind a onchange event on the form that would check if every checkbox is checked.

::: apps/system/test/unit/update_manager_test.js
@@ +732,4 @@
>              var item = UpdateManager.downloadDialogList.children[0];
> +            var expectedMarkup = ['systemUpdate',
> +                                  '<label class="required">required</label>',
> +                                  '<div>5.05 MB</div>'];

you don't use this markup, do you ?

Comment 13

[Etienne Segonzac (:etienne)]

Attachment: Patch proposal v3

Comment 14

[Julien Wajsberg [:julienw]]

Comment on attachment 685105 [details] [diff] [review]
Patch proposal v3

Review of attachment 685105 [details] [diff] [review]:
-----------------------------------------------------------------

we're getting there

::: apps/system/js/update_manager.js
@@ +17,4 @@
>    _downloadedBytes: 0,
>    _errorTimeout: null,
>    _wifiLock: null,
> +  _systemUpdateDisplayed: false,

I would choose "_hasSystemUpdate" instead.

@@ +65,5 @@
>  
>      this.container.onclick = this.containerClicked.bind(this);
>      this.laterButton.onclick = this.cancelPrompt.bind(this);
> +    this.downloadButton.onclick = this.startDownloads.bind(this);
> +    this.downloadDialogList.onclick = this.updateDownloadButton.bind(this);

did you try with "onchange" instead of "onclick" ?

@@ +176,5 @@
>        listItem.textContent = updatable.name;
>  
> +      // The user can choose not to update an app
> +      var checkContainer = document.createElement('label');
> +      if (updatable instanceof AppUpdatable) {

I'd be more comfortable if this test was reversed : the positive part of the |if| is for SystemUpdatable because this is the exception.

@@ +219,5 @@
> +    var dialog = this.downloadDialogList;
> +    var checkboxes = dialog.querySelectorAll('input[type="checkbox"]');
> +    for (var i = 0; i < checkboxes.length; i++) {
> +      if (checkboxes[i].checked) {
> +        disabled = false;

you could |break| here.

::: apps/system/locales/system.en-US.properties
@@ +94,4 @@
>  updatesAvailableMessage[few]={{ u }} updates available. <span>Tap to download.</span>
>  updatesAvailableMessage[many]={{ n }} updates available. <span>Tap to download.</span>
>  updatesAvailableMessage[other]={{ n }} updates available. <span>Tap to download.</span>
> +numberOfUpdates={[ plural(n) ]}

be careful as this applies with an offset, you should rebase

::: apps/system/style/update_manager/update_manager.css
@@ +156,2 @@
>    display: block;
> +  width: calc(100% - 12rem);

isn't it possible to use a simple padding here ?

div always use 100% anyway so it would be simpler. (if this works, I'm not so good in CSS ;) )

::: apps/system/test/unit/update_manager_test.js
@@ -632,3 @@
>          UpdateManager.init();
>  
> -        var evt = document.createEvent('MouseEvents');

I'd like to see if using "git format-patch --patience" would take this line in the diff (because it's still there after the patch). Just out of curiosity :)

@@ +699,5 @@
> +          UpdateManager.removeFromUpdatesQueue(systemUpdatable);
> +          UpdateManager.container.click();
> +
> +          evt = document.createEvent('MouseEvents');
> +          evt.initEvent('click', true, true);

do you use this ?

Comment 15

[Etienne Segonzac (:etienne)]

(In reply to Julien Wajsberg [:julienw] from comment #14)
> ::: apps/system/style/update_manager/update_manager.css
> @@ +156,2 @@
> >    display: block;
> > +  width: calc(100% - 12rem);
> 
> isn't it possible to use a simple padding here ?
> 
> div always use 100% anyway so it would be simpler. (if this works, I'm not
> so good in CSS ;) )

Nope it's pushing the "required" label away.

> 
> @@ +699,5 @@
> > +          UpdateManager.removeFromUpdatesQueue(systemUpdatable);
> > +          UpdateManager.container.click();
> > +
> > +          evt = document.createEvent('MouseEvents');
> > +          evt.initEvent('click', true, true);
> 
> do you use this ?

Yep I want the dialog to be re-rendered.


Patch incoming...

Comment 16

[Etienne Segonzac (:etienne)]

Attachment: Patch v4

Crossing fingers...

Comment 17

[Julien Wajsberg [:julienw]]

Comment on attachment 685299 [details] [diff] [review]
Patch v4

Review of attachment 685299 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

thanks for this nice UI :)

::: apps/system/test/unit/update_manager_test.js
@@ +699,5 @@
> +          UpdateManager.removeFromUpdatesQueue(systemUpdatable);
> +          UpdateManager.container.click();
> +
> +          evt = document.createEvent('MouseEvents');
> +          evt.initEvent('click', true, true);

you're not using |evt| in this suite.

Comment 18

[Etienne Segonzac (:etienne)]

Attachment: Final patch

Here is the final patch.

Comment 19

[Julien Wajsberg [:julienw]]

Comment on attachment 685543 [details] [diff] [review]
Final patch

r=me

Comment 20

[Staś Małolepszy :stas]

Comment on attachment 685543 [details] [diff] [review]
Final patch

r=me on the localization part, thanks!

Comment 21

[Etienne Segonzac (:etienne)]

https://github.com/mozilla-b2g/gaia/commit/6adc6c7b76186164359d8fa40b4d682cb4a6bc0c