1400154 - Preference UI cloud storage option flicker

Status: RESOLVED WONTFIX

(Firefox :: Settings UI, defect, P5)

Description

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

[Note]:
- See https://bugzilla.mozilla.org/show_bug.cgi?id=1391486#c11 and https://bugzilla.mozilla.org/show_bug.cgi?id=1391486#c12 for background

[Affected versions]:
- 56.0b11 build1 (20170911193316)
- 57.0a1 (2017-09-14)

[Affected platforms]:
- Windows 10 x64
- Ubuntu 16.04 x64
- Mac OS X 10.11.6

[Steps to reproduce]:
Prerequisite:
- Cloud storage provider client desktop app installed, such as Dropbox

1. Launch Firefox with a clean profile
2. Open about:config and create "cloud.services.api.enabled" boolean pref (set its value to "true") and "cloud.services.storage.key" string pref (set its value to a provider key such as "Dropbox")
3. Ensure you have a valid download path for the cloud storage downloads by manually creating the "Downloads" folder in the provider's folder e.g. ~/Dropbox/Downloads
4. Open about:preferences, switch between the 3 options under the "Downloads" section and inspect each corresponding radio button 

[Expected result]:
- The user is able to easily switch between each download option 
- Each corresponding radio button is fully functional

[Actual result]:
- When switching between the third ("Always ask you where to save files") and the second option ("Save files to <provider name>") from the "Downloads" section, the first option ("Save files to <system default downloads>") gets selected for a very short time instead of the second one (see the screencast https://goo.gl/EbFQA2).

[Regression range]:
- This issue is triggered by bug 1391486

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Marking wontfix for 58 since we are still doing a shield study (bug 1391486) to learn more about this.

Punam, can you work on fixing this? From a brief look at the code I think it might be related to your use of await.

Comment 2

[Punam Dahiya [:pdahiya]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #1)
> Marking wontfix for 58 since we are still doing a shield study (bug 1391486)
> to learn more about this.
> 
> Punam, can you work on fixing this? From a brief look at the code I think it
> might be related to your use of await.

Yes, assigned myself to look into it and submit fix Thanks!

Comment 3

[Punam Dahiya [:pdahiya]]

Attachment: Bug 1400154 - Preferences UI download option flicker

***

Review commit: https://reviewboard.mozilla.org/r/199274/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199274/

Comment 4

[Punam Dahiya [:pdahiya]]

Hi Jared

I have tried fixing the flicker issue by explicitly storing the radio option selected when switching away from alwaysAsk option and handling that in browser.download.useDownloadDir onsyncfrompreference handler. Please review

Link to try server

https://treeherder.mozilla.org/#/jobs?repo=try&revision=228b0cec4905f75f6ffaa464b7b0f202123f6f4c

Thanks!

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8928044 [details]
Bug 1400154 - Preferences UI download option flicker

https://reviewboard.mozilla.org/r/199274/#review207464

This seems very complex for something that shouldn't need to be. When the user clicks on the radio button, why are we changing the value again once useDownloadDir becomes true? It seems we may be doing more work than necessary.

Comment 6

[Punam Dahiya [:pdahiya]]

Comment on attachment 8928044 [details]
Bug 1400154 - Preferences UI download option flicker

https://reviewboard.mozilla.org/r/199274/#review208562

::: browser/components/preferences/in-content/main.js:2370
(Diff revision 1)
>  
> +      // If user has switched away from alwaysAsk radio option
> +      // setting browser.download.useDownloadDir to true, check for
> +      // _useDownloadDirOption to get id of radio option selected
> +      // and set folderListPref with respective value
> +      if (this._useDownloadDirOption) {

saveToCloud and saveTo radio options has browser.download.useDownloadDir preference set to true. When user switches back from alwaysAsk to useDownloadDir as true we need to check which option is selected and set the folderListPref value accordingly

With CloudStorage download option available, there are two ways folderList pref value can change

a) switching between SaveTo and SaveToCloud radio option. This is handled in handleSaveToCommand

b)switching from alwaysAsk radio option to either SaveTo or SaveToCloud, this is handled here inside onsyncfromprefernce handler.
https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/main.xul#660

Failing to explicitly handle this case here plus use of await with getProviderifInUse on every onsyncfrompreference handling, I believe is the reason we see flicker.

Comment 7

[Punam Dahiya [:pdahiya]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> Comment on attachment 8928044 [details]
> Bug 1400154 - Preferences UI download option flicker
> 
> https://reviewboard.mozilla.org/r/199274/#review207464
> 
> This seems very complex for something that shouldn't need to be. When the
> user clicks on the radio button, why are we changing the value again once
> useDownloadDir becomes true? It seems we may be doing more work than
> necessary.

Tried answering your query above, please let me know if it can be refactored simpler. Thanks

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

It was great to meet you in person Punam. To put our notes in this bug, Gijs and I met with Punam in Austin and discussed this bug. We would like to hold off on fixing this while we wait to get more data from the Shield study. This bug, while noticeable and "janky", does not affect the usability of the feature and requires the user to follow some steps that are not very common.

Since there is not a simple straight-forward fix for this, let's leave this bug on file and if we determine that we will move forward with this UI we can look at restructuring how it works so we will not need such a complex fix for this.

Comment 9

[Gregory Pappas [:gregp]]

Cloud storage integration was removed in bug 1751093