+++ This bug was initially created as a clone of Bug #1208145 +++ See attachment 8698997 [details] So that I can safer in some situations, as a Firefox user, I want to show only the passwords I want to show and have selected. See attached animation. Acceptance criteria: * When the user selects one or more rows, the Show button is enabled * When the user clicks to Show password, password is shown, the button converts to become a Hide button, and the row remains selected * When the user has the password column hidden, and presses Show, the password column becomes unmasked with the selected password shown * With passwords shown, when the user makes a different selection, or leaves the password section, any shown passwords become hidden again and the Hide button becomes a Show button again * When no rows are selected, the Show button is disabled * When no passwords exist, the Show button is disabled == Some of the code from bug 1121291 can be reused (that should probably stay a separate patch for easier review) to make the column show by default but we now also want a button, not just double-click to edit (as that wasn't discoverable enough). This bug should focus on the "Edit password" button. See also bug 1209267 about masking.

Hey Matthew, do you think it will need a multi-selection feature here?

Hey Matthew, After taking a look the source codes, I have some questions about how to implement the feature you mentioned: 1. For each password item, we will use type="password" to show/hide the password content. However, I didn't find a proper way to do that. (I tried getCellProperties to return a string "type=\"password\"" or "type=\"text\"") 2. I suppose that selectionChanged[1] of nsITreeView is a way to know when the rows are selected. It seems not work. Is this way correct? Thanks! :) [1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsITreeView#selectionChanged()

(In reply to Sean Lee [:seanlee][:weilonge] from comment #2) > After taking a look the source codes, I have some questions about how to > implement the feature you mentioned: > 1. For each password item, we will use type="password" to show/hide the > password content. However, I didn't find a proper way to do that. (I tried > getCellProperties to return a string "type=\"password\"" or "type=\"text\"") I think we'll just have setCellText just return the mask characters when the password should be hidden for a row. > 2. I suppose that selectionChanged[1] of nsITreeView is a way to know when > the rows are selected. It seems not work. Is this way correct? I don't have time to look into this today.

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #3) > I think we'll just have setCellText just return the mask characters when the > password should be hidden for a row. Sorry, I meant getCellText but then we need to make sure when the user double-clicks to edit the password that the real password shows.

(In reply to Sean Lee [:seanlee][:weilonge] (PTO: 6/7~6/10, WW: 6/13~6/17)) from comment #2) > 2. I suppose that selectionChanged[1] of nsITreeView is a way to know when > the rows are selected. It seems not work. Is this way correct? Can you attach your patch that tried to use selectionChanged? It will be easier for me to debug that than research.

Review commit: https://reviewboard.mozilla.org/r/57826/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/57826/

Hey MattN, Please see my WIP patch at attachment 8760114 [details]. Thanks :) (Sorry for no update. I am working on Content Handling stuff.)

I'm not sure what selectionChanged is for. I wonder if it's leftover or something? Anyways, https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Tutorial/Tree_Selection explains to use @onselect and our tree is already doing that and calling `SignonSelected()` so you can use that. I actually think our multi-selection handling might already be broken so if you want to fix that please do it in a separate commit first then add the toggling logic in a 2nd commit. Thanks.

Comment on attachment 8760114 [details] Bug 1257078 - Implement a selection feature for showing some specific password.; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57826/diff/1-2/

Comment on attachment 8760114 [details] Bug 1257078 - Implement a selection feature for showing some specific password.; Hey Matthew, Could you help to review the patch? I use a property "plainText" to decide if the password text field shows plain text or not. At beginning, I tried to modify getCellText function in passwordManager.js, but I found that it has to handle many states like showing mask char, copying password, and editing inline. The patch would be difficult to maintain and read. Letting nsTreeBodyFrame::PaintText in nsTreeBodyFrame.cpp to handle showing plain text is a easier way for me. However, there might be some issues that I didn't see. Thank you!

Comment on attachment 8760114 [details] Bug 1257078 - Implement a selection feature for showing some specific password.; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57826/diff/2-3/

The patch attachment 8760114 [details] fixes one part of mochitest. In browser_passwordmgrdlg.js, step 3 is skipped because I am trying to find a better solution.

https://reviewboard.mozilla.org/r/57826/#review62412 ::: layout/xul/tree/nsTreeBodyFrame.cpp:3696 (Diff revision 3) > - if (aColumn->Type() == nsITreeColumn::TYPE_PASSWORD) { > + nsAutoString properties; > + mView->GetCellProperties(aRowIndex, aColumn, properties); > + nsTreeUtils::TokenizeProperties(properties, mScratchArray); > + > + NS_NAMED_LITERAL_STRING(plainText, "plainText"); > + > + if (aColumn->Type() == nsITreeColumn::TYPE_PASSWORD && !properties.Equals(plainText)) { > nsTextEditRules::FillBufWithPWChars(&text, text.Length()); My concern is that I don't know if there's precedence for properties having predefined behaviour. Normally the consumer sets the properties and also uses them to style but I don't know if there are other magic properties that change behaviour. I saw this a few weeks ago and thought I would think about it more but it ended up just delaying the reviewing more. I should have probably just mentioned this and you could have investigated the concern. One idea may be to have an attribute on the tree itself which we toggle when the button is clicked. That attribute indicates whether selected rows should reveal their password? Ideally we would have support for the equivalent of -webkit-text-security so we could implement this outside of the tree implementation but I actually don't think that CSS property is a good idea to expose to the web.

Comment on attachment 8760114 [details] Bug 1257078 - Implement a selection feature for showing some specific password.; See comment 13

(In reply to Matthew N. [:MattN] from comment #13) > https://reviewboard.mozilla.org/r/57826/#review62412 > > ::: layout/xul/tree/nsTreeBodyFrame.cpp:3696 > (Diff revision 3) > > - if (aColumn->Type() == nsITreeColumn::TYPE_PASSWORD) { > > + nsAutoString properties; > > + mView->GetCellProperties(aRowIndex, aColumn, properties); > > + nsTreeUtils::TokenizeProperties(properties, mScratchArray); > > + > > + NS_NAMED_LITERAL_STRING(plainText, "plainText"); > > + > > + if (aColumn->Type() == nsITreeColumn::TYPE_PASSWORD && !properties.Equals(plainText)) { > > nsTextEditRules::FillBufWithPWChars(&text, text.Length()); > > My concern is that I don't know if there's precedence for properties having > predefined behaviour. Normally the consumer sets the properties and also > uses them to style but I don't know if there are other magic properties that > change behaviour. Could you give a specific example to explain "also uses them to style"? Will the precedence issue be a critical cause in this patch? > > I saw this a few weeks ago and thought I would think about it more but it > ended up just delaying the reviewing more. I should have probably just > mentioned this and you could have investigated the concern. > > One idea may be to have an attribute on the tree itself which we toggle when > the button is clicked. That attribute indicates whether selected rows should > reveal their password? Ideally we would have support for the equivalent of > -webkit-text-security so we could implement this outside of the tree > implementation but I actually don't think that CSS property is a good idea > to expose to the web. Is [1] a right place to add a new attribute for tree component? May I say your thoughts is like this pseudo code? <property name="showSelectedPassword" onget="... JS codes ..." onset="showSelectedPassword(val);"/> then manipulate the tree like this: document.querySelector("tree#signonsTree").setAttribute("showSelectedPassword", "true"); Another idea was to manipulate the password record in treechildren, but it had no way to get the dom element of each password record. For example, I would like to get the DOM element at Password column-row[1]. This approach is another way to control displaying plain text or dots. I am not sure that I fully understand your suggestion. Please point it out if there is something incorrect. Thank you. [1] http://searchfox.org/mozilla-central/source/toolkit/content/widgets/tree.xml

(In reply to Sean Lee [:seanlee][:weilonge] from comment #15) > (In reply to Matthew N. [:MattN] from comment #13) > > https://reviewboard.mozilla.org/r/57826/#review62412 > > > > ::: layout/xul/tree/nsTreeBodyFrame.cpp:3696 > > (Diff revision 3) > > > - if (aColumn->Type() == nsITreeColumn::TYPE_PASSWORD) { > > > + nsAutoString properties; > > > + mView->GetCellProperties(aRowIndex, aColumn, properties); > > > + nsTreeUtils::TokenizeProperties(properties, mScratchArray); > > > + > > > + NS_NAMED_LITERAL_STRING(plainText, "plainText"); > > > + > > > + if (aColumn->Type() == nsITreeColumn::TYPE_PASSWORD && !properties.Equals(plainText)) { > > > nsTextEditRules::FillBufWithPWChars(&text, text.Length()); > > > > My concern is that I don't know if there's precedence for properties having > > predefined behaviour. Normally the consumer sets the properties and also > > uses them to style but I don't know if there are other magic properties that > > change behaviour. > Could you give a specific example to explain "also uses them to style"? > Will the precedence issue be a critical cause in this patch? What I'm saying is that I don't there there are any other cases in mozilla-central where the tree "property" automatically changes the visual style on a tree (without writing your own CSS for that). I don't think properties were made to change the default styles so I don't think we should use it for that (like your patch does). > > I saw this a few weeks ago and thought I would think about it more but it > > ended up just delaying the reviewing more. I should have probably just > > mentioned this and you could have investigated the concern. > > > > One idea may be to have an attribute on the tree itself which we toggle when > > the button is clicked. That attribute indicates whether selected rows should > > reveal their password? Ideally we would have support for the equivalent of > > -webkit-text-security so we could implement this outside of the tree > > implementation but I actually don't think that CSS property is a good idea > > to expose to the web. > > Is [1] a right place to add a new attribute for tree component? > May I say your thoughts is like this pseudo code? > > <property name="showSelectedPassword" > onget="... JS codes ..." > onset="showSelectedPassword(val);"/> I don't think we need to make it a property, using the content attribute is enough I think. > then manipulate the tree like this: > document.querySelector("tree#signonsTree"). > setAttribute("showSelectedPassword", "true"); Yes, like that. How about "revealSelectedPasswords"? "Reveal" is more clear and "passwords" is plural since more than one can be selected. > Another idea was to manipulate the password record in treechildren, but it > had no way to get the dom element of each password record. > For example, I would like to get the DOM element at Password column-row[1]. > This approach is another way to control displaying plain text or dots. I don't think that's possible with this type of tree where the display is driven by the data returned by the API calls like getStyleAt(…), etc. > I am not sure that I fully understand your suggestion. Please point it out > if there is something incorrect. > > Thank you. > > [1] > http://searchfox.org/mozilla-central/source/toolkit/content/widgets/tree.xml

Comment on attachment 8760114 [details] Bug 1257078 - Implement a selection feature for showing some specific password.; https://reviewboard.mozilla.org/r/57826/#review78694 ::: toolkit/components/passwordmgr/test/browser/browser_passwordmgr_contextmenu.js:53 (Diff revision 3) > info("Select the first row (with an empty username)"); > selection.select(0); > assertMenuitemEnabled("copyusername", false, "empty username"); > assertMenuitemEnabled("editusername", true); > assertMenuitemEnabled("copypassword", true); > - assertMenuitemEnabled("editpassword", false, "password column hidden"); > + assertMenuitemEnabled("editpassword", true, "password column hidden"); "password column hidden" is stale so please remove that argument

Hey Matthew, The feature and tests are finished. Could you help to review it? Thanks. For "FilterPasswords();" without storing the selection, it can be reproduced in m-c as well.

Comment on attachment 8793168 [details] Bug 1257078 - Remove the redundant spaces in layout/xul/tree/TreeBodyFrame.cpp.; https://reviewboard.mozilla.org/r/79968/#review79348 Please improve the commit message and mention the filename

Comment on attachment 8760114 [details] Bug 1257078 - Implement a selection feature for showing some specific password.; https://reviewboard.mozilla.org/r/57826/#review78792 I tested this patch and thought that the issue with losing the selection was less of an issue than it is. It happens whenever the user toggles the passwords so I don't think the issue is acceptable to ship. Can you fix the selection issue? ::: dom/base/nsGkAtomList.h:1085 (Diff revision 5) > GK_ATOM(resources, "resources") > GK_ATOM(result, "result") > GK_ATOM(resultPrefix, "result-prefix") > GK_ATOM(retargetdocumentfocus, "retargetdocumentfocus") > GK_ATOM(rev, "rev") > +GK_ATOM(revealSelectedPasswords, "revealSelectedPasswords") Sorry for not noticing in my advice earlier but XUL attributes are normally all lowercase ::: layout/xul/tree/nsTreeBodyFrame.cpp:3708 (Diff revision 5) > + nsIContent* baseContent = GetBaseElement(); > + bool revealSelectedPasswords = isSelected && baseContent && > + baseContent->HasAttr(kNameSpaceID_None, nsGkAtoms::revealSelectedPasswords); > + > + if (aColumn->Type() == nsITreeColumn::TYPE_PASSWORD && !revealSelectedPasswords) { > TextEditRules::FillBufWithPWChars(&text, text.Length()); If you change `text.Length()` to 11 this will also fix bug 1209267 which is a blocker to this shipping for security reasons. I would rather we just fix it here now. ::: toolkit/components/passwordmgr/content/passwordManager.js:80 (Diff revision 5) > togglePasswordsButton = document.getElementById("togglePasswords"); > signonsIntro = document.getElementById("signonsIntro"); > removeButton = document.getElementById("removeSignon"); > removeAllButton = document.getElementById("removeAllSignons"); > > + togglePasswordsButton.setAttribute("disabled", "true"); You're already setting this in the markup so is this necessary? ::: toolkit/components/passwordmgr/content/passwordManager.js:445 (Diff revision 5) > - document.getElementById("passwordCol").hidden = !showingPasswords; > + if (!showingPasswords) { > + signonsTree.removeAttribute("revealSelectedPasswords"); > + } else { When you have an `if` and an `else` you should try to avoid the negative by inverting the bodies as its more natural to read: ``` if (showingPasswords) { signonsTree.setAttribute("revealselectedpasswords", "true"); } else { signonsTree.removeAttribute("revealselectedpasswords"); } ``` ::: toolkit/components/passwordmgr/content/passwordManager.xul:86 (Diff revision 5) > data-field-name="username" persist="width"/> > <splitter class="tree-splitter"/> > <treecol id="passwordCol" label="&treehead.password.label;" flex="15" > - ignoreincolumnpicker="true" > + ignoreincolumnpicker="true" type="password" > data-field-name="password" persist="width" > - hidden="true"/> > + hidden="false"/> Can you remove the attribute altogether since we won't toggle it anymore and it would be consistent with siteCol and userCol ::: toolkit/components/passwordmgr/test/browser/browser_passwordmgr_sort.js:95 (Diff revision 5) > + sTree.view.selection.rangedSelect(0, 9, true); > > EventUtils.synthesizeMouse(toggleButton, 1, 1, {}, win); I honestly don't know if selection is synchronous so if this doesn't work it may be async.