Remove or hide the more/less button in details pane for items which doesn't use extended properties (e.g. history)

VERIFIED FIXED in Firefox 3.7a1

Status

()

Firefox
Bookmarks & History
VERIFIED FIXED
10 years ago
7 years ago

People

(Reporter: whimboo, Assigned: Margaret)

Tracking

(Depends on: 1 bug, {polish, verified1.9.2})

Trunk
Firefox 3.7a1
polish, verified1.9.2
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(status1.9.2 beta1-fixed)

Details

Attachments

(1 attachment, 11 obsolete attachments)

(Reporter)

Description

10 years ago
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008042122 Minefield/3.0pre

The patch on bug 407541 also fixes bug 415824 which hides unnecessary history item properties within the details pane now. With that change the more/less button at the bottom is useless because no extended information will be displayed. So it should also be removed/hidden.

Updated

10 years ago
Keywords: polish

Updated

10 years ago
Duplicate of this bug: 430511

Updated

10 years ago
Duplicate of this bug: 430811

Updated

10 years ago
Duplicate of this bug: 436710

Comment 4

9 years ago
Is this going to be resolved? I notice it still exists on Firefox 3.1b3, but it seems from a layman's perspective to be a somewhat trivial fix.

Comment 5

9 years ago
Created attachment 367299 [details] [diff] [review]
v1

How's this?
Attachment #367299 - Flags: review?(mano)
Comment on attachment 367299 [details] [diff] [review]
v1

This will get broken as we add fields.

I think we should:
1. Create some extraFields araay within the dialog code.
2. Avoid the css solution
3. Use that array here too.

It should be straightforward.
Attachment #367299 - Flags: review?(mano) → review-

Comment 7

9 years ago
Created attachment 370728 [details] [diff] [review]
v2

