I believe the wording of this View menu item should be more indicative, intuitive, meaningful, instructive of what this option does. Changing "View/Browser" for "View/Document in Browser Pane" will help users figure out this API better without documentation. Also, a lot of related options in the menu should be grayed, dimmed if "View/Document in Browser Pane" is unchecked. - "View/Blink Selected Element" should be dimmed, grayed, disabled if "View/Document in Browser Pane" is unchecked. - "Search/Select Element By Click" should be dimmed, grayed, disabled if "View/Document in Browser Pane" is unchecked. - The simili-pseudo button with the tooltip text "Find a node to inspect by clicking on it" should NOT be displayed at all if "View/Document in Browser Pane" is unchecked.

I agree the change to the menu item would be feasible, and very easy to do. Why should we disable blink? You might notice the document which Inspector is looking at will still blink in the right place (approximately) when you select an element.

Mass re-assigning bugs to dom.inspector@extensions.bugs

If the browser pane (after unchecking "View/Browser") is not there, not visible, not rendered, what good is there to allow the user to check/uncheck the "View/Blink Selected Element"? Making the "Blink Selected Element" menuitem dimmed, grayed out can only help the user to better understand intuitively how to use such menu items. When I first use DOM inspector, I was checking and unchecking such menuitems and noticing no results. I felt confused by the interface. The blinking only happens, appears in the browser (lower, bottom) pane. Remove that pane and you should also consequently disable related menuitems. Every menuitem representing a functionality which can only work with the presence of the browser pane should get dimmed, disabled, grayed out when the browser (lower, bottom) pane is removed, is not rendered. I don't know another way to explain this. Adding polish keyword, severity: minor

In Firefox 8 doesn't appear any View->Browser menu. Changing to pre-4.0 versions?

This bug is about the View menu in the DOM Inspector window.

Is this the code for the DOM Inspector add-on? Then, the change is easy.

Is this the code for the DOM Inspector add-on? http://mxr.mozilla.org/comm-central/source/mozilla/extensions/inspector/resources/locale/en-US/inspector.dtd#54 Then, the change is easy.

(In reply to Javi Rueda from comment #7) > Is this the code for the DOM Inspector add-on? Correct. > http://mxr.mozilla.org/comm-central/source/mozilla/extensions/inspector/ > resources/locale/en-US/inspector.dtd#54 > > Then, the change is easy. Correct.

Changes "Browser" to "Document" and its access key from "B" to "c".

Comment on attachment 559954 [details] [diff] [review] Patch for bug 159027 UNTESTED >- <!ENTITY cmdToggleBrowser.label "Browser"> … >+ <!ENTITY cmdToggleBrowser.label "Document"> Any reason you went for this over the proposed wording? Also, be sure to change the entity name, so it's easier to detect this change for l10n.

Corrected patch: * Label was not "Document" in Browser Pane, but "Document in Browser Pane" * Changed entity names for making easier the work for localizers * Changed name for the id on the menu-item.

Comment on attachment 560559 [details] [diff] [review] Patch for bug 159027 UNTESTED You need to remove the other, non-en-US localizations from resources/Makefile.in. I'm also not sure that "Document in Browser Pane" is the best label. "Show Document in Browser Pane" is probably better. Sorry about that. r+ with those two changes.

(In reply to Colby Russell :crussell from comment #12) > Comment on attachment 560559 [details] [diff] [review] > Patch for bug 159027 UNTESTED > > You need to remove the other, non-en-US localizations from > resources/Makefile.in. > You mean rename the entities in the directories for the locales indicated in Makefile.in? > I'm also not sure that "Document in Browser Pane" is the best label. "Show > Document in Browser Pane" is probably better. Sorry about that. > When opening menu "view" means "I want to view document in Browser pane" or "I want to view the object viewer" and so on. If changing to it "show document" it would be "I want to view show document in browser pane". Anyway, I would change it to whatever string you say to me.

(In reply to Javi Rueda from comment #13) > (In reply to Colby Russell :crussell from comment #12) > > Comment on attachment 560559 [details] [diff] [review] > > Patch for bug 159027 UNTESTED > > > > You need to remove the other, non-en-US localizations from > > resources/Makefile.in. > > > > You mean rename the entities in the directories for the locales indicated in > Makefile.in? No. Just remove those locale entries from the makefile itself. > When opening menu "view" means "I want to view document in Browser pane" Yeah, but not really. None of the other menus work like that. cf Edit or the preferences below the separator in the View menu or the View menu in Firefox. > "I want to view the object viewer" And even though this reading sounds plausible, it doesn't even work like that, either.

(In reply to Colby Russell :crussell from comment #14) > (In reply to Javi Rueda from comment #13) > > (In reply to Colby Russell :crussell from comment #12) > > > Comment on attachment 560559 [details] [diff] [review] > > > Patch for bug 159027 UNTESTED > > > > > > You need to remove the other, non-en-US localizations from > > > resources/Makefile.in. > > > > > > > You mean rename the entities in the directories for the locales indicated in > > Makefile.in? > > No. Just remove those locale entries from the makefile itself. Should the $(NULL) be kept? On the other side, the change to the entity name has been done.

Sorry, I meant 'entity label'.

Yes. A line ending with a backslash means, "This part is continued on the following line." This lets us put each locale on its own line. $(NULL) is a trick that means, "Even though the last line ended with a backslash, that was really the end." If $(NULL) weren't there, it would mean you'd have to also remove the backslash on the en-US line, which wouldn't look very nice in a diff. By leaving the backslash alone, it means that in the generated diff, the only lines modified are the ones that are being removed (or added) entirely.

Renaming the menu item. Modifying Makefile.

Comment on attachment 560880 [details] [diff] [review] Disabling some menu items. > setBrowser: function IA_SetBrowser(aValue, aToggleSplitter) > { > this.mShowBrowser = aValue; > if (aToggleSplitter) { > this.openSplitter("Browser", aValue); > } > var cmd = document.getElementById("cmdToggleDocument"); > cmd.setAttribute("checked", aValue); >+ >+ // Some menu items should be disabled if Browser pane is not showed (checked). Line lengths should be fewer than 80 columns. >+ // Note that btnSelecting is an observer of cmdSelectByClick. >+ var cmdB = document.getElementById("cmdFlashOnSelect"); >+ cmdB.disabled = !aValue; >+ var cmdB = document.getElementById("cmdSelectByClick"); >+ cmdB.disabled = !aValue; A few things here. You should be able to reuse |cmd|; there doesn't appear to be a reason to introduce a new |cmdB| variable. (You've also declared it twice :) cmdSelectByClick doesn't exist (but cmd:selectByClick does); so trying to set |disabled| throws an error. Setting the |disabled| property on a XUL command element doesn't actually do anything. MDN is wrong here. (The common |disabled| setter is implemented in XBL, so only visible controls get this binding.) When trying to toggle command elements' disabled states, you'll need to set and remove the "disabled" attribute directly. Having said that, the suggestions in comment 3 about disabling these menuitems are also wrong. The menuitems in question should only be disabled if the browser pane is closed *and* the inspected document is actually loaded through the browser pane. If someone is inspecting a document that has been loaded in a tab or anywhere else where DOM Inspector has attached to an existing document, the commands shouldn't be disabled when the browser pane is closed. (In fact, in that case, the browser pane is wholly irrelevant, so the browser menuitem itself should be disabled.) But you can save all that for another bug if you like and just stick with the string change.

Comment on attachment 560744 [details] [diff] [review] Renaming the View/Browser menu item [Checkin: comment 24] Did you mean to mark attachment 560744 [details] [diff] [review] as obsolete? It looks like you really want to apply both patches.

(In reply to Colby Russell :crussell from comment #20) > Comment on attachment 560880 [details] [diff] [review] > Disabling some menu items. > > > setBrowser: function IA_SetBrowser(aValue, aToggleSplitter) > > { > > this.mShowBrowser = aValue; > > if (aToggleSplitter) { > > this.openSplitter("Browser", aValue); > > } > > var cmd = document.getElementById("cmdToggleDocument"); > > cmd.setAttribute("checked", aValue); > >+ > >+ // Some menu items should be disabled if Browser pane is not showed (checked). > > Line lengths should be fewer than 80 columns. > > >+ // Note that btnSelecting is an observer of cmdSelectByClick. > >+ var cmdB = document.getElementById("cmdFlashOnSelect"); > >+ cmdB.disabled = !aValue; > >+ var cmdB = document.getElementById("cmdSelectByClick"); > >+ cmdB.disabled = !aValue; > > A few things here. > > You should be able to reuse |cmd|; there doesn't appear to be a reason to > introduce a new |cmdB| variable. (You've also declared it twice :) > > cmdSelectByClick doesn't exist (but cmd:selectByClick does); so trying to > set |disabled| throws an error. > > Setting the |disabled| property on a XUL command element doesn't actually do > anything. MDN is wrong here. (The common |disabled| setter is implemented > in XBL, so only visible controls get this binding.) When trying to toggle > command elements' disabled states, you'll need to set and remove the > "disabled" attribute directly. > > Having said that, the suggestions in comment 3 about disabling these > menuitems are also wrong. The menuitems in question should only be disabled > if the browser pane is closed *and* the inspected document is actually > loaded through the browser pane. If someone is inspecting a document that > has been loaded in a tab or anywhere else where DOM Inspector has attached > to an existing document, the commands shouldn't be disabled when the browser > pane is closed. (In fact, in that case, the browser pane is wholly > irrelevant, so the browser menuitem itself should be disabled.) > > But you can save all that for another bug if you like and just stick with > the string change. About comment 21: comment 0 talks about 2 changes, 1 for the string and 1 for disabling the menu items, so I prefered to split those into 2 patches in order to make the whole process easier to everybody, although second one was very different from the one that gives the title to this bug. Then, once feedback was ok, resend it to review and once a positive review, ask for check-in both patches. (Yes, I normally choose the wrong way to do things). Then, is it ok to ask for check-in and let them mark as resolved-fixed? Also, I am thankful for explanation in comment 17 :-).

(In reply to Javi Rueda from comment #22) > Then, is it ok to ask for check-in and let them mark as resolved-fixed? It's okay to check in attachment 560744 [details] [diff] [review], but not attachment 560880 [details] [diff] [review] for the reasons given in comment 20. Do you have commit privileges? If you ever submit a patch that gets a positive review, you can add the "checkin-needed" keyword above and someone will come along to do that for you. Also, regarding attachment attachment 560880 [details] [diff] [review]: you should mark an attachment as review? when you'd like someone to review it and use the feedback flag when you'd like some comments from a relevant third party who isn't the person performing the review. > Also, I am thankful for explanation in comment 17 :-). Thanks for the patch. :)

Comment on attachment 560744 [details] [diff] [review] Renaming the View/Browser menu item [Checkin: comment 24] http://hg.mozilla.org/dom-inspector/rev/82eb29e08cf4