893091 - Bold the download name and host in the download infobar

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: Downloads, defect)

Description

[Marina Samuel [:emtwo]]

May require a change/addition to browserBundle.formatStringFromName().

Comment 1

[Sam Foster [:sfoster] (he/him)]

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.

Comment 2

[Sam Foster [:sfoster] (he/him)]

Attachment: 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?

Comment 3

[Matt Brubeck (: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. :/

Comment 4

[Sam Foster [:sfoster] (he/him)]

Attachment: 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.

Comment 5

[Matt Brubeck (: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?

Comment 6

[Sam Foster [:sfoster] (he/him)]

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.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/f8ec5f403f64

Comment 8

[Kamil Jozwiak [:kjozwiak]]

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