Closed Bug 1156252 Opened 9 years ago Closed 8 years ago

Confusing UI in "Cancel Download" dialog box

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Version:
40 Branch
Platform:
All
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox45 affected, firefox46 verified)

Status:
RESOLVED FIXED
Milestone:
Firefox 46
Tracking Flags:
Tracking Status
firefox45 --- affected
firefox46 --- verified

People

(Reporter: nyuszika7h, Assigned: hammoudmo, Mentored)

Details

(Whiteboard: [lang=java][good next bug])

Attachments

(2 files, 2 obsolete files)

Screenshot of the confusing dialog
205.34 KB, image/jpeg
Details
Update to previous patch with requested changes to the names of the strings.
2.31 KB, patch
mcomella
: review+
hammoudmo
: checkin+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:40.0) Gecko/20100101 Firefox/40.0
Build ID: 20150419030206

Steps to reproduce:

Start a download, then try to cancel it.


Actual results:

Cancel Download
==============

Do you want to cancel this download?

[Cancel] [OK]


Expected results:

It's pretty confusing, because "Cancel" cancels the *cancellation* of the download. It should be something else, like "Yes" and "No".
Do we want to change this dialog?
Flags: needinfo?(alam)
OS: Windows 8.1 → Android
Hardware: x86 → All
To my knowledge, this wasn't really custom of our end. I think it's standard and consistent across the rest of the platform.

I think this is a WONTFIX for now unless I'm misunderstanding something.
Flags: needinfo?(alam)
I do think it's confusing the way it is - I was about to report this myself when I found this bug.

Two ideas:

What if we change the strings to "Stop Download"/"Do you want to stop this download?" This way, we avoid the awkward collision with "cancel". This would be a quick, easy fix. "Stop" might be too close in meaning to "Pause", though.

What if clicking the download notification just sent the user to the download manager? This is how Chrome does it, so there's some precedent.
The problem then is that cancelling downloads is less discoverable - the user has to know how to long press. This is probably not good. (Maybe we should allow the user to cancel in-progress downloads with a single tap anyway.)
Flags: needinfo?(alam)
I'm hesitant to change away from system defaults for "cancel" and "ok" here. Since, throughout the system, the prompts are all the same and that releases the user from unnecessary cognitive loads. But, I also see a cost of change associated with going away from "cancel download" to "stop download" here as well.

I think we're over thinking the issue here and reading too much into it. This is still a WONTFIX for me. 

I'm going to CC Matej to see what he thinks.

Matej, there's concern that "cancel" (i.e. dismissing this action) is confused with "cancelling" the download. Do you see an issue here? It's worth noting that these dialog windows are used system wide and have "Cancel" and "OK" in the same spots, exactly the same as the screenshot.
Flags: needinfo?(alam) → needinfo?(matej)
I agree that this is a little confusing. The "cancel" button is essentially canceling your cancellation, but not canceling the download. I like the idea of leaving the buttons but changing the copy. Maybe instead of "Stop download" it could be "Exit download" or "Abort download." Either of those would be better than using "cancel" twice with different meanings.
Flags: needinfo?(matej)
"Abort download" seems to make the most sense to me since "Stop" has start/stop/pause connotations.
Hey Mohamed! Are you still interested in tackling this one?

---

Anthony, is the appropriate change to change the copy to "Abort download"?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(hammoudmo)
Flags: needinfo?(alam)
Mentor: michael.l.comella, liuche
Whiteboard: [lang=java][good next bug]
Mohamed, to address bug 1213299 comment 3, since we're not changing the prompt buttons but rather the main text, do you have enough information to continue?
Hi Michael! 

Yes. My understanding is to modify:

1. downloadCancelPromptTitle to be modified to 'Abort Download' from 'Cancel Download'
2. downloadCancelPromptMessage to be modified to 'Are you sure you wish to abort the download?'

(Personally I think that's much more clear as it's no longer conflicting with the 'cancel' button)

Both are properties in browser.properties so it should be an straightforward change. Though I'm still getting the hang of the environment (first time contributor :)) so I may poke you on IRC in a bit!

Thanks.
Flags: needinfo?(hammoudmo)
As the person who filed Bug 1213299, my $0.02: the proposed fix is inadequate and has a number of drawbacks:

1. Users do not read dialog box text [0].

2. Furthermore, even if they did, "abort" is a synonym for "cancel" and the user might still be confused.

3. Even if you disagree with (2), the notification dropdown uses the verb "cancel", so you will be making the language inconsistent between the dropdown and the dialog, and that will have to be rectified.

