Closed Bug 848772 Opened 7 years ago Closed 6 years ago

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

Categories

(Firefox for Android :: General, enhancement)

ARM
Android
enhancement
Not set

Tracking

()

VERIFIED FIXED
Firefox 27
Tracking Status
firefox27 --- verified

People

(Reporter: aaronmt, Assigned: nivivon)

References

Details

(Whiteboard: [mentor=margaret][lang=html])

Attachments

(3 files, 3 obsolete files)

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
Attached image make it so
Duplicate of this bug: 876049
Duplicate of this bug: 875977
Attached image HTC One
This looks silly on the HTC One
Duplicate of this bug: 908150
Examples from the dupe above

about:firefox    -->  About Firefox
about:feedback   -->  Firefox Feedback
about:downloads  -->  Downloads
about:addons     -->  Add-ons
about:apps       -->  Apps
Summary: Nit: Use hardcoded link titles for tabs with our in-content UI for about:privatebrowsing and about:home → Nit: Use hardcoded link titles for tabs with our in-content UI for about:privatebrowsing and about:home and others
Whiteboard: [mentor=margaret][lang=html]
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.
Flags: needinfo?(aaron.train)
Flags: needinfo?(aaron.train)
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
Attached patch title-additions.patch (obsolete) — Splinter Review
+ '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?
Attachment #794409 - Flags: review?(margaret.leibovic)
Home sounds great. Capitalized please.
Attachment #794409 - Flags: review?(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.
Attachment #794409 - Flags: review-
Aaron, are you still planning to get this in before the merge?
Not at the moment no, this is free to take for anyone with the above feedback recommendations.
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.
(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!
Assignee: nobody → nivivon
Attached patch title-additions.patch (obsolete) — Splinter Review
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!
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
(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.
Summary: Nit: Use hardcoded link titles for tabs with our in-content UI for about:privatebrowsing and about:home and others → Nit: Use hardcoded link titles for tabs tray with our in-content UI for about:privatebrowsing and about:home and others
Attachment #794409 - Attachment is obsolete: true
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 :)
Attachment #809285 - Flags: review?(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.
Attachment #809285 - Flags: review?(margaret.leibovic) → feedback+
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. :-)
Attachment #809599 - Flags: review?(margaret.leibovic)
Attachment #809599 - Flags: review?(margaret.leibovic)
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.
Attachment #809285 - Attachment is obsolete: true
Attachment #809599 - Attachment is obsolete: true
Attachment #809607 - Flags: review?(margaret.leibovic)
Comment on attachment 809607 [details] [diff] [review]
title-additions.patch

This looks great, thanks!
Attachment #809607 - Flags: review?(margaret.leibovic) → review+
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!
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/890fc46bcf53
Keywords: checkin-needed
Whiteboard: [mentor=margaret][lang=html] → [mentor=margaret][lang=html][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/890fc46bcf53
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=margaret][lang=html][fixed-in-fx-team] → [mentor=margaret][lang=html]
Target Milestone: --- → Firefox 27
\o/
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.