Closed Bug 1113681 Opened 5 years ago Closed 5 years ago

Weird behavior when dragging search engines by the checkbox


(Firefox :: Search, defect)

35 Branch
Not set



Firefox 38


(Reporter: FlorinMezei, Assigned: abdelrahman, Mentored)




(1 file, 1 obsolete file)

Reproducible with: 
- Firefox 35 Beta 5 - BuildID: 20141218174327
- Firefox 36 Aurora latest - BuildID: 20141219004003
- Firefox 37 Nightly latest - BuildID: 20141218030202

Steps to reproduce:
1. Open Firefox.
2. Click the search magnifying glass in the search field and select "Change Search Settings".
3. In the list of one-click search engines, drag an engine by its name into a different position in the list.
4. In the same list of search engines attempt to drag providers by their checkbox.

Expected results:
Drag is not supported and the only action performed is select/deselect.
Drag is supported and is performed correctly (but then there is no select/deselect action also recorded in this case).

Actual results:
Both the drag and the selection is performed, but the result of the drag action is that the search engine in step #3 is actually moved to the new position. See

Note that if step #3 is skipped, then only the selection action is performed (no drag action is performed).
I think we should just prevent dragging from the checkboxes.

Returning early in this function will prevent the drag operation.

The event parameter is a drag event, documented at

There's a code sample here showing how to convert the clientX/clientY coordinates into a row and column:
Mentor: florian

I added isCheckBox method to EngineView in this patch instead of editing isSelectable and use it because it may be used for another thing.
Attachment #8553850 - Flags: review?(florian)
Comment on attachment 8553850 [details] [diff] [review]
rev 1 - prevent dragging engines from checkbox

Review of attachment 8553850 [details] [diff] [review]:

Looks good, could you please make the same change to the non-in-content version of the file?

::: browser/components/preferences/in-content/search.js
@@ +487,5 @@
>    cycleHeader: function(column) { },
>    selectionChanged: function() { },
>    cycleCell: function(row, column) { },
>    isEditable: function(index, column) { return != "engineName"; },
> +  isCheckBox: function(index, column) { return == "engineShown"; },

There's an "  // nsITreeView" comment above this whole section of the code that indicates this part of the code only implements the nsITreeView interface. Could you please add a comment above isCheckBox indicating that it's just a helper, and not part of nsITreeView?
Attachment #8553850 - Flags: review?(florian) → feedback+
Assignee: nobody → a.ahmed1026
I've moved isCheckBox to Helpers section "// Helpers" instead of adding comment in nsITreeView section.
Attachment #8553850 - Attachment is obsolete: true
Attachment #8556318 - Flags: review?(florian)
Comment on attachment 8556318 [details] [diff] [review]
rev 2 - prevent dragging engines from checkbox

Review of attachment 8556318 [details] [diff] [review]:

Attachment #8556318 - Flags: review?(florian) → review+
sorry had to back this out for merge conflicts down from m-c in  browser_searchbar_openpopup.js
Whiteboard: [fixed-in-fx-team]
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
> -  if (selectedIndex >= 0) {
> +  var tree = document.getElementById("engineList");
> +  var row = { }, col = { }, child = { };
> +  tree.treeBoxObject.getCellAt(event.clientX, event.clientY, row, col, child);
> +  if (selectedIndex >= 0 && !gEngineView.isCheckBox(row.value, col.value)) {

Yuck. Please use the newer and better API:

var cell = tbo.getCellAt(aEvent.clientX, aEvent.clientY);
if (selectedIndex >= 0 && !gEngineView.isCheckBox(cell.row, cell.col)) {

> There's a code sample here showing how to convert the clientX/clientY
> coordinates into a row and column:
Somebody please update that example?
(In reply to Philip Chee from comment #10)

If you think something needs to be improved, please file a new bug, thanks.
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.