4. Even if you fix the issue from (3), the solution is only applicable to English.  When it comes time to i18n the dialog, you will have to revisit this issue and ensure that a sufficiently distinct verb with the appropriate meaning is available in the localized language.  I don't know if Firefox Android is i18n'd yet, but by making this decision now, you are setting your translator up for failure.

I still think the right solution is to label the buttons with the verbs that you're asking the user to choose between, even if you will not be able to use the standard dialog box.




[0] See the top answer here:
http://ux.stackexchange.com/questions/9946/should-i-use-yes-no-or-ok-cancel-on-my-message-box
Hi Keunwoo

Fair points! My concern is the current prompt.confirm does not support property values for button titles (it's a simple ok/cancel prompt). nsIPromptService has some other functions that may help us (https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIPromptService)

I'm thinking a fair compromise would be to keep everything the way it is, but to simply change the cancel message text to "Are you sure you wish to stop the download? Press OK to stop the download, and cancel to continue the download." (Even that sounds weird really!)

There's also the concern with, as you pointed out, with translation.
(In reply to Michael Comella (:mcomella) from comment #8)
> Hey Mohamed! Are you still interested in tackling this one?
> 
> ---
> 
> Anthony, is the appropriate change to change the copy to "Abort download"?

Yep! 

"Abort download" in the title, and "Do you want to abort this download?"
Flags: needinfo?(alam)
Hey,

I just uploaded my first patch.. ever. :o

No idea if I did it correctly but hopefully I did! Any feedback, etc, I'm all ears (metaphorically speaking anyway:))

Thanks,
Mohamed
Anthony, no thoughts on comment 11?
Flags: needinfo?(alam)
Assignee: nobody → hammoudmo
Comment on attachment 8672902 [details] [diff] [review]
Updated browser.properties to support changes to UI Text. Property values of downloadCancelPromptTitle and downloadCancelPromptMessage were updated

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

(In reply to Mohamed Hammoud from comment #15)
> I just uploaded my first patch.. ever. :o

Kudos! :)

> No idea if I did it correctly but hopefully I did! Any feedback, etc, I'm
> all ears (metaphorically speaking anyway:))

One thing I'd recommend is to flag a reviewer when you upload a patch (click on "Details" next to the patch, select "?" next to review, and enter a reviewer's email). I flagged a reviewer for you so this doesn't get lost! They should get back to you soon.
Attachment #8672902 - Flags: review?(liuche)
Comment on attachment 8672902 [details] [diff] [review]
Updated browser.properties to support changes to UI Text. Property values of downloadCancelPromptTitle and downloadCancelPromptMessage were updated

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

A few changes, but this is the right idea!

::: mobile/android/locales/en-US/chrome/browser.properties
@@ +39,5 @@
>  alertSearchEngineDuplicateToast='%S' is already one of your search engines
>  
>  alertPrintjobToast=Printing…
>  
> +downloadCancelPromptTitle=Abort Download

We localize all our strings, so to signal that the value of this string has changed, we also need to change this title.

Generally, if it's in the same place, we'll just increment the version of this, so turning it into
downloadCancelPromptTitle1=Abort Download

And also the same for the subsequent message.

Be sure to update where these are referenced!
Attachment #8672902 - Flags: review?(liuche) → feedback+
Hi Chenxia,

Thanks for your feedback! I will be doing the requested changes shortly.

I just wanted to get a clarification on what you mean on "Generally, if it's in the same place, we'll just increment the version of this, so turning it into
downloadCancelPromptTitle1=Abort Download"

Do we want to keep the old strings (I assume not). Is changing the names to something like 'downloadAbortPromptTitle' sufficient? I wasn't sure what you meant by if it is in the same place, we'd increment the version. :)

Thanks! Still new to this but starting to get the hang of it... baby steps.
Flags: needinfo?(liuche)
No problem!

> Do we want to keep the old strings (I assume not). Is changing the names to
> something like 'downloadAbortPromptTitle' sufficient? I wasn't sure what you
> meant by if it is in the same place, we'd increment the version. :)

The new string title has a "1" at the end of it - so this is what I mean by incrementing the title.
Flags: needinfo?(liuche)
Hi again,

Sorry for the delay on this. Got caught up with a bunch of midterms. :-)

I've updated the string titles to support the localization of strings (as well as in their respective functions).

Let me know if there's anything more that needs to be tweaked for this!

