Add page title to about: pages that do not have a title

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
5 years ago
5 months ago

People

(Reporter: sevaan, Assigned: arpit73, Mentored)

Tracking

(Blocks 5 bugs, {good-first-bug})

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=html])

Attachments

(4 attachments, 4 obsolete attachments)

The following about: pages do not have page titles. Suggested titles are included:

about:				» About Firefox* 
about:buildconfig 		» Build Config
about:config 			» Configuration
about:license 			» Licenses
about:memory 			» Memory
about:plugins 			» Plugins
about:privatebrowsing 		» Start Private Browsing
about:rights 			» About Your Rights
about:sync-log			» Sync Log
about:webrtc			» WebRTC

* or Aurora, or Nightly.
Flags: firefox-backlog+
(In reply to Sevaan Franks [:sevaan] from comment #0)
> Created attachment 8470861 [details]
> Example of About: Page Title
> 
> The following about: pages do not have page titles.

Some do:

> about:plugins 			» Plugins

This has the title "About Plugins"

> about:privatebrowsing 		» Start Private Browsing

This has the title "Would you like to start private browsing" in a normal window, and "New Tab" in private browsing windows (where I presume we don't want it to say "Start Private Browsing"?).

> about:webrtc			» WebRTC

This has the title "WebRTC Internals"


Also,

> about:sync-log			» Sync Log

This is a file listing. I don't know that we can really give it a title, or if calling it "Sync Log" when it clearly isn't the sync log, but a directory listing containing sync logs, is useful.
Sorry, yes. Those page titles need updating too. Should I file a separate bug for those?

We'd like to remove some of the redundant "abouts" in the titles, hence the change to plugins.

"Start Private Browing" fits better in a tab than the entire question "Would you like to start private browsing?"

"WebRTC Internals" I am okay with keeping...but just thought WebRTC was pithier.

If we can't change the title for Sync Log, that's okay...but "Sync Logs" (the plural) does work for the page.
Points: --- → 3
Blocks: 1369800
Depends on: 1372005
Keywords: good-first-bug
Hardware: x86 → All
Whiteboard: [lang-html]
Please don't set the good first bug keyword unless there is also a mentor for the bug. Can you mentor this bug? If so, please add yourself in the mentor field and provide a comment indicating how the bug should be fixed.
Flags: needinfo?(sebastianzartner)
Keywords: good-first-bug
Thank you for the info, Gijs! I never mentored a bug so far, though I know the places which need to be touched. So, I'll try it.

Anyone interested in this bug should start looking for the files behind the about: pages. One that already has a proper title is the troubleshooting information page:

https://dxr.mozilla.org/mozilla-central/rev/7dddbd85047c6dc73ddbe1e423cd643a217845b3/toolkit/content/aboutSupport.xhtml#17

The title for it is stored in a DTD file here:

https://dxr.mozilla.org/mozilla-central/rev/7dddbd85047c6dc73ddbe1e423cd643a217845b3/toolkit/locales/en-US/chrome/global/aboutSupport.dtd#5

Other pages are missing this information, e.g. about:memory, or about:performance, and instead show their URI as title:

https://dxr.mozilla.org/mozilla-central/rev/7dddbd85047c6dc73ddbe1e423cd643a217845b3/toolkit/components/aboutmemory/content/aboutMemory.xhtml
https://dxr.mozilla.org/mozilla-central/rev/7dddbd85047c6dc73ddbe1e423cd643a217845b3/toolkit/components/aboutperformance/content/aboutPerformance.xhtml

So, to add a title to those pages, you first have to add an entity to the related DTD file (or create a new file if it doesn't exist yet) and then reference it within the <title> element of the about:* page.

An updated list of pages not having a proper title plus title suggestions are:

about:			» About Firefox* 
about:buildconfig	» Build Configuration
about:checkerboard	» Checkerboard Analyzer
about:config		» Configuration
about:memory		» Memory Analyzer
about:performance	» Performance Analyzer
about:studies		» Studies
about:sync-log		» Profile Synchronization Log
about:url-classifier	» URL Classifier

* or Beta, or Firefox Developer Edition, or Nightly. To get the right name use the &brandShortName; entity in the title string. See https://dxr.mozilla.org/mozilla-central/rev/7dddbd85047c6dc73ddbe1e423cd643a217845b3/browser/locales/en-US/chrome/browser/aboutHealthReport.dtd#6 as an example.

There are some pages, which could profit from renaming their title. For example, some of them have a redundant "About" like about:plugins.

Sebastian
Mentor: sebastianzartner
Flags: needinfo?(sebastianzartner)
Keywords: good-first-bug
Whiteboard: [lang-html] → [lang=html][lang=dtd]
Hi there, thought I might try this one out as my first Mozilla bug. Based on comments above I have made some changes:

about:			» About Firefox* 
Adjusted dtd and html, using &brandShortName;

about:buildconfig	» Build Configuration
Uses a different template system. Manually set as it didn't seem to get along with xml/dtd at all.

about:checkerboard	» Checkerboard Analyzer
Adjusted (added dtd, registered in jar manifest).

about:config		» Configuration
Adjusted dtd and html.

about:memory		» Memory Analyzer
Adjusted (added dtd). Page title is adjusted in aboutMemory.js when certain options are chosen. Updated aboutMemory.js to take original title from dtd.

about:performance	» Performance Analyzer
Adjusted (added dtd, registered in jar manifest).

about:studies		» Studies
Is packed in the extensions section of browser (mozilla-central/browser/extensions/shield-recipe-client/content/about-studies). Have left as-is, seems like it should be handled independently in a shield-recipe-client based bug perhaps?

about:sync-log		» Profile Synchronization Log
I think the HTML for this is generated from: https://dxr.mozilla.org/mozilla-central/source/netwerk/streamconv/converters/nsIndexedToHTML.cpp so I guess originally from C++ code. I haven't adjusted this.
 
about:url-classifier	» URL Classifier
Adjusted, added title property to dtd.

If this seems ok, I'll make a patch with these changes, otherwise I'm all ears for feedback. Thanks!
(In reply to Neville from comment #5)
> about:			» About Firefox* 
> Adjusted dtd and html, using &brandShortName;

Good.

> about:buildconfig	» Build Configuration
> Uses a different template system. Manually set as it didn't seem to get
> along with xml/dtd at all.

I didn't try that myself yet, but if it's not possible to use a DTD in that case, I guess it's better than nothing to manually set the title.
Just for reference, the file is available at https://dxr.mozilla.org/mozilla-central/rev/a9d372645a32b8d23d44244f351639af9d73b96a/toolkit/content/buildconfig.html.

> about:checkerboard	» Checkerboard Analyzer
> Adjusted (added dtd, registered in jar manifest).

Good.

> about:config		» Configuration
> Adjusted dtd and html.

Good.

> about:memory		» Memory Analyzer
> Adjusted (added dtd). Page title is adjusted in aboutMemory.js when certain
> options are chosen. Updated aboutMemory.js to take original title from dtd.

Great! You're right, e.g. the title is changed when you load a previously saved memory dump.

> about:performance	» Performance Analyzer
> Adjusted (added dtd, registered in jar manifest).

Good.

> about:studies		» Studies
> Is packed in the extensions section of browser
> (mozilla-central/browser/extensions/shield-recipe-client/content/about-
> studies). Have left as-is, seems like it should be handled independently in
> a shield-recipe-client based bug perhaps?

I'd say that this should also be covered in this bug, but let's ask the person responsible for that code. Mike, do you think this should be handled here or in a separate bug?

> about:sync-log		» Profile Synchronization Log
> I think the HTML for this is generated from:
> https://dxr.mozilla.org/mozilla-central/source/netwerk/streamconv/converters/
> nsIndexedToHTML.cpp so I guess originally from C++ code. I haven't adjusted
> this.

Right. I assumed that's hard to do, as it uses the same code as when you display any folder of the file system. It may be possible to adjust it to accept customized titles, though that's probably not worth the effort. So let's keep it as is.

> about:url-classifier	» URL Classifier
> Adjusted, added title property to dtd.

Good.

> If this seems ok, I'll make a patch with these changes, otherwise I'm all
> ears for feedback. Thanks!

Yes, please do so!

Sebastian
Flags: needinfo?(mcooper)
> > about:studies		» Studies
> > Is packed in the extensions section of browser
> > (mozilla-central/browser/extensions/shield-recipe-client/content/about-
> > studies). Have left as-is, seems like it should be handled independently in
> > a shield-recipe-client based bug perhaps?
> 
> I'd say that this should also be covered in this bug, but let's ask the
> person responsible for that code. Mike, do you think this should be handled
> here or in a separate bug?

Keeping this in one bug is fine.

We'll need to also patch the upstream source at https://github.com/mozilla/normandy/tree/master/recipe-client-addon. Neville, if you're comfortable doing so, please file a pull request against that Github repo. If not, I can port the patch from this bug to the other repo.
Flags: needinfo?(mcooper)
Thanks for the feedback folks! 

Mike, I'm happy to also patch the upstream at github, would be good to have some clarification on how you'd prefer it fixed. My (incomplete/incorrect?) understanding is the advantage to using .dtd files as part of the locale section is that if/when it's localized elsewhere, native language equivalents of these files will be used for naming instead. The recipe client addon doesn't have localization stuff yet, would you like me to add it or is it ok for it to depend on the toolkit's locale stuff?

I attempted to use hg bzexport to export a patch but it said it failed as I'm not assigned to this bug. I didn't realise it would try to submit the patch directly! I seems to have partly worked, hence the attachment. I was just trying it out sorry. I also notice that it includes all the additional files that have nothing to do with this patch but were included after pulling down the latest changes - should I not have done a pull? 

Cheers!
Your patch obviously includes unrelated changes. Can you please rebase it, so it only includes the changes relevant to this bug?

Sebastian
Assignee: nobody → nev
Status: NEW → ASSIGNED
(In reply to Neville from comment #9)
> Mike, I'm happy to also patch the upstream at github, would be good to have
> some clarification on how you'd prefer it fixed. My (incomplete/incorrect?)
> understanding is the advantage to using .dtd files as part of the locale
> section is that if/when it's localized elsewhere, native language
> equivalents of these files will be used for naming instead. The recipe
> client addon doesn't have localization stuff yet, would you like me to add
> it or is it ok for it to depend on the toolkit's locale stuff?

For now the recipe client addon, and by extension about:studies, is not localized. We prioritized getting basic functionality done for English audiences before localization, since the early studies were targeted at only English speaking audiences. In the future we plan to revisit this to add support for localization.

It would be fine to make this change English-only for the recipe client addon. I would also be quite happy if you wanted to introduce localization to about:studies, but I assume that is too much work for this bug. I don' thave any opinions about particular methods you might use.
Yep sorry about that. I thought I'd get a chance to review the patch before upload, and also thought the patch might just be what was in hg outgoing. This one contains just the files I've altered.

For about:studies I have simply adjusted the title in the html for now. I'll look further into localization methods before making any more serious changes here.

Contrary to my earlier comment, I have now updated about:buildconfig to use a .dtd from locales also.
Comment on attachment 8901035 [details] [diff] [review]
Add page title to about: pages that do not have a title

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

Looks very good already. Only the buildconfig.xhtml needs to be fixed and the reference to the old .html be replaced by the new .xhtml one at https://dxr.mozilla.org/mozilla-central/rev/892c8916ba32b7733e06bfbfdd4083ffae3ca028/docshell/base/nsAboutRedirector.cpp#43.

Setting this to r-, because without the changes above this can't be shipped.

Sebastian

::: toolkit/content/buildconfig.xhtml
@@ +13,5 @@
> +#filter substitution
> +#include @TOPOBJDIR@/source-repo.h
> +<html xmlns="http://www.w3.org/1999/xhtml">
> +<head>
> +  <meta charset="UTF-8">

To be a valid XML, this tag and the other <meta> and <link> tags need to be closed.
Attachment #8901035 - Flags: review-
Thanks Sebastian, 

I've updated the patch with your changes. Once I renamed buildconfig I should have searched the whole project for references, is this how you picked it up or are there particular tests that I should have been running?
Attachment #8901620 - Flags: review?(sebastianzartner)
(In reply to Neville from comment #16)
> Thanks Sebastian, 
> 
> I've updated the patch with your changes. Once I renamed buildconfig I
> should have searched the whole project for references, is this how you
> picked it up or are there particular tests that I should have been running?

First, I've simply accessed all the pages in the browser to test them. There I saw that about:buildconfig failed to load.
Then I searched for what was causing the errors and found that piece of code. Once I changed it and compiled again I saw that it threw XML errors and fixed them.

I still need to do the review, which may take a bit as I'm quite busy the next days. I set myself as reviewer again, so I don't forget about it.

Sebastian
Attachment #8901620 - Flags: review?(sebastianzartner) → review+
Comment on attachment 8901620 [details] [diff] [review]
Add page title to about: pages that do not have a title

>diff --git a/toolkit/locales/jar.mn b/toolkit/locales/jar.mn
>--- a/toolkit/locales/jar.mn
>+++ b/toolkit/locales/jar.mn
>@@ -8,6 +8,10 @@
> % locale global @AB_CD@ %locale/@AB_CD@/global/
>   locale/@AB_CD@/global/about.dtd                       (%chrome/global/about.dtd)
>   locale/@AB_CD@/global/aboutAbout.dtd                  (%chrome/global/aboutAbout.dtd)
>+  locale/@AB_CD@/global/aboutBuildConfig.dtd                  (%chrome/global/aboutBuildConfig.dtd)
>+  locale/@AB_CD@/global/aboutCheckerboard.dtd                       (%chrome/global/aboutCheckerboard.dtd)
>+  locale/@AB_CD@/global/aboutMemory.dtd                       (%chrome/global/aboutMemory.dtd)
>+  locale/@AB_CD@/global/aboutPerformance.dtd                       (%chrome/global/aboutPerformance.dtd)
>   locale/@AB_CD@/global/aboutReader.properties          (%chrome/global/aboutReader.properties)
>   locale/@AB_CD@/global/aboutRights.dtd                 (%chrome/global/aboutRights.dtd)
>   locale/@AB_CD@/global/aboutNetworking.dtd             (%chrome/global/aboutNetworking.dtd)

Nit: You should align the indentation with the one of the other entries here. With that it's r+.
Ah, the perils of a narrow editor window!

I have amended the patch. Sorry for the delay.

Thanks!
Comment on attachment 8904415 [details] [diff] [review]
Add page title to about: pages that do not have a title

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

Can you use "hg mv --after" (see "hg help mv" for details) to ensure that mercurial knows you moved buildconfig.html to buildconfig.xhtml, and we preserve per-line history where the lines don't change? :-)

Also, when you next put up a patch, can you add a review flag (select "?" in the dropdown and put sebo or my email in the text field)
Also, I've just realized from reading through https://hg.mozilla.org/mozilla-central/file/d225dbb6b59f/browser/components/about/AboutRedirector.cpp (which are not listed in about:about) that there are two more about: pages missing a title:

about:blocked
about:tabcrashed

The first should have "Deceptive Site" as title and the latter "Tab Crashed".
Is it possible you add a proper title for them, too?

Sebastian
Flags: needinfo?(nev)
(In reply to Sebastian Zartner [:sebo] from comment #22)
> Also, I've just realized from reading through
> https://hg.mozilla.org/mozilla-central/file/d225dbb6b59f/browser/components/
> about/AboutRedirector.cpp (which are not listed in about:about) that there
> are two more about: pages missing a title:
> 
> about:blocked

This does get a title, see:

https://dxr.mozilla.org/mozilla-central/source/browser/base/content/blockedSite.xhtml#141-142

We don't care about the case where people manually load this, ditto for about:neterror and about:certerror.

> about:tabcrashed

Same: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/aboutTabCrashed.js#61


So please don't touch these. Adding a hardcoded title will just cause additional flashing in the tabstrip / window title because we'll update the title twice.
(In reply to :Gijs from comment #23)
> (In reply to Sebastian Zartner [:sebo] from comment #22)
> > Also, I've just realized from reading through
> > https://hg.mozilla.org/mozilla-central/file/d225dbb6b59f/browser/components/
> > about/AboutRedirector.cpp (which are not listed in about:about) that there
> > are two more about: pages missing a title:
> > 
> > about:blocked
> 
> This does get a title
> 
> ...
> 
> > about:tabcrashed
> 
> Same

Ah, right, missed that. Sorry!

Sebastian
Neville, are you still interested in finishing up your patch? It's looks like you were very close. The last missing bit was mentioned by Gijs in comment 21.

Sebastian
Comment on attachment 8900587 [details] [diff] [review]
Clean/shorten about: page titles

The last patch also needs rebasing now.
Attachment #8900587 - Attachment is obsolete: true
Hello everyone! I would like to work on this bug in case no one is working on this. Can someone please guide me on how to proceed? Thanks in advance!
Hi, I'm interested in working on this bug.

Hello, I'm also interested in working on this bug.

I am also interested in working on this issue.

Bug 1051846 - Added dtd file for corrosponding about pages. r=sebo

Current changes.

about:checkerboard  ->  Checkerboard Analyzer
about:memory        ->  Memory Analyzer
about:buildconfig   ->  about:buildconfig
about:studies       ->  Studies
about:performance   ->  Performance Analyzer

about:buildconfig doesn't seem to be working.

Whiteboard: [lang=html][lang=dtd] → [lang=html]
Attachment #9051642 - Attachment description: Bug 1051846 - Added title for about:checkerboard r=Gijs → Bug 1051846 - Add a <title> to about:checkerboard and about:memory, r=Gijs
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0649bb18323b
Add a <title> to about:checkerboard and about:memory, r=Gijs
Assignee: nev → arpitbharti73
Points: 3 → ---
Keywords: leave-open
Flags: needinfo?(nev)

Changed from 'Task Manager' to 'Performance Analyzer'

I'm concerned about the state of this bug. It's a loose motley collection of various pages, and it's not clear that it's keeping track with what's already in the tree in terms of which pages to update (e.g. we've just had a patch to update about:performance which already has a page title).

Can I suggest we close this and you file follow-ups for specific pages that you still think need updates?

Flags: needinfo?(sebastianzartner)

I agree with you. This bug was obviously too broad to be target-aimed. I close it as FIXED, as this has got patches and titles were added. I'll file follow-ups as needed.

Sebastian

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Flags: needinfo?(sebastianzartner)
Keywords: leave-open
Resolution: --- → FIXED
Blocks: 1538422
Blocks: 1538423
Blocks: 1538424
Blocks: 1538428
Blocks: 1538429
Blocks: 1538431
Attachment #9052678 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.