Display page title in DOM Inspector Window

RESOLVED FIXED

Status

Other Applications
DOM Inspector
--
enhancement
RESOLVED FIXED
14 years ago
8 years ago

People

(Reporter: dolphinling, Assigned: Michael Gall)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

14 years ago
In the bar (where it says "Browser") that seperates the actual DOM inspector
from the viewing box, the title of the page should be displayed.

Comment 1

13 years ago
I agree. Here's another "should" comment:
The DOM Inspector should automatically load the page you're at (the current tab)
rather than sit there waiting for you to paste the url. Does "view source" ask
you "What page?" It does not. I don't think the DOM Inspector should either.

Once this is solved, we may even have the convenience of shortcut keys and
buttons in some new toolbar extensions.
XFox:  CTRL+SHIFT+I from the page you're looking at.  It is your friend.

If you could generate a patch to implement this, it'd be appreciated.
Product: Core → Other Applications
(Assignee)

Comment 3

11 years ago
Created attachment 252741 [details] [diff] [review]
Implements this functionality

This doesn't add's the address (chrome or web address) to the title of the DOMI
(Assignee)

Comment 4

11 years ago
Hahah, this doesn't add the tite, it add's the address.
I think you want viewer.subject.title or something along those lines.
(Assignee)

Comment 6

11 years ago
Created attachment 253046 [details] [diff] [review]
This adds the title

I'm not entirely sure about adding the title, the address itself is a reasonably good identifier of the document being inspected.
Attachment #253046 - Flags: review?
Comment on attachment 253046 [details] [diff] [review]
This adds the title

>+          window.title = document.getElementById("inspector-bundle").getString("sidebar.title") + " - " + aEvent.subject.title;

Line wrapping is 80 characters, so fix that, then request review from db48x@yahoo, sr from neil@httl

I didn't actually test this, but it looks like it will work.
Attachment #253046 - Flags: review?
Modifying summary - we should ideally put this in the window title (like most apps that work with documents)
Assignee: dom-inspector → michael
Summary: Display page title in DOM Inspector "browser" bar → Display page title in DOM Inspector Window
(Assignee)

Comment 9

11 years ago
Created attachment 253054 [details] [diff] [review]
Fixes this bug

Fixing the linewrap with this one
Attachment #252741 - Attachment is obsolete: true
Attachment #253046 - Attachment is obsolete: true
Attachment #253054 - Flags: superreview?(neil)
Attachment #253054 - Flags: review?(db48x)
(Assignee)

Comment 10

11 years ago
Created attachment 253056 [details] [diff] [review]
Fixes linewrapping inspector.xul
Attachment #253054 - Attachment is obsolete: true
Attachment #253054 - Flags: superreview?(neil)
Attachment #253054 - Flags: review?(db48x)

Updated

11 years ago
Attachment #253056 - Flags: superreview?(neil)
Attachment #253056 - Flags: review?(db48x)
(Assignee)

Updated

11 years ago
Attachment #253056 - Flags: review?(db48x)
Comment on attachment 253056 [details] [diff] [review]
Fixes linewrapping inspector.xul

First off, are you sure this patch is needed? It appears that even without it the titlebar contains the title of the page being inspected.

The bug summary looks like it was referring to the divider between the content area and the tree view, not the window title bar.

>+          window.title = document.getElementById("inspector-bundle")
>+          	.getString("sidebar.title") + " - " + aEvent.subject.title;

fwiw, I prefer a slightly different wrapping style:

         window.title = document.getElementById("inspector-bundle")
                                .getString("sidebar.title") + " - " +
                        aEvent.subject.title;

Also, it appears that there's at least one tab character in that whitespace. Replace that with spaces.
Comment on attachment 253056 [details] [diff] [review]
Fixes linewrapping inspector.xul

Ah, I was just testing the wrong build. r=db48x
Attachment #253056 - Flags: review?(db48x)
Attachment #253056 - Flags: review+
Actually, make one other change before you check this in. Use document.title instead of window.title.

Comment 14

11 years ago
Comment on attachment 253056 [details] [diff] [review]
Fixes linewrapping inspector.xul

>+          window.title = document.getElementById("inspector-bundle")
>+          	.getString("sidebar.title") + " - " + aEvent.subject.title;
1. What db48x said. Doesn't anyone ever open the Error Console?
2. The page title normally precedes the application title e.g.
   About: - DOM Inspector
3. document.documentElement.getAttribute("title") is a better way to get
   the "DOM Inspector" title.
