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

VERIFIED FIXED in Firefox 38

Status

()

VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: cbadau, Assigned: deepakkoli93, Mentored)

Tracking

33 Branch
Firefox 39
Points:
---
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox38 verified, firefox39 verified)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 attachment, 5 obsolete attachments)

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.

Updated

4 years ago
Flags: firefox-backlog+
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
Mentor: manishearth
Whiteboard: [good first bug][lang=js]
note: replacing the onclick handler with oncommand *might* work too, try it out!
Assigning to Deepak from IRC.
Assignee: nobody → deepakkoli93
Status: NEW → ASSIGNED
(Assignee)

Comment 4

4 years ago
Created attachment 8535997 [details] [diff] [review]
patch

The oncommand event is not firing so had to check which button was clicked
Attachment #8535997 - Flags: review?(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)
Attachment #8535997 - Flags: review?(manishearth) → feedback+
(Assignee)

Comment 6

4 years ago
Created attachment 8538488 [details] [diff] [review]
patch

Added the fix for Allowed Sites - Pop-ups
Attachment #8538488 - Flags: review?(manishearth)
Attachment #8538488 - Attachment is patch: true
Attachment #8538488 - Attachment mime type: text/x-patch → text/plain
Attachment #8535997 - Attachment is obsolete: true
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!
Attachment #8538488 - Flags: review?(manishearth)
Attachment #8538488 - Flags: review?(jaws)
Attachment #8538488 - Flags: feedback+
(Assignee)

Comment 8

4 years ago
Created attachment 8538501 [details] [diff] [review]
patch1
Attachment #8538488 - Attachment is obsolete: true
Attachment #8538488 - Flags: review?(jaws)
Attachment #8538501 - Flags: review?(gavin.sharp)
Attachment #8538501 - Flags: review?(gavin.sharp) → review?(jaws)
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.
Attachment #8538501 - Flags: review?(jaws) → feedback+
(Assignee)

Comment 10

4 years ago
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
Flags: needinfo?(jaws)
(Assignee)

Comment 11

4 years ago
Created attachment 8539393 [details] [diff] [review]
patch3.patch

Applied the method suggested by jaws. The issue that i mentioned in my previous comment has been resolved.
Attachment #8538501 - Attachment is obsolete: true
Attachment #8539393 - Flags: review?(jaws)
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.
Attachment #8539393 - Flags: review?(jaws) → feedback+
(Assignee)

Comment 13

4 years ago
Created attachment 8539448 [details] [diff] [review]
patch4.patch

Made the changes suggested by jaws.
Attachment #8539393 - Attachment is obsolete: true
Attachment #8539448 - Flags: review?(jaws)
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.
Attachment #8539448 - Flags: review?(jaws) → feedback+
(Assignee)

Comment 15

4 years ago
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'));
  });
});
Flags: needinfo?(jaws)
Flags: needinfo?(MattN+bmo)
(Assignee)

Comment 16

4 years ago
Created attachment 8544374 [details] [diff] [review]
1039540.patch

Hi, 
i've tried to follow the approach suggested by Matt. Can someone review the patch.
Attachment #8539448 - Attachment is obsolete: true
Attachment #8544374 - Flags: review?(jaws)
Attachment #8544374 - Flags: review?(MattN+bmo)
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)
Attachment #8544374 - Flags: review?(jaws)
Attachment #8544374 - Flags: review?(MattN+bmo)
Attachment #8544374 - Flags: feedback+
Flags: needinfo?(jaws)
Flags: needinfo?(MattN+bmo)
https://hg.mozilla.org/integration/fx-team/rev/817390369f57
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][fixed-in-fx-team]
Matt, can you request uplift on this patch for Aurora 38?
status-firefox38: --- → affected
Flags: needinfo?(MattN+bmo)
https://hg.mozilla.org/mozilla-central/rev/817390369f57
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][fixed-in-fx-team] → [good first bug][lang=js]
Target Milestone: --- → Firefox 39

Updated

3 years ago
Iteration: --- → 39.3 - 30 Mar
Flags: qe-verify?
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
Flags: needinfo?(MattN+bmo)
Attachment #8544374 - Flags: approval-mozilla-aurora?
Flags: qe-verify? → qe-verify+
QA Contact: camelia.badau
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.
I think the patch didn't address that one.
Camelia, can you report that as a separate bug? Thanks!
Flags: needinfo?(camelia.badau)
Matt am I understanding rightly that this should stand as fixed/verified and that comment 22 should be reported as a different issue?
Flags: needinfo?(MattN+bmo)
(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.
Flags: needinfo?(MattN+bmo)
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.
Flags: needinfo?(sledru)
Flags: needinfo?(MattN+bmo)
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.
Flags: needinfo?(MattN+bmo)
(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.
Status: RESOLVED → VERIFIED
status-firefox39: fixed → verified
Flags: needinfo?(camelia.badau)
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.
Flags: needinfo?(sledru)
Attachment #8544374 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
Verified fixed on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.9.5 using Firefox 38 Beta 4 (buildID: 20150413143743).
status-firefox38: fixed → verified
You need to log in before you can comment on or make changes to this bug.