1526811 - Navigating through the attachments in the composers attachment bucket no more possible

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Richard Marti (:Paenglab)]

It's no more possible to navigate with the cursor keys through the attachments in the composer's attachment bucket.

Comment 1

[Jorg K (CEST = GMT+2)]

Which version?

Comment 2

[Richard Marti (:Paenglab)]

Beta 66 and trunk. Not checked but I think it was in 65 also.

Comment 3

[Jorg K (CEST = GMT+2)]

de-XBL issue?

Comment 4

[Magnus Melin [:mkmelin]]

Sounds likely. That's using xbl-richlistbox. Arshad is converting those atm in bug 1523607. Let's check once that's done.

Comment 6

[Jorg K (CEST = GMT+2)]

From the duplicate (bug 1541205) I've just filed: I get this in the error console:
JavaScript error: chrome://messenger/content/mailWidgets.xml, line 150: TypeError: box is null

Also, when the focus is on the body and I press Alt+M to focus the attachment bucket I get:
JavaScript error: chrome://messenger/content/richlistbox.xml, line 857: TypeError: this.currentItem._fireEvent is not a function.

Comment 7

[Alice0775 White]

From Bug 1541205.

Regression window:
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=92f2f9d3268cf833b2860a0cdf5a443e6e5562d8&tochange=992cb1d8b84f15a340dc72c3a7f61eb4da31e945
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=19fc512f3c95b2f24edefbf6ee87a7bf63e618d1&tochange=32810619d6b3fb498cba043bc19da2c62c878845

Regressed by:
992cb1d8b84f Richard Marti — Bug 1516813 - Port Bug 1472557: Remove the "scrollbox" anonymous element from "richlistbox". r=jorgk DONTBUILD

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: 1526811-fix-attachments.patch

OK, the scrollbox hunk is just a porting error. The _fireEvent() function doesn't exist any more, so we might as well remove it. At least it restores the functionality for now. I can't see any negative effects.

Comment 9

[:aceman]

Comment on attachment 9055340 [details] [diff] [review]
1526811-fix-attachments.patch

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

There are more of these box = this.scrollbox in mailWidgets.xml...
There is also <property name="scrollbox"> in that file that may be unneeded now.
And there is also <xul:scrollbox> inside <binding id="attachmentlist-horizontal"> while we removed that from <binding id="attachmentlist-vertical"> in bug 1516813.

Comment 10

[:aceman]

Comment on attachment 9055340 [details] [diff] [review]
1526811-fix-attachments.patch

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

And I still get that "box is null" error from the other places in the file, e.g. after a drop of a file into the bucket and moving with arrow keys.

Comment 11

[Jorg K (CEST = GMT+2)]

Ah well, maybe the de-XBL guys want to take over. At least we know this is fixable without 1523607 and we can fix TB 67 beta as well.

The main point is here: What about _fireEvent()?

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: 1526811-fix-attachments.patch (v2)

How about this then?

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 9055353 [details] [diff] [review]
1526811-fix-attachments.patch (v2)

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

I suppose, though this code is going away real soon now, so I wouldn't put much energy into it

Comment 14

[Jorg K (CEST = GMT+2)]

Comment on attachment 9055353 [details] [diff] [review]
1526811-fix-attachments.patch (v2)

Can I get an r+ here? As you know, this is broken on beta. Have you tried it in detail? I just followed Aceman's suggestions at 1:30 AM :-(

Comment 15

[Jorg K (CEST = GMT+2)]

Comment on attachment 9055353 [details] [diff] [review]
1526811-fix-attachments.patch (v2)

Can I get an r+ here? As you know, this is broken on beta. Have you tried it in detail? I just followed Aceman's suggestions at 1:30 AM :-(

Comment 16

[Jorg K (CEST = GMT+2)]

Wow, how did this happen?

Comment 17

[Magnus Melin [:mkmelin]]

Comment on attachment 9055353 [details] [diff] [review]
1526811-fix-attachments.patch (v2)

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

Sure, I think I ticked the wrong box.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/66cc9a7a0aaa
Port more of bug 1472557 to fix attachment navigation. r=mkmelin DONTBUILD

Comment 19

[Jorg K (CEST = GMT+2)]

Thanks again, Alice, your help was very valuable, as always!!

Comment 20

[alta88]

I'm pretty sure this regressed the attachment pane so that if there are numerous items that would create multiple rows, they are now all on one row with a long scrollbar, don't wrap, and the height cannot be sized with the splitter. preferredHeight is probably wrong.

Comment 21

[Jorg K (CEST = GMT+2)]

Actually no, when we landed it, it was all fine. With this M-C changeset: Thu Apr 04 07:34:14 2019 +0300, it's all fine locally. Please file a new bug and CC me, Magnus, Richard and Aceman.

Comment 22

[Jorg K (CEST = GMT+2)]

Hmm, my apologies, looks like I was wrong. I got confused about the attachment pane in the composer and the preview pane. More in bug 1542317.

Comment 23

[Jorg K (CEST = GMT+2)]

TB 67 beta 2:
https://hg.mozilla.org/releases/comm-beta/rev/e0ec15fa6c0626752a5ff03d6fdf2424f7d753fc

Comment 24

[Jean-Philippe MENGUAL]

Hi,

For a blind user, this bug is still blocked by Bug 1533360. Because with tthis bug, even if the crt moves and it is good because now I can remove again the attached file via del key, the screen canno follow the movement. An additional comment was added by Joanmarie on the bug.

Regards