Attachment #253056 - Flags: superreview?(neil) → superreview-
(In reply to comment #14)
> 3. document.documentElement.getAttribute("title") is a better way to get
>    the "DOM Inspector" title.

If we are changing that for each document, wouldn't it be better to not do it that way and use the stringbundle though?

Comment 16

11 years ago
document.documentElement.getAttribute("title") is the original title.
document.title is the current title.
(Assignee)

Comment 17

11 years ago
I added the page title after the DOM Inspector title because when browsing through a list of programs, knowing it is the DOM Inspector is more important than which actual document it is. However, in a program like Firefox, it is more important to know what the page is, the fact it is Firefox is ancillary.
To be consistent on each platform, here is what you should do:

OS X: [Document Title]
Windows and Linux: "[Document Title] - [Window Title]"

The reason OS X is different is because the application title is consistently on the left side of the menu bar at the top of the screen.
(Assignee)

Comment 19

11 years ago
Created attachment 253129 [details] [diff] [review]
Fixes document.title, order of document title and takes into account platform

Hopefully this should sort out all the aforementioned issues, I'm still not entirely sure having the document around that way is the best way to go, but obviously standards are standards. Maybe DOMI should have it's own icon to differentiate it, but that's for another bug.
Attachment #253056 - Attachment is obsolete: true
Attachment #253129 - Flags: superreview?(neil)
Attachment #253129 - Flags: review?(db48x)

Comment 20

11 years ago
Comment on attachment 253129 [details] [diff] [review]
Fixes document.title, order of document title and takes into account platform

Whoops. I just happened to try this on a page without a title. In that case the window title should of course be just "DOM Inspector".

Comment 21

11 years ago
Comment on attachment 253129 [details] [diff] [review]
Fixes document.title, order of document title and takes into account platform

...and we also need to deal with the case of the subject changing to something that doesn't even know what a title is. Sorry for not spotting it before.
Attachment #253129 - Flags: superreview?(neil) → superreview-
(Assignee)

Comment 22

11 years ago
Should we change to nothing, or should we display the address? I'd be happy to show the address all the time because I don't think the title is necessarily as descriptive as what we use the DOMI for anyway.
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED

Comment 23

11 years ago
Sure, the address would be fine. (But comment #21 still needs consideration.)
(Assignee)

Comment 24

11 years ago
Created attachment 253270 [details] [diff] [review]
Falls back to location, then just set's it back to the default.

I'm not sure if you wanted me to always use the location, or use the title. This falls through to the location if there is no title, and should fail gracefully if there is not location either.
Attachment #253129 - Attachment is obsolete: true
Attachment #253270 - Flags: superreview?(neil)
Attachment #253129 - Flags: review?(db48x)
Comment on attachment 253270 [details] [diff] [review]
Falls back to location, then just set's it back to the default.

>+          var docTitle = aEvent.subject.title ? aEvent.subject.title :
>+                                                aEvent.subject.location;

You should do |var docTitle = aEvent.subject.title || aEvent.subject.location;| instead.
Attachment #253270 - Flags: review?(db48x)

Comment 26

11 years ago
Comment on attachment 253270 [details] [diff] [review]
Falls back to location, then just set's it back to the default.

>+          var docTitle = aEvent.subject.title ? aEvent.subject.title :
>+                                                aEvent.subject.location;
Ah, I see Shawn already mentioned this.

>+          if(docTitle) {
Nit: space between if and (
Also location isn't supposed to be null here, so docTitle won't be, will it?

>+              document.title = docTitle + " - " + 
>+              	 document.documentElement.getAttribute("title");
Nit: choice of indent is either
     a) line up document with docTitle (because it's a list of strings)
     b) 4-space default indent for continuation lines
Attachment #253270 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 27

11 years ago
Are we guaranteed that aEvent.subject will have a location? That's why I'm still checking if docTitle exists.

What do I then need to do to check this in?
(In reply to comment #27)
> Are we guaranteed that aEvent.subject will have a location? That's why I'm
> still checking if docTitle exists.

Then you should be testing aEvent.subject

> What do I then need to do to check this in?

Get a patch that has r and sr, then I'll fine someone to check it in.
(Assignee)

Comment 29

11 years ago
Created attachment 253841 [details] [diff] [review]
Updated patch
Attachment #253270 - Attachment is obsolete: true
Attachment #253270 - Flags: review?(db48x)
Comment on attachment 253841 [details] [diff] [review]
Updated patch

>+          var docTitle = aEvent.subject.title || aEvent.subject.title; 

I think that's a typo :p
(Assignee)

Comment 31

11 years ago
Created attachment 253917 [details] [diff] [review]
Hopefully final patch

Certainly was.
Attachment #253841 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Attachment #253917 - Flags: superreview?(neil)
Attachment #253917 - Flags: review?(db48x)

Updated

11 years ago
Attachment #253917 - Flags: superreview?(neil) → superreview+
Comment on attachment 253917 [details] [diff] [review]
Hopefully final patch

much better. r=db48x
Attachment #253917 - Flags: review?(db48x) → review+

Updated

11 years ago
Whiteboard: [checkin needed]

Comment 33

11 years ago
checked into trunk.
mozilla/extensions/inspector/resources/content/inspector.js 1.32

The patch didn't apply with a weird error from patch and it also had a tab character in it. I applied the patch manually and removed that tab character.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]

Comment 34

11 years ago
The patch is correct, the checkin is/wasn't though.
You added an additional } in line 192:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/extensions/inspector/resources/content/inspector.js&rev=1.32#192
...and that breaks inspector, of course.

Comment 35

11 years ago
Duh! I did compare my cvs diff with the original patch, but didn't notice the difference :( The tree is closed now though.
Fixed the typo:
mozilla/extensions/inspector/resources/content/inspector.js 	1.33
QA Contact: timeless → dom-inspector
Blocks: 400095
You need to log in before you can comment on or make changes to this bug.