Bold the download name and host in the download infobar

VERIFIED FIXED in Firefox 26

Status

Firefox for Metro
Downloads
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: emtwo, Assigned: sfoster)

Tracking

unspecified
Firefox 26
x86_64
Windows 8
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [preview])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
May require a change/addition to browserBundle.formatStringFromName().
(Reporter)

Updated

5 years ago
Blocks: 893066
Whiteboard: [preview]

Updated

5 years ago
Blocks: 898477
No longer blocks: 893066
(Assignee)

Comment 1

5 years ago
The notification binding that gets used currently when we show this download infobar only supports a text string via its label attribute, so any markup we pass in there is rendered as-is as plain text, brackets and all. The xul:description element that represents the infobar text is happy to take elements as children, we'll just need another way to put it there.
Assignee: nobody → sfoster
(Assignee)

Comment 2

5 years ago
Created attachment 792541 [details] [diff] [review]
Format the save-or-run download notification bar text

I figured out what I was doing wrong and it turns out the new notification binding might not be necessary. This patch passes in an empty-string as the label attribute and immediately replaces the message content with an HTML <span> with our formatted message. 
Unfortunately, the notification's label property only checks the label attribute, not the contents of the messageText element - which is only populated by side-effect at creation. I don't know if there are implications to this we need to be concerned about. 
To do the formatting at all, somehow we have to get some additional markup in there. But I'm not sure if using innerHTML to set the <span>'s content has any bad implications in this context?
Attachment #792541 - Flags: review?(mbrubeck)
Comment on attachment 792541 [details] [diff] [review]
Format the save-or-run download notification bar text

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

This is fine; it's simpler than extending the notification binding and it looks like the .label property has no essential function.  r=mbrubeck with one change:

::: browser/metro/components/HelperAppDialog.js
@@ +105,4 @@
>      let msg = browserBundle.GetStringFromName("alertDownloadSave")
> +      .replace("#1", '<span class="download-filename-text">'+aLauncher.suggestedFileName+'</span>')
> +      .replace("#2", '<span class="download-size-text">'+downloadSize+'</span>')
> +      .replace("#3", '<span class="download-host-text">'+aLauncher.source.host+'</span>');

Perhaps I'm paranoid, but I think we should probably escape these in case there are special characters in, say, the suggested file name.  I don't know if we have an html escaping function handy or if we'd need to write one. :/
Attachment #792541 - Flags: review?(mbrubeck) → review+

Updated

5 years ago
See Also: → bug 907211
(Assignee)

Comment 4

5 years ago
Created attachment 794701 [details] [diff] [review]
New populateFragmentFromString+tests, format the save-or-run download notification bar text

ContentUtil.populateFragmentFromString uses textNodes to properly escape any markup or other naughtiness in the filename or localized string, and wraps the bits we want to format in a span+class. Take a look over the unit test to see if that looks sane and gets the coverage we need. 
Builds on the patch on bug 848137 which breaks out ContentUtil module from Util.
Attachment #792541 - Attachment is obsolete: true
Attachment #794701 - Flags: review?(mbrubeck)
Comment on attachment 794701 [details] [diff] [review]
New populateFragmentFromString+tests, format the save-or-run download notification bar text

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

::: browser/metro/modules/ContentUtil.jsm
@@ +16,5 @@
> +	  let match;
> +	  // walk over the string, building textNode/spans as necessary
> +	  // with the replacement content
> +	  // note that #1,#2 etc. may not appear in numerical order in the string
> +	  while((match = re.exec(str))) {

style nit: space after keywords ("while", "if")

A blank lines or two to break this up into logical groupings would also be welcome, just to make it less intimidating to timid readers like me. :)

::: browser/metro/theme/browser.css
@@ +1015,5 @@
> +  font-weight: bold;
> +}
> +.download-size-text {
> +  font-weight: normal;
> +}

Is this last rule really needed?
Attachment #794701 - Flags: review?(mbrubeck) → review+
(Assignee)

Comment 6

5 years ago
On fx-team: https://hg.mozilla.org/integration/fx-team/rev/f8ec5f403f64

Fixed up the style and formatting nits. Also removed that download-size-text rule. It was there as a placeholder to make it clearer that while we don't give file-size any particular style right now, we could. But, its easily added back if/when thats a requirement.
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/f8ec5f403f64
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26

Updated

4 years ago
Depends on: 961177
Went through the following verification processes using the following builds:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-02-25-03-02-01-mozilla-central/
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-02-25-00-40-03-mozilla-aurora/
- http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/28.0b6/win32/en-US/

- Launched several downloads from different websites and ensured that the files name and host are highlighted bold
- Went through a bunch of different file formats (exe, iso, zip, 7zip)
- Ensured that the rest of the string under the download info bar is correctly formatted without any visible issues
- Went through all of the above test cases in different variations of snapped view
- Used both the X1 Carbon and the Surface Pro 2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.