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

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Sevaan Franks [:sevaan]]

Attachment: Example of About: Page Title

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.

Comment 1

[:Gijs (he/him)]

(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.

Comment 2

[Sevaan Franks [:sevaan]]

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.

Comment 3

[:Gijs (he/him)]

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.

Comment 4

[Sebastian Zartner [:sebo]]

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

Comment 5

[Neville]

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!

Comment 6

[Sebastian Zartner [:sebo]]

(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

Comment 7

[Michael Cooper [:mythmon]]

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

Comment 8

[Neville]

Attachment: Clean/shorten about: page titles

Comment 9

[Neville]

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!

Comment 10

[Sebastian Zartner [:sebo]]

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

Sebastian

Comment 11

[Michael Cooper [:mythmon]]

(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.

Comment 12

[Neville]

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

Comment 13

[Neville]

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 14

[Sebastian Zartner [:sebo]]

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.

Comment 15

[Neville]

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

Comment 16

[Neville]

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?

Comment 17

[Sebastian Zartner [:sebo]]

(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

Comment 18

[Sebastian Zartner [:sebo]]

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+.

Comment 19

[Neville]

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

Comment 20

[Neville]

Ah, the perils of a narrow editor window!

I have amended the patch. Sorry for the delay.

Thanks!

Comment 21

[:Gijs (he/him)]

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)

Comment 22

[Sebastian Zartner [:sebo]]

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

Comment 23

[:Gijs (he/him)]

(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.

Comment 24

[Sebastian Zartner [:sebo]]

(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

Comment 25

[Sebastian Zartner [:sebo]]

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 26

[Matthew N. [:MattN]]

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

The last patch also needs rebasing now.

Comment 27

[Preeti[:preeti]]

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!

Comment 28

[Yushan Lin]

Hi, I'm interested in working on this bug.

Comment 29

[Suriyaa Sundararuban [:suriyaa]]

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

Comment 30

[Arpit Bharti]

I am also interested in working on this issue.

Comment 31

[Arpit Bharti]

Attachment: Bug 1051846 - Added titles to about pages. r=sebo

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

Comment 32

[Arpit Bharti]

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.

Comment 33

[Arpit Bharti]

Attachment: Bug 1051846 - Add a <title> to about:checkerboard and about:memory, r=Gijs

Bug 1051846 - Added title for about:memory r=Gijs

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0649bb18323b
Add a <title> to about:checkerboard and about:memory, r=Gijs

Comment 35

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/0649bb18323b

Comment 36

[Arpit Bharti]

Attachment: Bug 1051846 - Changed title for about:performance r=flod

Changed from 'Task Manager' to 'Performance Analyzer'

Comment 37

[:Gijs (he/him)]

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?

Comment 38

[Sebastian Zartner [:sebo]]

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