1113581 - Artifact when editing a keyword, the keyword is displayed under the text-area

Status: VERIFIED FIXED

(Firefox :: Search, defect, P4)

Description

[Petruta Horea [:phorea], Desktop QA]

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)

Comment 2

[Fischer [:Fischer]]

Attachment: 1113581-keyword-displayed-when-editing.png

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

Comment 3

[Fischer [:Fischer]]

Attachment: keyword-covered.png

Comment 4

[Fischer [:Fischer]]

Attachment: Bug 1113581 - Artifact when editing a keyword, the keyword is displayed under the text-area.

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

Comment 5

[Fischer [:Fischer]]

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 6

[Jared Wein [:jaws] (please needinfo? me)]

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 7

[Fischer [:Fischer]]

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 8

[Fischer [:Fischer]]

Attachment: Compensation_Issue.png

Comment 9

[Fischer [:Fischer]]

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 10

[Jared Wein [:jaws] (please needinfo? me)]

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 :)

Comment 11

[Florian Quèze [:florian]]

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.

Comment 12

[Fischer [:Fischer]]

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

Comment 13

[Fischer [:Fischer]]

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 14

[Fischer [:Fischer]]

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.

Comment 15

[Florian Quèze [:florian]]

https://reviewboard.mozilla.org/r/66384/#review64778

Looks good to me, thanks!

Comment 16

[Fischer [:Fischer]]

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

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/08a557e99638
Artifact when editing a keyword, the keyword is displayed under the text-area. r=jaws

Comment 18

[Carsten Book [:Tomcat]]

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

Comment 19

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/907d353fc1c3
Backed out changeset 08a557e99638 for breaking bc4 tests

Comment 20

[Fischer [:Fischer]]

Attachment: TreeView_Issue.png

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 21

[Fischer [:Fischer]]

Attachment: Original_and_Compensated.png

Comment 22

[Fischer [:Fischer]]

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 23

[Fischer [:Fischer]]

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 24

[Jared Wein [:jaws] (please needinfo? me)]

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 25

[Fischer [:Fischer]]

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/4-5/

Comment 26

[Fischer [:Fischer]]

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.

Comment 27

[Fischer [:Fischer]]

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/5-6/

Comment 28

[Fischer [:Fischer]]

@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 29

[Jared Wein [:jaws] (please needinfo? me)]

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>

Comment 30

[Fischer [:Fischer]]

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

Comment 31

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 32

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

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

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/d75e08f55916
Artifact when editing a keyword, the keyword is displayed under the text-area. r=jaws

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d75e08f55916

Comment 35

[Mohammad Maruf Islam]

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

Comment 36

[Jared Wein [:jaws] (please needinfo? me)]

Thanks!