848772 - Nit: Use hardcoded link titles for tabs tray with our in-content UI for about:privatebrowsing and about:home and others

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Description

[Aaron Train [:aaronmt]]

When the following tabs are opened they are displayed as:

* Firefox Start → displayed as  → "about:home" → would be nice to be displayed as "Firefox Start"

* Private Browsing → displayed as  → "about:privatebrowsing" → would be nice to be displayed as "Private Browsing" or "Private Tab" or similar


Apps, downloads, and Add-Ons, About Nightly currently use their title

Comment 1

[Ian Barlow (:ibarlow)]

Attachment: make it so

Comment 4

[Aaron Train [:aaronmt]]

Attachment: HTC One

This looks silly on the HTC One

Comment 6

[Aaron Train [:aaronmt]]

Examples from the dupe above

about:firefox    -->  About Firefox
about:feedback   -->  Firefox Feedback
about:downloads  -->  Downloads
about:addons     -->  Add-ons
about:apps       -->  Apps

Comment 7

[:Margaret Leibovic]

Fixing this will involve adding a <title> tag to the html files for these about: pages, and adding a string entity to the .dtd files they use for localization.

Here's an example of how we do it for about:feeback:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutFeedback.xhtml#19
http://mxr.mozilla.org/mozilla-central/source/mobile/android/locales/en-US/chrome/aboutFeedback.dtd#9

Note that we'll want to use &brandShortName; where we want "Firefox" to show up.

Comment 8

[Aaron Train [:aaronmt]]

about:home doesn't have an xhtml in use, so I think for about:home; a new entity and string (strings.xml.in) and some logic for setting the name before applying the same name for every tab in [1] would need to be done I think (e.g, R.strings.home_title or such)

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/TabsTray.java#261

Comment 9

[Aaron Train [:aaronmt]]

Attachment: title-additions.patch

+ 'about:home' -> '&brandShortName home'
+ 'about:privatebrowsing' re-uses 'Private Browsing'

Ian: Not sure if you want to substitute 'home' for 'start' and wether you want to capital either; thoughts?

Comment 10

[Ian Barlow (:ibarlow)]

Home sounds great. Capitalized please.

Comment 11

[:Margaret Leibovic]

Comment on attachment 794409 [details] [diff] [review]
title-additions.patch

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

It's confusing that we show a Java page on top of the actual about:home content, but it does have an xhtml file:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutHome.xhtml

We should still get progress events (and title change events) for that, so we should add a title tag there.

Comment 12

[Peter Retzer (:pretzer)]

Aaron, are you still planning to get this in before the merge?

Comment 13

[Aaron Train [:aaronmt]]

Not at the moment no, this is free to take for anyone with the above feedback recommendations.

Comment 14

[Niv Yahel]

I would like to work on this if no one else is. I'm currently working on Fennec for my Undergraduate Project for UCOSP (www.ucosp.ca), and tackling this would help me familiarize with the patching process.

I already have a build running on my phone.

Comment 15

[:Margaret Leibovic]

(In reply to Niv Yahel from comment #14)
> I would like to work on this if no one else is. I'm currently working on
> Fennec for my Undergraduate Project for UCOSP (www.ucosp.ca), and tackling
> this would help me familiarize with the patching process.
> 
> I already have a build running on my phone.

Thanks for offering to take this on. I'm assigning this bug to you.

I saw that you were already talking about this bug on IRC this morning, so I think you understand the approach. Feel free to ask more questions on IRC, or upload a patch for feedback!

Comment 16

[Niv Yahel]

Attachment: title-additions.patch

I added the a title to about:home. It now displays "&brandShortName; Home". I know the title has to be localized, so I added that as an entity in config.dtd because there was no aboutHome.dtd, and when I made one, I couldn't reference to it. I was wondering where the best place would be to put that entity.

Thanks!

Comment 17

[Wesley Johnston (:wesj)]

Do we want a title on about:home? I think we probably want to show "Enter a search or address" there, like desktop does on about:newtab

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Wesley Johnston (:wesj) from comment #17)
> Do we want a title on about:home? I think we probably want to show "Enter a
> search or address" there, like desktop does on about:newtab

I was confused to. This is for the Tabs Tray, not the URL Bar.

Comment 19

[:Margaret Leibovic]

Comment on attachment 809285 [details] [diff] [review]
title-additions.patch

When uploading a patch, you should use the review? flag to request review from a reviewer. For mentor bugs, it's a safe bet to choose the mentor, so I'll do that for you :)

Comment 20

[:Margaret Leibovic]

Comment on attachment 809285 [details] [diff] [review]
title-additions.patch

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

This is looking good! I just have a few comments for you to address before we land this. When you upload another patch with these changes, please flag me for review? and I'll give it an r+!

::: mobile/android/chrome/content/aboutHome.xhtml
@@ +5,5 @@
> +  %htmlDTD;
> +  <!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd" >
> +  %brandDTD;
> +  <!ENTITY % globalDTD SYSTEM "chrome://global/locale/global.dtd">
> +  %globalDTD;

I don't see you using anything from global.dtd, we don't need this entity.

::: mobile/android/locales/en-US/chrome/config.dtd
@@ +4,5 @@
>  
>  <!ENTITY search.placeholder2    "Search Settings">
>  <!ENTITY clear.altText          "Clear">
>  <!ENTITY newpref.label2         "Add a New Setting">
> +<!ENTITY abouthome.title        "&brandShortName; Home">

I don't think config.dtd is the right place for this. To be consistent with our other about: pages, I think we should make a new aboutHome.dtd file, and put the string in there.

Comment 21

[Niv Yahel]

Attachment: Created an aboutHome.dtd, and removed the global.dtd reference.

I realized that a new aboutHome.dtd file would've been a better solution earlier. However, the reference to the file didn't work earlier. After reading up on Chrome, I realized that I had to edit the manifest file and was able to fix this. :-)

Comment 22

[Niv Yahel]

Attachment: title-additions.patch

I realized that a new aboutHome.dtd file would've been a better solution earlier. However, the reference to the file didn't work earlier. After reading up on Chrome, I realized that I had to edit the manifest file and was able to fix this. :-)

EDIT: I didn't know how to remove the previous patch (I'm guessing it's simply not possible), but I accidentally uploaded a file with no changes. I'm still getting used to Mercurial, haha.

Comment 23

[:Margaret Leibovic]

Comment on attachment 809607 [details] [diff] [review]
title-additions.patch

This looks great, thanks!

Comment 24

[:Margaret Leibovic]

I'm adding the checkin-needed keyword to the bug, which means somebody with commit access will come along and land this for you. Nice work!

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/890fc46bcf53

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/890fc46bcf53

Comment 27

[Aaron Train [:aaronmt]]

\o/