Thanks,
Mohamed
Attachment #8672902 - Attachment is obsolete: true
Attachment #8679043 - Flags: review?(liuche)
Attachment #8679043 - Flags: feedback?(liuche)
Comment on attachment 8679043 [details] [diff] [review]
Updated string names for localization support as requested.

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

No need to flag me for both review and feedback! I'd prefer increment, rather than changing the whole name of the string, sorry I wasn't more explicit about that in my earlier comment.

::: mobile/android/locales/en-US/chrome/browser.properties
@@ +39,5 @@
>  alertSearchEngineDuplicateToast='%S' is already one of your search engines
>  
>  alertPrintjobToast=Printing…
>  
> +downloadAbortPromptTitle=Abort Download

I'd prefer if you just incremented this string name, to
downloadCancelPromptTitle2 - this string is essentially the same, and that way it's a little easier to see that this string has been changed if someone is just glancing through trying to find a bug or something.
Attachment #8679043 - Flags: review?(liuche)
Attachment #8679043 - Flags: feedback?(liuche)
Attachment #8679043 - Flags: feedback+
Clearing my NI here since it seems like this is moving on track now :)
Flags: needinfo?(alam)
Any movement here Mohamed?
Flags: needinfo?(hammoudmo)
Hi everyone,

I extremely apologize for the delay. I had some issues getting some things sorted out and it turned out to be a lot more difficult than I thought. |mach artifact| nor |mach build| were working as intended. Huge shout out to nalexander for the help in getting everything sorted! 

I've made the changes as we agreed upon, namely the iteration of the names of the strings as well as their subsequent messages.

Let me know if there is anything that needs to be tweaked. 

Thank you!

Mohamed
Flags: needinfo?(hammoudmo)
Attachment #8702996 - Flags: review?(michael.l.comella)
Attachment #8702996 - Flags: review?(liuche)
Comment on attachment 8702996 [details] [diff] [review]
Update to previous patch with requested changes to the names of the strings.

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

I made a push to our try test servers (above).

Once the push goes green, you can add the "checkin-needed" keyword [1] to get your patch checked in. Note that all patches added via checkin-needed keyword need an associated green try run. Let me know if you need help reading the results.

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree

(In reply to Mohamed Hammoud from comment #25)
> I extremely apologize for the delay. I had some issues getting some things
> sorted out and it turned out to be a lot more difficult than I thought.
> |mach artifact| nor |mach build| were working as intended. Huge shout out to
> nalexander for the help in getting everything sorted! 

No worries – we understand! I apologize for the delay in review – the holiday season has been crazy. I hope you enjoyed yours! :)
Attachment #8702996 - Flags: review?(michael.l.comella)
Attachment #8702996 - Flags: review?(liuche)
Attachment #8702996 - Flags: review+
(In reply to Michael Comella (:mcomella) from comment #27)
> I made a push to our try test servers (above).

And by above, I mean comment 28.
Great! I'll keep an eye on the job. :-D
I've just reviewed the job and it seems all the builds have succeeded except that a couple tests did fail. Looking at the exceptions, they don't seem to be related to this bug. Are we in trouble?
(In reply to Mohamed Hammoud from comment #31)
> I've just reviewed the job and it seems all the builds have succeeded except
> that a couple tests did fail. Looking at the exceptions, they don't seem to
> be related to this bug. Are we in trouble?

Our test suites sometimes have intermittent issues that are unrelated to the push. Usually, you can click on the failing suite and compare the error output to the various intermittent issues we have filed but in this case, it appears these issues are unfiled. To be safe, I retriggered the test suite a few times so that should be done in a few hours but I agree that these problems look unrelated.

Since these failures look unrelated, I'm fine adding "checkin-needed" but for full process, it could be worthwhile to check on the results of the retriggers.
By the way, you can use the "Need more information from" field to give a user a persistent notification so they'll be more likely to see your comments or questions.
Comment on attachment 8702996 [details] [diff] [review]
Update to previous patch with requested changes to the names of the strings.

After reviewing the logs, the failures have come back but they do not seem to be related to this bug. As a result I've ticked a checkin-needed.
Attachment #8702996 - Flags: checkin+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3d0c2f2ad423
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Hey Mohamed! If you wanted a follow-up, perhaps bug 1056031 or for something more complex, you could pick up where bug 1155860 left off?
I'll definitely take a look!
Tested using:
Device: One A2001 (Android 5.1.1) 
Build: Firefox for Android 46.0a2 (2016-02-16)

Tapping the download notification, the following message will be displayed:
"Abort Download
Do you want to abort this download?
[Cancel] [OK]"
status-firefox45: --- → affected
status-firefox46: fixed → verified
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: