Reproduced with Nightly 37.0a1 2014-12-18 under Win 8.1 32-bit, Ubuntu 12.04 LTS 32-bit and Mac OSX 10.9.5. Steps to reproduce: 1. Double click on the keyword cell associated to a search engine and add a keyword (eg yh for yahoo) 2. Click outside the textarea or press Enter 3. Click again on the previously added keyword to edit it Expected results: The keyword can be edited - no artifacts Actual results: The text below the text area is visible - screenshot: http://i.imgur.com/T5H7P3J.png Note: 1. It is easier to notice when zooming in the page. 2. It is not reproducible when Preferences/Search is opened in separate window (browser.preferences.inContent is set to false)

Upload the issue screenshot to Bugzilla from http://i.imgur.com/T5H7P3J.png in case of being deleted.

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

Comment on attachment 8773702 [details] Bug 1113581 - Artifact when editing a keyword, the keyword is displayed under the text-area. Change the review request to the feedback request because of uncertain of who is the reviewer. ---------------------------------------------- @Jared, Could you please have a look at this patch or forward it to the right reviewer ? We need to make some compensation so as to let the input filed cover the whole text underneath. I considered and tested the case that the keyword area is full, displaying like "kkkkkkkkkkkkkk...". Please see the attached keyword-covered.png. Thanks

Comment on attachment 8773702 [details] Bug 1113581 - Artifact when editing a keyword, the keyword is displayed under the text-area. https://reviewboard.mozilla.org/r/66384/#review63304 ::: toolkit/content/widgets/tree.xml:372 (Diff revision 1) > + input.left = cellRect.x - 1; > + input.width = cellRect.width + textRect.x - cellRect.x + 4; Where does the -1 and +4 come from? Are those dependent on the theme or platform? If they come from the theme, please find a way to read the values from the theme so this code will adapt with any theme changes.

Comment on attachment 8773702 [details] Bug 1113581 - Artifact when editing a keyword, the keyword is displayed under the text-area. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66384/diff/1-2/

Comment on attachment 8773702 [details] Bug 1113581 - Artifact when editing a keyword, the keyword is displayed under the text-area. @Jared, It is about the platform. Please see the attached Compensation_Issue.png for the details. I think a better way is to hide the text when editing, then we don’t have to deal with these “px” stuff. However, I cannot find a way to hide the text after looking into [1] & [2]. Also the original codes use the input to cover the text, so I guess it might be unable to hide the text in our situation. There is another approach. We could delete the keyword text when editing and save it somewhere to restore it later. In this approach, we don’t have to worry about the length of the text underneath. I have tested and updated the patch. How do you think of this approach ? Or please let me know if there is a way to hide the text underneath. Thank you [1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsITreeBoxObject [2] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsITreeView#getCellText%28%29

Comment on attachment 8773702 [details] Bug 1113581 - Artifact when editing a keyword, the keyword is displayed under the text-area. https://reviewboard.mozilla.org/r/66384/#review64200 Yes, this is a much better approach. Looks good :)

https://reviewboard.mozilla.org/r/66384/#review64208 ::: toolkit/content/widgets/tree.xml:384 (Diff revision 2) > this._editingColumn = column; > > + // Clear the keyword because we don't want the text appearing underneath the input. > + this.view.setCellText(this._editingRow, this._editingColumn, ""); > + // Save the original keyword so we can restore it after stoping editing. > + input.setAttribute("data-original-keyword", input.value); tree.xml is a generic binding, the word 'keyword' here seems specific to the usage we are making of the tree in the search panel, I don't think it should appear here. ::: toolkit/content/widgets/tree.xml:404 (Diff revision 2) > var input = this.inputField; > var editingRow = this._editingRow; > var editingColumn = this._editingColumn; > this._editingRow = -1; > this._editingColumn = null; > + var value = input.getAttribute("data-original-keyword"); This value isn't needed if 'accept' is true, so I think the getAttribute call should be in an 'else' block. ::: toolkit/content/widgets/tree.xml:412 (Diff revision 2) > } > + this.view.setCellText(editingRow, editingColumn, value); > > input.hidden = true; > input.value = ""; > this.removeAttribute("editing"); It seems we should remove the 'data-original-keyword' attribute too.

Make no longer block 718011 since it is not about moving into in-content and already being checked by 1271779.

Comment on attachment 8773702 [details] Bug 1113581 - Artifact when editing a keyword, the keyword is displayed under the text-area. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66384/diff/2-3/

Comment on attachment 8773702 [details] Bug 1113581 - Artifact when editing a keyword, the keyword is displayed under the text-area. @Jared, Based on Comment 11 by Florian, the patch has been updated a little bit. Could you please have a look? Thank you.

https://reviewboard.mozilla.org/r/66384/#review64778 Looks good to me, thanks!

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2325d3b09712&selectedJob=24688554

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/fx-team/rev/08a557e99638 Artifact when editing a keyword, the keyword is displayed under the text-area. r=jaws

backed out for causing failures like https://treeherder.mozilla.org/logviewer.html#?job_id=10856857&repo=fx-team#L2266

Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/fx-team/rev/907d353fc1c3 Backed out changeset 08a557e99638 for breaking bc4 tests

For the test issue in [1], it is that when calling the view's setCellText method would trigger the select event and then stopEditing method under bookmark dialog window. As a result, it would be unable to rename newly created folder. However, under the Search Preference page, the select event wouldn't be trigger. See TreeView_Issue.png for the details. [1] browser/components/places/tests/browser/browser_bookmarksProperties.js

Comment on attachment 8773702 [details] Bug 1113581 - Artifact when editing a keyword, the keyword is displayed under the text-area. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66384/diff/3-4/

Comment on attachment 8773702 [details] Bug 1113581 - Artifact when editing a keyword, the keyword is displayed under the text-area. @Jared, For this bug, the situation is kind of complicated. The solution of calling this.view.setCellText to clear temporarily in Comment 9 would bring issue to the bookmark dialog window. Please see Comment 20 for the details. I traced the code a bit. - When startEditing, the setCellText method is at [1]. - Then PlacesUtils.bookmarks.setItemTitle is called at [2] - and then nsNavBookmarks::SetItemTitle is called at [3] to set bookmark folder title. - During this call process, the tree select event would be triggered at [4], - then the stopEditing would be called at [5]. As a result, it wouldn’t be able to rename bookmark folder when creating one new (startEditng then stopEditing immediately). I am not sure if it is a good direction to take on why the tree select event is invoked inside the bookmark dialog window but not invoked inside the Search Preference panel. Since this is a long call process and could have its reason. So I am thinking that keep using the input field to cover the text underneath. However, we need some compensation. Please see the attached Compensation_Issue.png for the details. I update the patch for this compensation. Please see Original_and_Compensated.png for the result. As you can see from Original_and_Compensated.png, the input field inside bookmark dialog window is different (Occupying the entire space) in RTL. Since this difference exists already and no matter in which case, I propose we could focus on resolve the Search Preference Panel first. And open another bug to deal with it. With the updated patch, the failure in Comment 18 no longer exists. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f990b51350d8 How do you think ? BTW, for the difference, there could be 2 approaches. (1) Do extra calculation so the input field wouldn’t occupy the entire space and wouldn’t cover the folder icon. However, just tried to look into the folder icon rect info in RTL, I still cannot figure out it. The info returned by box.getCoordsForCellItem(row, column, "image"); seems unable to locate the icon’s actual position. (2) Make input field occupy entire space both in RTL and LTR. Consider nested folders case, the width of input field is squeezed. When editing, maybe it is better to have longer space so could see clear what we are typing. Thanks [1] https://dxr.mozilla.org/mozilla-central/source/browser/components/places/content/treeView.js#1686 [2] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/PlacesUtils.jsm#3089 [3] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavBookmarks.cpp#1492 [4] https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/tree.xml#725 [5] https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/tree.xml#399

Comment on attachment 8773702 [details] Bug 1113581 - Artifact when editing a keyword, the keyword is displayed under the text-area. https://reviewboard.mozilla.org/r/66384/#review67290 ::: toolkit/content/widgets/tree.xml (Diff revisions 3 - 4) > - // Clear the text because we don't want the text appearing underneath the input. > - this.view.setCellText(this._editingRow, this._editingColumn, ""); > - // Save the original text so we can restore it after stoping editing. > - input.setAttribute("data-original-text", input.value); > - > this.setAttribute("editing", "true"); It would be much better if we can get this version of the patch to work. Do we need to move the 'setCellText' call up earlier in this method? Does some of the code need to be moved in to the selectText timeout earlier?

Comment on attachment 8773702 [details] Bug 1113581 - Artifact when editing a keyword, the keyword is displayed under the text-area. Cancel the auto-generated review request.

@Jared, After looking into places/content/treeView.js, it seems that many operations would invoke select action then select event. Therefore, this patch respects that select event and like your comment #24, the patch put the 'setCellText' call before setting the '_editingColumn'. This is because 'stopEditing' would check the '_editingColumn' then do actions. The local tests are passed and the try is here : https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c95ade9cadd Thank you.

Comment on attachment 8773702 [details] Bug 1113581 - Artifact when editing a keyword, the keyword is displayed under the text-area. https://reviewboard.mozilla.org/r/66384/#review72138 ::: toolkit/content/widgets/tree.xml:91 (Diff revisions 4 - 6) > # builderView is obsolete (see bug 202393) > <property name="builderView" > onget="return this.view; /*.QueryInterface(Components.interfaces.nsIXULTreeBuilder)*/" > readonly="true"/> > <field name="pageUpOrDownMovesSelection"> > - { > + !/Mac/.test(navigator.platform) It would be better to use AppConstants here and other places that we want to check what the platform is. This is because AppConstants can keep this type of logic in one place and allow for it to change in the future if necessary without us having to find similar code throughout the project. ::: toolkit/content/widgets/tree.xml:671 (Diff revisions 4 - 6) > this.stopEditing(true); > this.focus(); > return true; > } > > - let { AppConstants } = > + if (/Mac/.test(navigator.platform)) { Same comment here. ::: toolkit/content/widgets/tree.xml (Diff revisions 4 - 6) > <handler event="click" button="0" phase="target"> > <![CDATA[ > if (event.target != event.originalTarget) > return; > > - let { AppConstants } = You can keep a reference to AppConstants in a property of the binding so that it doesn't need to be duplicated everywhere. <property name="appConstants" readonly="true"> <getter><![CDATA[ let {AppConstants} = Components.utils.import("resource://gre/modules/AppConstants.jsm", {}); return AppConstants; ]]></getter> </property>

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #29) Hi Jared, For the below feedbacks, do you mean that update the patch for AppConstants btw ? Thanks > Comment on attachment 8773702 [details] > Bug 1113581 - Artifact when editing a keyword, the keyword is displayed > under the text-area. > > https://reviewboard.mozilla.org/r/66384/#review72138 > > ::: toolkit/content/widgets/tree.xml:91 > (Diff revisions 4 - 6) > > # builderView is obsolete (see bug 202393) > > <property name="builderView" > > onget="return this.view; /*.QueryInterface(Components.interfaces.nsIXULTreeBuilder)*/" > > readonly="true"/> > > <field name="pageUpOrDownMovesSelection"> > > - { > > + !/Mac/.test(navigator.platform) > > It would be better to use AppConstants here and other places that we want to > check what the platform is. > > This is because AppConstants can keep this type of logic in one place and > allow for it to change in the future if necessary without us having to find > similar code throughout the project. > > ::: toolkit/content/widgets/tree.xml:671 > (Diff revisions 4 - 6) > > this.stopEditing(true); > > this.focus(); > > return true; > > } > > > > - let { AppConstants } = > > + if (/Mac/.test(navigator.platform)) { > > Same comment here. > > ::: toolkit/content/widgets/tree.xml > (Diff revisions 4 - 6) > > <handler event="click" button="0" phase="target"> > > <![CDATA[ > > if (event.target != event.originalTarget) > > return; > > > > - let { AppConstants } = > > You can keep a reference to AppConstants in a property of the binding so > that it doesn't need to be duplicated everywhere. > > <property name="appConstants" readonly="true"> > <getter><![CDATA[ > let {AppConstants} = > Components.utils.import("resource://gre/modules/AppConstants.jsm", {}); > return AppConstants; > ]]></getter> > </property>

Yeah, I was saying it would be better to use AppConstants.jsm here but I do see that tree.xml is already using this style so you don't have to switch to AppConstants.jsm.

For future reference, commit messages should summarize what the patch is doing rather than restating the problem being solved. Thanks! http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html#write-detailed-commit-messages

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/fx-team/rev/d75e08f55916 Artifact when editing a keyword, the keyword is displayed under the text-area. r=jaws

I have reproduced this bug with Firefox Nightly 37.0a1 (Build ID: 20141219030202) on Windows 8.1, 64-bit. Verified as fixed with Latest Firefox Nightly 51.0a1 (Build ID: 20160902030222) Mozilla/5.0 (Windows NT 6.3; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0

Thanks!