1113681 - Weird behavior when dragging search engines by the checkbox

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

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.
OR
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 http://www.screencast.com/t/qDCeOuDxS9Q.

Note that if step #3 is skipped, then only the selection action is performed (no drag action is performed).

Comment 1

[Florian Quèze [:florian]]

I think we should just prevent dragging from the checkboxes.

Returning early in this function http://hg.mozilla.org/mozilla-central/annotate/3d846527576f/browser/components/preferences/in-content/search.js#l192 will prevent the drag operation.

The event parameter is a drag event, documented at https://developer.mozilla.org/en-US/docs/Web/Events/dragstart

There's a code sample here showing how to convert the clientX/clientY coordinates into a row and column:
https://developer.mozilla.org/en-US/Add-ons/Code_snippets/Tree#Getting_the_cell_from_a_mouse_click

Comment 2

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 1 - prevent dragging engines from checkbox

Hello,

I added isCheckBox method to EngineView in this patch instead of editing isSelectable and use it because it may be used for another thing.

Comment 3

[Florian Quèze [: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 column.id != "engineName"; },
> +  isCheckBox: function(index, column) { return column.id == "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?

Comment 4

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 2 - prevent dragging engines from checkbox

I've moved isCheckBox to Helpers section "// Helpers" instead of adding comment in nsITreeView section.

Comment 5

[Florian Quèze [:florian]]

Comment on attachment 8556318 [details] [diff] [review]
rev 2 - prevent dragging engines from checkbox

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

Thanks!

Comment 6

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/65ca9f64dd0a

Comment 7

[Carsten Book [:Tomcat]]

sorry had to back this out for merge conflicts down from m-c in  browser_searchbar_openpopup.js

Comment 8

[Carsten Book [:Tomcat]]

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

Comment 9

[Carsten Book [:Tomcat]]

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

Comment 10

[Philip Chee]

> -  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:
> https://developer.mozilla.org/en-US/Add-ons/Code_snippets/Tree#Getting_the_cell_from_a_mouse_click
Somebody please update that example?

Comment 11

[Florian Quèze [:florian]]

(In reply to Philip Chee from comment #10)

If you think something needs to be improved, please file a new bug, thanks.

Comment 12

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/projects/cypress/rev/e11d6c1705f9