1039540 - In-content preferences: The rows of sub-dialogs are sorted when right clicking on them

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Camelia Badau [:cbadau], Desktop QA]

This bug was filled based on the discussion from bug 1037081 comment 8.

Reproducible on the latest Nightly (BuildID: 20140715030207)
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0

Steps to reproduce: 
1. Launch Firefox.
2. Open about:preferences. 
3. Open any "Exceptions" or "Saved Passwords" sub-dialog. 
4. Right click on any sort preferences (e.g: Site, Status, Username). 

Actual results: The rows are sorted when right clicking on them. 
e.g: under Security tab -> "Saved Passwords" sub dialog -> if I perform right click on any sort preferences (e.g: site, username), the Context Menu (with "Copy Username", "Copy Password" options) is displayed, but the rows are also sorted

Expected results: No sort is performed when right clicking on the sort preferences(The Context Menu is displayed - where is the case).  

Notes: This issue is reproducible also on Mac OSX 10.9.2 and Ubuntu 13.10 32bit.

Comment 1

[Manish Goregaokar [:manishearth]]

For the password manager exceptions, the dialog is here[1], and the javascript is here[2]

we need to tweak the event handler to not capture right click events (tweak the function to take an event parameter and check event.button).

I'll add links to the other locations for this bug if you need them, though it might be a fun exercise to look for them yourself! 


Let me know if you need any more information for this.


 [1]: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/content/passwordManagerExceptions.xul
 [2]: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/content/passwordManagerExceptions.js

Comment 2

[Manish Goregaokar [:manishearth]]

note: replacing the onclick handler with oncommand *might* work too, try it out!

Comment 3

[Manish Goregaokar [:manishearth]]

Assigning to Deepak from IRC.

Comment 4

[Deepak]

Attachment: patch

The oncommand event is not firing so had to check which button was clicked

Comment 5

[Manish Goregaokar [:manishearth]]

Comment on attachment 8535997 [details] [diff] [review]
patch

Review of attachment 8535997 [details] [diff] [review]:
-----------------------------------------------------------------

This is a good start!

There are a couple of other pages which need the fix. There's the "Allowed Sites - Popups" and "Allowed Sites - Addons" pages (Preferences -> Content and Preferences -> Security)

There's also the offline exceptions page under Preferences -> Advanced -> Network and a whole bunch of them under Preferences -> Advanced -> Certificates.


Let me know if you need help finding these (you can use http://dxr.mozilla.org/mozilla-central/ for code search)

Comment 6

[Deepak]

Attachment: patch

Added the fix for Allowed Sites - Pop-ups

Comment 7

[Manish Goregaokar [:manishearth]]

Comment on attachment 8538488 [details] [diff] [review]
patch

Review of attachment 8538488 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! We just need a commit message as explained in IRC (and review from someone with review powers), and we should be good to go!

Comment 8

[Deepak]

Attachment: patch1

Comment 9

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

Comment on attachment 8538501 [details] [diff] [review]
patch1

Review of attachment 8538501 [details] [diff] [review]:
-----------------------------------------------------------------

This works, but I think we can do better :)

::: browser/components/preferences/permissions.xul
@@ +53,5 @@
>            onkeypress="gPermissionManager.onPermissionKeyPress(event)"
>            onselect="gPermissionManager.onPermissionSelected();">
>        <treecols>
>          <treecol id="siteCol" label="&treehead.sitename.label;" flex="3"
> +                onclick="if(event.button == 0) gPermissionManager.onPermissionSort('rawHost');" persist="width"/>

We should be working towards reducing inline script like this. You can see bug 956482 for an example.

Instead of adding to this inline script, let's do the following:

1) In permissions.js, add an event listener for DOMContentLoaded, and within the event listener, add event listeners for both of these columns.

For example,
document.addEventListener("domcontentloaded", function onDOMContentLoaded() {
  function sortTreeColumn(aEvent, aFunc, aColumn) {
    if (aEvent.button != 0)
      return;
    aFunc(aColumn);
  }
  let siteColumn = document.getElementById("siteCol");
  siteColumn.addEventListener("onclick", event => {
    sortTreeColumn(event, gPermissionManager.onPermissionSort, 'rawHost');
  });
  let statusColumn = document.getElementById("statusCol");
  statusColumn.addEventListener("onclick", event => {
    sortTreeColumn(event, gPermissionManager.onPermissionSort, 'capability');
  });
});