(In reply to comment #6)
> 1. Create some extraFields araay within the dialog code.

I named it "_additionalInfoFields" to sync with toggleAdditionalInfoFields().

> 2. Avoid the css solution

It sounds like your concern is that you want to specify the extra fields in one easy-to-update place instead of in multiple places or in CSS.  This patch still uses CSS to do the actual hiding, but it no longer relies the fields' being specified in CSS.
Attachment #367299 - Attachment is obsolete: true
Attachment #370728 - Flags: review?(mano)

Updated

9 years ago
Assignee: nobody → adw
Comment on attachment 370728 [details] [diff] [review]
v2

This is still complex (setting a "static" attribute dynamically is just bad). Let's do get rid of the css rule.
Attachment #370728 - Flags: review?(mano) → review-

Updated

9 years ago
Assignee: adw → mleibovic

Comment 9

9 years ago
Here's a conversation I had with Mano about one part of this bug -- hiding extra info fields when the more/less button is toggled.  He suggests adding a broadcaster to editBookmarkOverlay that the extra info fields observe.  Then we can toggle the collapsed attribute of the observer to control the state of the fields.  One thing to watch out for, though, is that gEditItemOverlay._showHideRows() already collapses fields that the overlay's consumers don't need, so we should be careful not to un-collapse a field that the consumer has requested be collapsed.  Maybe using hidden instead of collapsed on the observer would be better.

[5:15pm] adw: denied! ouch
[5:16pm] Mano: just use the collapsed property
[5:16pm] Mano: though i like you use of every
[5:16pm] Mano: your*
[5:16pm] adw: Mano: the reason i kept the css solution is because the edit pane uses collapsed itself
[5:16pm] Mano: it uses hidden, doesn't it?
[5:17pm] adw: or rather, we collapse widgets that aren't appropriate
[5:17pm] Mano: and if it doesn't "it should".
[5:17pm] Mano: .hidden != .collapsed
[5:17pm] adw: so we're going to be using both hidden and collapsed?
[5:17pm] adw: that seems... confusing
[5:18pm] Mano: i don't think it does, but just put a nice comment explaining the rational
[5:18pm] Mano: moving the code to some other file doesn't make it less confusing 
[5:19pm] adw: i guess we disagree, i think using an attribute to indicate this widget is additional is cleaner, but will do
[5:20pm] Mano: adw really, setAttribute should be used only when the attribute isn't always set
[5:20pm] Mano: or has different values
[5:20pm] adw: Mano: that's understandable, but when the widgets are brought in from an overlay, that's not feasible
[5:21pm] adw: err
[5:21pm] adw: nm
[5:21pm] Mano: i'm temped to say "overlay to overlay", but understandably you won't do that 
[5:21pm] adw: the reason i was setting them dynamically was to keep them all together in the one array
[5:21pm] Mano: yeah i know
[5:22pm] Mano: you could also use a broadcaster/observer
[5:22pm] Mano: i'm fine with adding that to the overlay and only use it in places.xul
[5:23pm] adw: really?  adding additionalInfoField atts to widgets in editBookmarksOverlay and using them only in places.xul?
[5:25pm] Mano: wait, the extra fields are in the overlay right?
[5:26pm] adw: yes
[5:26pm] Mano: if we add a a broadcaster there named, say, "extraFields"
[5:26pm] Mano: and then you set the broadcaster attribute on each of the extra fields
[5:26pm] Mano: then set the "collapsed" attribute on the broadcaster in places.js
[5:26pm] Mano: that's fine.
(Assignee)

Comment 10

9 years ago
Created attachment 380223 [details] [diff] [review]
v3

I added a broadcaster and have the additional fields observe the hidden attribute. I left the array, since it's still used to determine infoBoxExpanderWrapper.hidden by looking at the collapsed attributes of those field elements.
Attachment #380223 - Flags: review?(mano)

Comment 11

9 years ago
Comment on attachment 380223 [details] [diff] [review]
v3

Just a first-pass for Mano...

>diff --git a/browser/components/places/content/places.js b/browser/components/places/content/places.js
>--- a/browser/components/places/content/places.js
>+++ b/browser/components/places/content/places.js
>@@ -624,18 +633,22 @@ var PlacesOrganizer = {
>         infoBox.setAttribute("wasminimal", "true");
>       infoBox.removeAttribute("minimal");
>       infoBoxExpanderWrapper.hidden = true;
>     }
>     else {
>       if (infoBox.getAttribute("wasminimal") == "true")
>         infoBox.setAttribute("minimal", "true");
>       infoBox.removeAttribute("wasminimal");
>-      infoBoxExpanderWrapper.hidden = false;
>+      infoBoxExpanderWrapper.hidden =
>+        this._additionalInfoFields.every(function (id)
>+          document.getElementById(id).collapsed);
>     }
>+    if (infoBox.getAttribute("minimal") == "true")
>+      additionalInfoBroadcaster.hidden = true;

additionalInfoBroadcaster also needs to be unhidden if infoBox is not minimal, as you do in toggleAdditionalInfoFields.  With this patch, if you for example select "Recent Tags" in the left pane, toggle the expander twice and then click a regular bookmark folder in the left pane, the description field will be hidden.

Other than that, looks fine to me.  Could you fix it, test it out and make sure it works, and then request Mano's review again?
Attachment #380223 - Flags: review?(mano) → review-

Updated

9 years ago
Attachment #370728 - Attachment is obsolete: true
Drew - do you think this is a candidate for an automated test?

Comment 13

9 years ago
Yeah. :)  There are really two things to test: 1) The extra info fields are hidden as appropriate to the type of node selected and expander state, and 2) the expander is hidden if there's nothing to expand.  Situations like the example in comment 11, too.

Comment 14

9 years ago
Margaret, the tests we're referring to are browser chrome tests.  They're good for testing UI because they allow you to open up parts of the browser's front end and automate anything a live user might do:

https://developer.mozilla.org/en/Browser_chrome_tests

The test here will need to open the Library, select some nodes in the left pane, toggle the expander button, and make sure things are hidden properly.  The existing browser chrome tests for the Places front end offer some good examples of automating these types of actions.  They live here:

http://mxr.mozilla.org/mozilla-central/source/browser/components/places/tests/browser/

In particular these two open up the Library and select stuff in the left pane's tree:

http://mxr.mozilla.org/mozilla-central/source/browser/components/places/tests/browser/browser_library_left_pane_commands.js

http://mxr.mozilla.org/mozilla-central/source/browser/components/places/tests/browser/browser_library_search.js

This doc is more general, but it explains how the Places tree view works:

https://developer.mozilla.org/en/Displaying_Places_information_using_views#Using_the_tree_view

If you need help, feel free to ping me or anybody else in #places. :)
(Reporter)

Updated

9 years ago
Flags: in-testsuite?
(Assignee)

Comment 15

9 years ago
Created attachment 380893 [details] [diff] [review]
v4

Added browser chrome test.
Attachment #380893 - Flags: review?(adw)

Comment 16

9 years ago
Comment on attachment 380893 [details] [diff] [review]
v4

>diff --git a/browser/components/places/content/organizer.css b/browser/components/places/content/organizer.css
>\ No newline at end of file

Could you add a newline here?

>diff --git a/browser/components/places/content/places.js b/browser/components/places/content/places.js
>+  // IDs of fields from editBookmarkOverlay that should be hidden when infoBox
>+  // is minimal
>+  _additionalInfoFields: [
>+    "editBMPanel_descriptionRow",
>+    "editBMPanel_loadInSidebarCheckbox",
>+    "editBMPanel_keywordRow",
>+  ],

Could you note in this comment that these names should be kept in sync with the IDs of the elements observing additionalInfoBroadcaster?

>+    if (infoBox.getAttribute("minimal") == "true")
>+      additionalInfoBroadcaster.hidden = true;
>+    else
>+      additionalInfoBroadcaster.hidden = false;

These could be rewritten to one line, additionalInfoBroadcaster.hidden = ...

>diff --git a/browser/components/places/tests/browser/browser_430148.js b/browser/components/places/tests/browser/browser_430148.js

There are a bunch of older tests whose filenames are bug numbers, but we're trying now to give them more descriptive names.  Maybe browser_library_infoBox.js?

>+    // Add a visit
>+    PlacesUtils.history.addVisit(PlacesUtils._uri(TEST_URI), Date.now() * 1000,
>+                                 null, PlacesUtils.history.TRANSITION_TYPED,
>+                                 false, 0);

Unfortunately profile state including bookmarks and history is persisted across test runs.  So any visits or bookmarks you add in one test will appear in the next test unless you remove them first.  In this case you should call nsIBrowserHistory.removeAllPages at the end of your test:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/public/nsIBrowserHistory.idl#127

>+    var infoBoxExpanderWrapper = gLibrary.document.getElementById("infoBoxExpanderWrapper");

Please keep lines 80 chars or less.

>+    isnot(infoBoxExpanderWrapper, null,
>+      "Correctly grabbed expander button wrapper.");

Please indent the string argument here with the left paren.  We do that in general for args that are on different lines from their functions, unless the function name ends on like the 75th char or something.

Actually there's a JS style guide at https://developer.mozilla.org/En/Javascript_Style_Guide, although different modules often have slightly different styles.  Mostly you get a feel for whatever module you're working on.

>+    PO.selectLeftPaneQuery('AllBookmarks');

We tend to use double quotes for strings, so please use them here.

>+    ok(infoBoxExpanderWrapper.getAttribute("hidden"),
>+      "Expander button is hidden for all bookmarks node.");

While it's true that in practice infoBoxExpanderWrapper.getAttribute("hidden") is truthy iff infoBoxExpanderWrapper is actually hidden, it would be more correct to check infoBoxExpanderWrapper.getAttribute("hidden") == "true" or better yet, infoBoxExpanderWrapper.hidden.

>+    // open today node
>+    var historyNode = PO._places.selectedNode
>+                          .QueryInterface(Ci.nsINavHistoryContainerResultNode);

Dots should trail -- not lead -- when an expression is more than one line, and please indent the QI so it's even with the PO.

>+    historyNode.containerOpen = true;
>+    PO._places.selectNode(historyNode.getChild(0));
>+    is(PO._places.selectedNode.title, "Today",
>+      "We correctly selected today node.");

I don't think it's important to make sure the node is the Today node.  You added a visit up top, so it should be OK to just sanity check that the first child actually exists before selecting it.

>+    // open history item: expander button should be hidden
>+    var view = PO._content.treeBoxObject.view;
>+    if (view.rowCount > 0)
>+      view.selection.select(0);

Generally in tests you should assert that as much as possible is true or false instead of checking whether things exists.  You added a visit, so the history item should exist, and you should make sure it does.

>+    ok(infoBoxExpanderWrapper.getAttribute("hidden"),
>+      "Expander button is hidden for history item.");
>+

historyNode should be closed here after you're done with it.  Same with menuNode farther down.

>+    // open recently bookmarked node
>+    var menuNode = PO._places.selectedNode
>+                        .QueryInterface(Ci.nsINavHistoryContainerResultNode);
>+    menuNode.containerOpen = true;
>+    PO._places.selectNode(menuNode.getChild(0));
>+    is(PO._places.selectedNode.title, "Recently Bookmarked",
>+      "We correctly selected recently bookmarked node.");

It's better if our tests our locale-independent; using "Recently Bookmarked" assumes the test will always run under en-US.  (Actually that's the case now, but maybe not in the future.)  You should get the node's title from the Places string bundle.  A good example is:

http://mxr.mozilla.org/mozilla-central/source/browser/components/places/tests/browser/browser_library_left_pane_commands.js#77

Except you want to pass "recentlyBookmarkedTitle" to getString.

Also a couple of deeper things:  1) This test doesn't check that the extra info fields are (un)hidden as appropriate.  Since your patch changes how that's done and we don't already have a test for it, you should test it here.  That includes a) making sure the fields are actually hidden upon selecting a node (since more/less starts out in the less state), b) toggling the more/less button to make sure the hidden fields become unhidden, and c) toggling more/less once again to make sure the fields are re-hidden.  2) This test misses the example in comment 11.  Toggling more/less as described in 1 and then moving on to the next node should be sufficient.

This is a lot, but good work so far! :)
Attachment #380893 - Flags: review?(adw) → review-
(Assignee)

Comment 17

9 years ago
Created attachment 381131 [details] [diff] [review]
v5
Attachment #380223 - Attachment is obsolete: true
Attachment #380893 - Attachment is obsolete: true
Attachment #381131 - Flags: review?(adw)

Updated

9 years ago
Attachment #381131 - Flags: review?(adw) → review-

Comment 18

9 years ago
Comment on attachment 381131 [details] [diff] [review]
v5

>diff --git a/browser/components/places/tests/browser/browser_library_infoBox.js b/browser/components/places/tests/browser/browser_library_infoBox.js

>+    ok(infoBoxExpanderWrapper.getAttribute("hidden"),
>+       "Expander button is hidden for all bookmarks node.");

Please always check infoBoxExpanderWrapper.hidden instead.

>+    // open history child node
>+    var historyNode = PO._places.selectedNode.
>+                      QueryInterface(Ci.nsINavHistoryContainerResultNode);

Check more/less here too.

>+    isnot(recentlyBookmarkedTitle, null, recentlyBookmarkedTitle);

You should pass a helpful string as the assertion message, like "Correctly got the recently bookmarked title locale string".

>+    // make sure additional fields are hidden by default
>+    var addInfoBroadcaster = gLibrary.document.
>+                             getElementById("additionalInfoBroadcaster");
>+    isnot(addInfoBroadcaster, null,
>+          "Correctly grabbed additional info broadcaster.");
>+    ok(addInfoBroadcaster.getAttribute("hidden"),
>+       "Additional info fields are hidden by default.");

This will work, but it's testing your implementation details instead of the desired outcome.  It would be better to make sure each individual field is hidden.

