Weird behavior when dragging search engines by the checkbox

RESOLVED FIXED in Firefox 38

Status

()

Firefox
Search
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: FlorinMezei, Assigned: abdelrhman, Mentored)

Tracking

35 Branch
Firefox 38
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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).
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
Mentor: florian@queze.net
(Assignee)

Comment 2

3 years ago
Created attachment 8553850 [details] [diff] [review]
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.
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 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?
Attachment #8553850 - Flags: review?(florian) → feedback+
Assignee: nobody → a.ahmed1026
(Assignee)

Comment 4

3 years ago
Created attachment 8556318 [details] [diff] [review]
rev 2 - prevent dragging engines from checkbox

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

Thanks!
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]
https://hg.mozilla.org/mozilla-central/rev/d4459f047dc6
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38

Comment 10

3 years ago
> -  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?
(In reply to Philip Chee from comment #10)

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

Updated

3 years ago
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.