2) Do the same thing for both passwordManager.xul/passwordManager.js and passwordManagerExceptions.xul/passwordManagerExceptions.js.

Comment 10

[Deepak]

can we do the following?

document.addEventListener("DOMContentLoaded", function onDOMContentLoaded() {
  function sortTreeColumn(aEvent, aColumn) {
    if (aEvent.button != 0)
      return;
    gPermissionManager.onPermissionSort(aColumn);
  }
  let siteColumn = document.getElementById("siteCol");
  siteColumn.addEventListener("click", event =>{
    sortTreeColumn(event, 'rawHost');
  });
  let statusColumn = document.getElementById("statusCol");
  statusColumn.addEventListener("click", event =>{
    sortTreeColumn(event, 'capability');
  });
});

Passing gPermissionManager.onPermissionSort inside the sortTreeColumn function is giving the following error:
JavaScript error: chrome://global/content/treeUtils.js, line 58: TypeError: aDataSet is undefined

However, there is no such problem in passwordManager.js and passwordManagerExceptions.js

Comment 11

[Deepak]

Attachment: patch3.patch

Applied the method suggested by jaws. The issue that i mentioned in my previous comment has been resolved.

Comment 12

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

Comment on attachment 8539393 [details] [diff] [review]
patch3.patch

Review of attachment 8539393 [details] [diff] [review]:
-----------------------------------------------------------------

Overall this looks good. Just a couple cosmetic changes and then we should be all set.