>+    // toggle fields and make sure they are hidden/unhidden as expected
>+    PO.toggleAdditionalInfoFields();

Similarly, it would be better to click the button instead of directly calling the function that's called when you click the button.  You can grab the button element and button.click().

Could you do these two checks -- make sure additional info fields are hidden by default and the double toggle -- for each node you visit?  The same way you check that more/less is hidden.  You could put them in a helper function.
(Assignee)

Comment 19

9 years ago
Created attachment 381184 [details] [diff] [review]
v6

I didn't test the additional info fields visibility in nodes where the additional info fields are collapsed, since they won't be hidden because they're collapsed. Do you want me to make tests that make sure these are collapsed like I'm expecting them to be?
Attachment #381131 - Attachment is obsolete: true
Attachment #381184 - Flags: review?(adw)

Comment 20

9 years ago
Comment on attachment 381184 [details] [diff] [review]
v6

>diff --git a/browser/components/places/content/places.js b/browser/components/places/content/places.js

>     if (!aNode) {
>       infoBoxExpanderWrapper.hidden = true;
>@@ -629,8 +639,11 @@ var PlacesOrganizer = {
>       if (infoBox.getAttribute("wasminimal") == "true")
>         infoBox.setAttribute("minimal", "true");
>       infoBox.removeAttribute("wasminimal");
>-      infoBoxExpanderWrapper.hidden = false;
>+      infoBoxExpanderWrapper.hidden =
>+        this._additionalInfoFields.every(function (id)
>+          document.getElementById(id).collapsed);
>     }
>+    additionalInfoBroadcaster.hidden = infoBox.getAttribute("minimal");

Please check that the minimal attribute == "true".  As before, in practice it's enough to check hasAttribute("minimal") but more correct to check that its value == "true".

(In reply to comment #19)
> I didn't test the additional info fields visibility in nodes where the
> additional info fields are collapsed, since they won't be hidden because
> they're collapsed. 

Actually collapsed and hidden are independent, but yeah, what I said about toggling infoBoxExpanderWrapper for each node is wrong since it doesn't make sense to toggle it if it's hidden.  The button should be toggled only when infoBoxExpanderWrapper is not hidden.

> Do you want me to make tests that make sure these are
> collapsed like I'm expecting them to be?

Hmm, a separate test file isn't necessary but now that you mention it, whenever you check that infoBoxExpanderWrapper is hidden, you should also check that each elt in _additionalInfoFields is collapsed, and when infoBoxExpanderWrapper is not hidden, that at least one elt in _additionalInfoFields is not collapsed.

>diff --git a/browser/components/places/tests/browser/browser_library_infoBox.js b/browser/components/places/tests/browser/browser_library_infoBox.js

>+    // open history item
>+    var view = PO._content.treeBoxObject.view;
>+    ok(view.rowCount > 0, "History item exists.");
>+    view.selection.select(0);
>+    ok(infoBoxExpanderWrapper.hidden,
>+       "Expander button is hidden for history item.");
>+    checkAddInfoFields(PO, "history item");

As I (now :)) say, since infoBoxExpanderWrapper is hidden here, it's OK to not checkAddInfoFields().

>+    // open recently bookmarked node
>+    var menuNode = PO._places.selectedNode.
>+                   QueryInterface(Ci.nsINavHistoryContainerResultNode);
>+    menuNode.containerOpen = true;
>+    var childNode = menuNode.getChild(0);
>+    isnot(childNode, null, "Bookmarks menu child node exists.");
>+    PO._places.selectNode(childNode);
>+    var recentlyBookmarkedTitle = PlacesUIUtils.
>+                                  getString("recentlyBookmarkedTitle");
>+    isnot(recentlyBookmarkedTitle, null,
>+          "Correctly got the recently bookmarked title locale string.");
>+    is(PO._places.selectedNode.title, recentlyBookmarkedTitle,
>+       "Correctly selected recently bookmarked node.");

Nit: Since you've already got the recently bookmarked node in childNode, it would be clearer to use that instead of PO._places.selectedNode in the title check.  Then you can check the title and make sure you have the right node before you select it.

>+    checkAddInfoFields(PO, "recently bookmarked node");

Also, don't forget to check infoBoxExpanderWrapper.hidden here.

>+  PO._additionalInfoFields.forEach(function(id){

Nit: Spaces between function (id) { on these three lines.

OK, one more patch and I think it should be good to go.
Attachment #381184 - Flags: review?(adw) → review-
(Assignee)

Comment 21

9 years ago
Created attachment 381313 [details] [diff] [review]
v7
Attachment #381184 - Attachment is obsolete: true
Attachment #381313 - Flags: review?(adw)

Updated

9 years ago
Attachment #381313 - Flags: review?(adw) → review-

Comment 22

9 years ago
Comment on attachment 381313 [details] [diff] [review]
v7

>diff --git a/browser/components/places/tests/browser/browser_library_infoBox.js b/browser/components/places/tests/browser/browser_library_infoBox.js

>+    // open recently bookmarked node
>+    var menuNode = PO._places.selectedNode.
>+                   QueryInterface(Ci.nsINavHistoryContainerResultNode);
>+    menuNode.containerOpen = true;
>+    var childNode = menuNode.getChild(0);

I didn't notice this before, but this is the second declaration of childNode.

>+    isnot(childNode, null, "Bookmarks menu child node exists.");
>+    var recentlyBookmarkedTitle = PlacesUIUtils.
>+                                  getString("recentlyBookmarkedTitle");
>+    isnot(recentlyBookmarkedTitle, null,
>+          "Correctly got the recently bookmarked title locale string.");
>+    is(childNode.title, recentlyBookmarkedTitle,
>+       "Correctly selected recently bookmarked node.");
>+    PO._places.selectNode(childNode);
>+
>+    // open first bookmark

Don't forget to do all the appropriate checks for the recently bookmarked node.

>+function checkAddInfoFieldsNotCollapsed(PO) {
>+  PO._additionalInfoFields.forEach(function (id) {
>+    ok(!getAndCheckElmtById(id).collapsed,
>+       "Additional info field correctly not collapsed: #" + id);
>+  });
>+}

You want to make sure that there exists a field that's not collapsed, not that every field is not collapsed.  (Check out https://developer.mozilla.org/en/Core_JavaScript_1.5_Reference/Global_Objects/Array/some)
(Assignee)

Comment 23

9 years ago
Created attachment 381554 [details] [diff] [review]
v8
Attachment #381313 - Attachment is obsolete: true
Attachment #381554 - Flags: review?(adw)

Comment 24

9 years ago
Comment on attachment 381554 [details] [diff] [review]
v8

>diff --git a/browser/components/places/tests/browser/browser_library_infoBox.js b/browser/components/places/tests/browser/browser_library_infoBox.js

>+    // open top-level folders

Nit: open the All Bookmarks node

>+    PO.selectLeftPaneQuery("History");

Please add a comment about what you're opening above this line.

>+    PO.selectLeftPaneQuery("BookmarksMenu");

Ditto.

>+    // open recently bookmarked node
>+    var menuNode = PO._places.selectedNode.
>+                   QueryInterface(Ci.nsINavHistoryContainerResultNode);
>+    menuNode.containerOpen = true;
>+    childNode = menuNode.getChild(0);
>+    isnot(childNode, null, "Bookmarks menu child node exists.");
>+    var recentlyBookmarkedTitle = PlacesUIUtils.
>+                                  getString("recentlyBookmarkedTitle");
>+    isnot(recentlyBookmarkedTitle, null,
>+          "Correctly got the recently bookmarked title locale string.");
>+    is(childNode.title, recentlyBookmarkedTitle,
>+       "Correctly selected recently bookmarked node.");
>+    PO._places.selectNode(childNode);
>+    ok(infoBoxExpanderWrapper.hidden,
>+       "Expander button is hidden for bookmarks menu node.");

Nit: "Expander button is hidden for recently bookmarked node."

>+    checkAddInfoFieldsCollapsed(PO);

For the recently bookmarked node, infoBoxExpanderWrapper should not be hidden and some field should not be collapsed.  These two checks should fail...

... but they don't because of a dumb problem I didn't notice until now.  If a selection exists in PO._places but PO._places is not focused, infoBox is hidden and its parent deck shows itemsCountBox instead, which displays a message like "N items, Select an item to view and edit its properties".  These two checks therefore pass when they shouldn't.  (You can see for yourself by commenting out everything in run() after this line.)  Honestly I don't know why during some checks PO._places is focused and during other checks it's not.

So, after you select a node in the tree could you 1) manually focus PO._places and 2) make sure the selected element in detailsDeck is infoBox?
Attachment #381554 - Flags: review?(adw) → review-
(Assignee)

Comment 25

9 years ago
Created attachment 381682 [details] [diff] [review]
v9
Attachment #381554 - Attachment is obsolete: true
Attachment #381682 - Flags: review?(adw)

Updated

9 years ago
Attachment #381682 - Flags: review?(adw) → review+

Comment 26

9 years ago
Comment on attachment 381682 [details] [diff] [review]
v9

>diff --git a/browser/components/places/tests/browser/browser_library_infoBox.js b/browser/components/places/tests/browser/browser_library_infoBox.js

>+/**
>+ *  Test enabled commands in the left pane folder of the Library.
>+ */

Whoops, please replace this comment with an accurate one.

>+    PO.selectLeftPaneQuery("BookmarksMenu");
>+    isnot(PO._places.selectedNode, null,
>+          "Correctly selected bookmarks menu node.");

Please add a comment about what you're opening above these lines.

>+    checkInfoBoxSelected(PO);

>+    // open first bookmark
>+    var view = PO._content.treeBoxObject.view;
>+    ok(view.rowCount > 0, "Bookmark item exists.");
>+    view.selection.select(0);

checkInfoBoxSelected(PO);

>+    // make sure additional fields are still hidden in second bookmark item
>+    ok(view.rowCount > 1, "Second bookmark item exists.");
>+    view.selection.select(1);

checkInfoBoxSelected(PO);

With those changes, r=adw.  Let's see what Mano says on the new patch...

\o/
(Assignee)

Comment 27

9 years ago
Created attachment 381795 [details] [diff] [review]
v10
Attachment #381682 - Attachment is obsolete: true
Attachment #381795 - Flags: review?(mano)
Comment on attachment 381795 [details] [diff] [review]
v10

OK.. Sorry for the long delay here.

Please make sure to add a comment in the xul file on the sync "issue". r=mano otherwise.
Created attachment 400017 [details] [diff] [review]
for checkin
Attachment #381795 - Attachment is obsolete: true
Created attachment 400018 [details] [diff] [review]
for checkin (for real)

wrong patch, sorry
Attachment #400017 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/657b6d9cef9b
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a1
Comment on attachment 400018 [details] [diff] [review]
for checkin (for real)

asking approval. nice polish and has a test.
Attachment #400018 - Flags: approval1.9.2?
(Reporter)

Comment 33

9 years ago
Verified fixed on trunk with builds on all platforms like Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20090914 Minefield/3.7a1pre (.NET CLR 3.5.30729) ID:20090914045953

The patch has automated tests.
Status: RESOLVED → VERIFIED
Flags: in-testsuite? → in-testsuite+
Comment on attachment 400018 [details] [diff] [review]
for checkin (for real)

a192=beltzner
Attachment #400018 - Flags: approval1.9.2? → approval1.9.2+
(Reporter)

Comment 36

9 years ago
Verified fixed on 1.9.2 with builds on all platforms like Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2b1pre) Gecko/20090930 Namoroka/3.6b1pre ID:20090930033826
Keywords: verified1.9.2
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
Depends on: 647983
You need to log in before you can comment on or make changes to this bug.