Closed
Bug 1051846
Opened 10 years ago
Closed 6 years ago
Add page title to about: pages that do not have a title
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
FIXED
People
(Reporter: sevaan, Assigned: arpitbharti73, Mentored)
References
(Blocks 4 open bugs)
Details
(Keywords: good-first-bug, Whiteboard: [lang=html])
Attachments
(4 files, 4 obsolete files)
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+
Comment 1•10 years ago
|
||
(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.
Reporter | ||
Comment 2•10 years ago
|
||
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.
Updated•10 years ago
|
Points: --- → 3
Updated•7 years ago
|
Comment 3•7 years ago
|
||
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
Comment 4•7 years ago
|
||
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!
Comment 6•7 years ago
|
||
(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)
Comment 7•7 years ago
|
||
> > 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!
Comment 10•7 years ago
|
||
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
Comment 11•7 years ago
|
||
(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•7 years ago
|
||
Comment 13•7 years ago
|
||
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•7 years ago
|
||
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-
Comment 15•7 years ago
|
||
Comment 16•7 years ago
|
||
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?
Updated•7 years ago
|
Attachment #8901620 -
Flags: review?(sebastianzartner)
Comment 17•7 years ago
|
||
(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
Updated•7 years ago
|
Attachment #8901620 -
Flags: review?(sebastianzartner) → review+
Comment 18•7 years ago
|
||
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•7 years ago
|
||
Comment 20•7 years ago
|
||
Ah, the perils of a narrow editor window! I have amended the patch. Sorry for the delay. Thanks!
Comment 21•7 years ago
|
||
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•7 years ago
|
||
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)
Comment 23•7 years ago
|
||
(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•7 years ago
|
||
(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•7 years ago
|
||
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•7 years ago
|
||
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
Updated•7 years ago
|
Attachment #8901035 -
Attachment is obsolete: true
Updated•7 years ago
|
Attachment #8901620 -
Attachment is obsolete: true
Comment 27•6 years ago
|
||
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!
Assignee | ||
Comment 31•6 years ago
|
||
Bug 1051846 - Added dtd file for corrosponding about pages. r=sebo
Assignee | ||
Comment 32•6 years ago
|
||
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.
Updated•6 years ago
|
Whiteboard: [lang=html][lang=dtd] → [lang=html]
Updated•6 years ago
|
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
Comment 34•6 years ago
|
||
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/0649bb18323b Add a <title> to about:checkerboard and about:memory, r=Gijs
Updated•6 years ago
|
Updated•6 years ago
|
Flags: needinfo?(nev)
Comment 35•6 years ago
|
||
bugherder |
Comment 37•6 years ago
|
||
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)
Comment 38•6 years ago
|
||
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: 6 years ago
Flags: needinfo?(sebastianzartner)
Keywords: leave-open
Resolution: --- → FIXED
Updated•6 years ago
|
Attachment #9052678 -
Attachment is obsolete: true
You need to log in
before you can comment on or make changes to this bug.
Description
•