::: browser/components/preferences/permissions.js
@@ +304,5 @@
>    },
>  
>    
>    onPermissionSort: function (aColumn)
> +  { 

nit, you can remove this space character that got inserted.

@@ +371,5 @@
>    gPermissionManager.init(aParams);
>  }
> +
> +document.addEventListener("DOMContentLoaded", function onDOMContentLoaded() {
> +  function sortTreeColumn(aEvent, aFunc, aColumn) {

Hm, after more thought I think this name could better describe what this function is doing. Maybe we should rename it to:
`function callFunctionIfLeftClick(aEvent, aFunc, aFunctionArg)`

::: browser/components/preferences/permissions.xul
@@ +53,5 @@
>            onkeypress="gPermissionManager.onPermissionKeyPress(event)"
>            onselect="gPermissionManager.onPermissionSelected();">
>        <treecols>
>          <treecol id="siteCol" label="&treehead.sitename.label;" flex="3"
> +                persist="width"/>

nit, please align the 'persist' with 'id' above it (add another space before 'persist'). Here and below.

::: toolkit/components/passwordmgr/content/passwordManagerExceptions.js
@@ +119,5 @@
> +    aFunc(aColumn);
> +  }
> +  let rejectColumn = document.getElementById("rejectCol");
> +  rejectColumn.addEventListener("click", event =>{
> +    sortTreeColumn(event, RejectColumnSort, 'host');

Since this is the only consumer of this function in this file, we can get rid of the extra 'sortTreeColumn' function and move the contents of that function in to this function body.

Comment 13

[Deepak]

Attachment: patch4.patch

Made the changes suggested by jaws.

Comment 14

[Matthew N. [:MattN]]

Comment on attachment 8539448 [details] [diff] [review]
patch4.patch

Review of attachment 8539448 [details] [diff] [review]:
-----------------------------------------------------------------

Hello, I have some suggestions to make the patch simpler.

::: browser/components/preferences/permissions.js
@@ +370,5 @@
>  {
>    gPermissionManager.init(aParams);
>  }
> +
> +document.addEventListener("DOMContentLoaded", function onDOMContentLoaded() {

Same suggestion as passwordManager.js/xul

::: toolkit/components/passwordmgr/content/passwordManager.js
@@ +412,5 @@
> +  siteColumn.addEventListener("click", event =>{
> +    callFunctionIfLeftClick(event, SignonColumnSort, 'hostname');
> +  });
> +  let userColumn = document.getElementById("userCol");
> +  userColumn.addEventListener("click", event =>{

Please use one event listener on <treecols> instead and check the target is a <treecol> and then check the button. Then you don't need a helper. You may even be able to add the method using <treecols onclick="…"> too for consistency.

::: toolkit/components/passwordmgr/content/passwordManager.xul
@@ +62,5 @@
>            onselect="SignonSelected();"
>            context="signonsTreeContextMenu">
>        <treecols>
>          <treecol id="siteCol" label="&treehead.site.label;" flex="40"
> +                 persist="width"

I think you could do something like:
dataField="hostname" persist="width"

and then use the @dataField attribute for a click handler on <treecols> when the event.target is a <treecol>.

Keeping the name of the field on in the markup makes it easier for someone to map columns to the underlying data.

::: toolkit/components/passwordmgr/content/passwordManagerExceptions.xul
@@ -29,5 @@
>                onkeypress="HandleRejectKeyPress(event)"
>                onselect="RejectSelected();">
>            <treecols>
>              <treecol id="rejectCol" label="&treehead.site.label;" flex="5"
> -                     onclick="RejectColumnSort('host');" sortDirection="ascending"/>

I would recommend either doing the same as my suggestion for passwordManager.xul or just passing event as a second argument to this method as an optional argument and checking the button.

Comment 15

[Deepak]

Should i continue with the following approach for passwordManager.xul/js or are some other changes required to the following?

/browser/components/preferences/permissions.xul

<treecols>
        <treecol id="siteCol" label="&treehead.sitename.label;" flex="3"
                 dataField="rawHost" persist="width"/>
        <splitter class="tree-splitter"/>
        <treecol id="statusCol" label="&treehead.status.label;" flex="1"
                 dataField="capability" persist="width"/>
</treecols>

/browser/components/preferences/permissions.js

document.addEventListener("DOMContentLoaded", function onDOMContentLoaded(){
let treeColumns = document.getElementsByTagName("treecols");
 treeColumns[0].addEventListener("click", event =>{
  if((event.target.nodeName == "treecol") && (event.button == 0))
    gPermissionManager.onPermissionSort(event.target.getAttribute('dataField'));
  });
});

Comment 16

[Deepak]

Attachment: 1039540.patch

Hi, 
i've tried to follow the approach suggested by Matt. Can someone review the patch.

Comment 17

[Matthew N. [:MattN]]

Comment on attachment 8544374 [details] [diff] [review]
1039540.patch

Review of attachment 8544374 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for cleaning this up! :)

This is getting better but I'd like to take another look since there are still a few things which can improve.

::: browser/components/preferences/permissions.js
@@ +372,5 @@
>  }
> +
> +document.addEventListener("DOMContentLoaded", function onDOMContentLoaded(){
> +let treeColumns = document.getElementsByTagName("treecols");
> + treeColumns[0].addEventListener("click", event =>{

You can move this into gPermissionManager.init. The notes from  passwordManagerExceptions.js also apply here.

::: toolkit/components/passwordmgr/content/passwordManager.js
@@ +402,5 @@
>    return token.isLoggedIn();
>  }
> +
> +document.addEventListener("DOMContentLoaded", function onDOMContentLoaded(){
> +let treeColumns = document.getElementsByTagName("treecols");

You can put the body of this function in SignonsStartup instead

@@ +405,5 @@
> +document.addEventListener("DOMContentLoaded", function onDOMContentLoaded(){
> +let treeColumns = document.getElementsByTagName("treecols");
> + treeColumns[0].addEventListener("click", event =>{
> +  if((event.target.nodeName == "treecol") && (event.button == 0))
> +    SignonColumnSort(event.target.getAttribute('dataField'));

See my notes on passwordManagerExceptions.js below as the same things apply

::: toolkit/components/passwordmgr/content/passwordManagerExceptions.js
@@ +111,5 @@
>    sortedCol.setAttribute("sortDirection", lastRejectSortAscending ?
>                                            "ascending" : "descending");
>  }
> +
> +document.addEventListener("DOMContentLoaded", function onDOMContentLoaded(){

No need for a new event listener when we already have a function running upon load named "RejectsStartup" (see line 9).

@@ +113,5 @@
>  }
> +
> +document.addEventListener("DOMContentLoaded", function onDOMContentLoaded(){
> +let treeColumns = document.getElementsByTagName("treecols");
> + treeColumns[0].addEventListener("click", event =>{

Nit: the two lines above have incorrect indentation
Nit: you're missing a space after `=>`

@@ +115,5 @@
> +document.addEventListener("DOMContentLoaded", function onDOMContentLoaded(){
> +let treeColumns = document.getElementsByTagName("treecols");
> + treeColumns[0].addEventListener("click", event =>{
> +  if((event.target.nodeName == "treecol") && (event.button == 0))
> +    RejectColumnSort(event.target.getAttribute('dataField'));

It's preferred to do an early return in cases like this. The style guide mentions errors but it applies in this case too: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Return_from_errors_immediately

The extra brackets around the 2 conditions aren't necessary either:

if (event.target.nodeName != "treecol" || event.button != 0) {
  return;
}
let sortField = event.target.getAttribute("dataField")
if (!sortField) {
  return;
}
RejectColumnSort(sortField);

Also note that I switched to double-quotes above.

I think you should also return if there is no dataField attribute. Sorry to do this but I think we should rename the attribute to data-field-name so it's clear that they aren't attributes that trees implement. https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Using_data_attributes (Note that the dataset getter doesn't work in XUL so continue to use getAttribute)

Also note that it would be nice to share the click handler function in passwordManagerCommon.js so it doesn't need to be duplicated since the only difference is the sort function (which you can bind as the first argument to the shared function)

Comment 18

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/817390369f57

Comment 19

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

Matt, can you request uplift on this patch for Aurora 38?

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/817390369f57

Comment 21

[Matthew N. [:MattN]]

Comment on attachment 8544374 [details] [diff] [review]
1039540.patch

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #19)
> Matt, can you request uplift on this patch for Aurora 38?

OK, it doesn't seem like something that's important IMO since it's not a regression AFAIK.

Approval Request Comment
[Feature/regressing bug #]: In-content preferences
[User impact if declined]: Right-clicking on a heading of some tree views will do a sort when not expected.
[Describe test coverage new/current, TreeHerder]: No tests for this change or related code.
[Risks and why]: Low-Med change to how clicks on column headings work to make the code cleaner and centralized.
[String/UUID change made/needed]: None

Comment 22

[Camelia Badau [:cbadau], Desktop QA]

Verified on Windows 7 64bit, Mac OSX 10.9.5 and Ubuntu 12.04 32bit using latest Aurora 39.0a2 (buildID:  	20150401004004) and only one mention should be done here: under Applications, the rows (Content Type and Action) are sorted when right clicking on them - is this intended behaviour? For the rest of the sub-dialogs, it is ok: the rows aren't sorted when right clicking on them.

Comment 23

[Manish Goregaokar [:manishearth]]

I think the patch didn't address that one.

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

Camelia, can you report that as a separate bug? Thanks!

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

Matt am I understanding rightly that this should stand as fixed/verified and that comment 22 should be reported as a different issue?

Comment 26

[Matthew N. [:MattN]]

(In reply to Liz Henry (:lizzard) from comment #25)
> Matt am I understanding rightly that this should stand as fixed/verified and
> that comment 22 should be reported as a different issue?

Yes, that's correct. This bug wasn't about the Applications pane.

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

Can you also nom this for beta approval? 38 is beta now. 
Sylvestre, it sounds like in-content preferences is lining up to be released with 38.

Comment 28

[Matthew N. [:MattN]]

I was waiting for baking on m-a and as I said in comment 21, I don't think it's that important to uplift.

Comment 29

[Camelia Badau [:cbadau], Desktop QA]

(In reply to Liz Henry (:lizzard) from comment #24)
> Camelia, can you report that as a separate bug? Thanks!

I filled bug 1150377. 
Marking this bug as VERIFIED FIXED.

Comment 30

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8544374 [details] [diff] [review]
1039540.patch

Merge happened. 
It is a new feature, we can polish it a bit more since we are early in the beta cycle.

Should be in 38 beta 2.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a9d533ac9ff4

Comment 32

[Camelia Badau [:cbadau], Desktop QA]

Verified fixed on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.9.5 using Firefox 38 Beta 4 (buildID: 20150413143743).