Closed Bug 1591505 Opened 7 years ago Closed 6 years ago

Unable to drag and drop (reorder) chat accounts

Categories

(Thunderbird :: Instant Messaging, defect)

Product:
Thunderbird
Component:
Instant Messaging
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(thunderbird_esr6871+ fixed, thunderbird71 fixed, thunderbird72 fixed)

Status:
VERIFIED FIXED
Milestone:
Thunderbird 72.0
Tracking Flags:
Tracking Status
thunderbird_esr68 71+ fixed
thunderbird71 --- fixed
thunderbird72 --- fixed

People

(Reporter: clokep, Assigned: khushil324)

References

(Regression)

Details

(Keywords: regression)

Attachments

(2 files, 3 obsolete files)

Bug-1591505_chat-account-drag-drop.patch
4.45 KB, patch
khushil324
: review+
jorgk-bmo
: approval-comm-beta+
Details | Diff | Splinter Review
Bug-1591505_chat-account-drag-drop.patch
4.47 KB, patch
jorgk-bmo
: approval-comm-esr68+
Details | Diff | Splinter Review

I'm not sure when this broke, but chat accounts can no longer be re-ordered via drag and drop with TB 71b1.

What are the exact STR? Where do you reorder them? We can ask Alice to find the regression.

Ah, I see, you need more than one chat account, go to the chat tab, click on "Show accounts". In TB 60 you can drag them around. Broken in TB 68.

I don't think we need a regression here, that's clearly broken by the de-XBL work.

Flags: needinfo?(khushil324)
Assignee: nobody → khushil324
Flags: needinfo?(khushil324)

(In reply to Jorg K (GMT+2) from comment #2)

Ah, I see, you need more than one chat account, go to the chat tab, click on "Show accounts". In TB 60 you can drag them around. Broken in TB 68.

Exactly. With multiple accounts you should be able to drag and drop them to reorder them.

I don't think we need a regression here, that's clearly broken by the de-XBL work.

Right. I think bug 1531312.

Thanks for taking this Khushil!

Regressed by: 1531312
Version: 71 → 68

This problem is also visible on the trunk.

https://searchfox.org/comm-central/rev/d4b09fef967c58f250556a930881b2df7e85fb14/mail/components/im/content/imAccounts.js#709 - needs to check closest("richlistbox"). I assume it might now be getting some internals?

Keywords: regression

Yes. But there are few other things involved from nsDragAndDrop.js. I am figuring that out. I will submit a patch by today noon. And it's also needs to be checked for ESR 68.

We are able to drag and drop now but the element show in the drop shadow is different. Should we do anything for that?

Attachment #9105818 - Flags: review?(mkmelin+mozilla)

This patch will apply for both, ESR 68 and Trunk.

Status: NEW → ASSIGNED
Comment on attachment 9105818 [details] [diff] [review] Bug-1591505_chat-account-drag-drop.patch Doesn't appear to work for me on linux. Where is one supposed to drop? Looks like only ever the onDragStart is triggered, but never the drop.
Comment on attachment 9105818 [details] [diff] [review] Bug-1591505_chat-account-drag-drop.patch Review of attachment 9105818 [details] [diff] [review]: ----------------------------------------------------------------- Can't get this to work
Attachment #9105818 - Flags: review?(mkmelin+mozilla) → review-
Attachment #9105818 - Attachment is obsolete: true
Attachment #9107302 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9107302 [details] [diff] [review] Bug-1591505_chat-account-drag-drop.patch Review of attachment 9107302 [details] [diff] [review]: ----------------------------------------------------------------- This is kind of working now. I do however get a (non-fatal) error in the native console while I drag stuff around JavaScript error: chrome://messenger/content/chat/imAccounts.js, line 809: TypeError: aEvent.explicitOriginalTarget.closest is not a function I didn't check, but now past XBL, maybe it should be aEvent.target anyways
Attachment #9107302 - Flags: review?(mkmelin+mozilla) → feedback+

Can you review it for ESR 68 also?

Attachment #9107302 - Attachment is obsolete: true
Attachment #9107476 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9107476 [details] [diff] [review] Bug-1591505_chat-account-drag-drop.patch Review of attachment 9107476 [details] [diff] [review]: ----------------------------------------------------------------- r=mkmelin 68 doesn't need an explicit review, only someone to check it applies/works ::: mail/components/im/content/imAccounts.js @@ +719,2 @@ > throw new Error("Element is not draggable!"); > } please remove this if clause. due to the above, it would never happen
Attachment #9107476 - Flags: review?(mkmelin+mozilla) → review+

I'll check it in 68 if you want.

Attachment #9107476 - Attachment is obsolete: true
Attachment #9107602 - Flags: review+
Keywords: checkin-needed-tb

(In reply to Jorg K (GMT+2) from comment #15)

I'll check it in 68 if you want.

Can you check it for ESR 68 if it is working or not.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/160cac84b99c
Fixed drag and drop (reorder) of chat accounts. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed-tb
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 72.0
Comment on attachment 9107602 [details] [diff] [review] Bug-1591505_chat-account-drag-drop.patch Thanks. Working fine on trunk. I'll check 68 unless you beat me to it: Download a working version of TB 68. Rename omni.ja to omni.zip, unpack it. Tweak the file in question. Run it.
Attachment #9107602 - Flags: approval-comm-esr68+
Attachment #9107602 - Flags: approval-comm-beta+
Attachment #9107602 - Flags: approval-comm-esr68+

This is working. Needed a bit of rebasing.

Attachment #9107662 - Flags: approval-comm-esr68+

Working in my TB 68 build.

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: