221934 - Display page title in DOM Inspector Window

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, enhancement)

Description

[dolphinling]

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

[XFox]

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.

Comment 2

[Alex Vincent [:WeirdAl]]

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.

Comment 3

[Michael Gall]

Attachment: Implements this functionality

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

Comment 4

[Michael Gall]

Hahah, this doesn't add the tite, it add's the address.

Comment 5

[Shawn Wilsher :sdwilsh]

I think you want viewer.subject.title or something along those lines.

Comment 6

[Michael Gall]

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

Comment 7

[Shawn Wilsher :sdwilsh]

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.

Comment 8

[Shawn Wilsher :sdwilsh]

Modifying summary - we should ideally put this in the window title (like most apps that work with documents)

Comment 9

[Michael Gall]

Attachment: Fixes this bug

Fixing the linewrap with this one

Comment 10

[Michael Gall]

Attachment: Fixes linewrapping inspector.xul

Comment 11

[Daniel Brooks [: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 12

[Daniel Brooks [:db48x]]

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

Ah, I was just testing the wrong build. r=db48x

Comment 13

[Daniel Brooks [:db48x]]

Actually, make one other change before you check this in. Use document.title instead of window.title.

Comment 14

[[e-mail]]

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.

Comment 15

[Shawn Wilsher :sdwilsh]

(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

[[e-mail]]

document.documentElement.getAttribute("title") is the original title.
document.title is the current title.

Comment 17

[Michael Gall]

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.

Comment 18

[Alex Faaborg [:faaborg] (Firefox UX)]

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.

Comment 19

[Michael Gall]

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

Comment 20

[[e-mail]]

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

[[e-mail]]

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.

Comment 22

[Michael Gall]

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.

Comment 23

[[e-mail]]

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

Comment 24

[Michael Gall]

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

Comment 25

[Shawn Wilsher :sdwilsh]

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.

Comment 26

[[e-mail]]

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

Comment 27

[Michael Gall]

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?

Comment 28

[Shawn Wilsher :sdwilsh]

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

Comment 29

[Michael Gall]

Attachment: Updated patch

Comment 30

[Shawn Wilsher :sdwilsh]

Comment on attachment 253841 [details] [diff] [review]
Updated patch

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

I think that's a typo :p

Comment 31

[Michael Gall]

Attachment: Hopefully final patch

Certainly was.

Comment 32

[Daniel Brooks [:db48x]]

Comment on attachment 253917 [details] [diff] [review]
Hopefully final patch

much better. r=db48x

Comment 33

[Nickolay_Ponomarev]

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.

Comment 34

[Robert Kaiser]

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

[Nickolay_Ponomarev]

Duh! I did compare my cvs diff with the original patch, but didn't notice the difference :( The tree is closed now though.

Comment 36

[:Gavin Sharp [email: [e-mail]]]

Fixed the typo:
mozilla/extensions/inspector/resources/content/inspector.js 	1.33