Closed Bug 1526811 Opened 6 years ago Closed 6 years ago

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

Categories

(Thunderbird :: Message Compose Window, defect)

Product:
Thunderbird
Component:
Message Compose Window
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(thunderbird67 fixed, thunderbird68 fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 68.0
Tracking Flags:
Tracking Status
thunderbird67 --- fixed
thunderbird68 --- fixed

People

(Reporter: Paenglab, Assigned: jorgk-bmo)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

1526811-fix-attachments.patch (v2)
(deleted), patch
mkmelin
: review+
mkmelin
: feedback+
jorgk-bmo
: approval-comm-beta+
Details | Diff | Splinter Review

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

Blocks: 1519346
Summary: Navigating through the attachments in the composers attachment list no more possible → Navigating through the attachments in the composers attachment bucket no more possible

Which version?

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

de-XBL issue?

Flags: needinfo?(mkmelin+mozilla)

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

Flags: needinfo?(mkmelin+mozilla)
Keywords: regression
Version: unspecified → 65

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.

Attached patch 1526811-fix-attachments.patch (obsolete) (deleted) — — Splinter Review

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.

Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #9055340 - Flags: review?(acelists)
Attachment #9055340 - Flags: feedback?(richard.marti)
Attachment #9055340 - Flags: feedback?(mkmelin+mozilla)
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 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.
Attachment #9055340 - Flags: review?(acelists) → review-

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()?

How about this then?

Attachment #9055340 - Attachment is obsolete: true
Attachment #9055340 - Flags: feedback?(richard.marti)
Attachment #9055340 - Flags: feedback?(mkmelin+mozilla)
Attachment #9055353 - Flags: feedback?(mkmelin+mozilla)
Attachment #9055353 - Flags: feedback?(acelists)
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
Attachment #9055353 - Flags: feedback?(mkmelin+mozilla)
Attachment #9055353 - Flags: feedback?(acelists)
Attachment #9055353 - Flags: feedback+
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 :-(
Attachment #9055353 - Flags: review?(mkmelin+mozilla)
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 :-(
Attachment #9055353 - Flags: review?(mkmelin+mozilla)

Wow, how did this happen?

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.
Attachment #9055353 - Flags: review?(mkmelin+mozilla)
Attachment #9055353 - Flags: review+

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

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED

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

Target Milestone: --- → Thunderbird 68.0
Attachment #9055353 - Flags: approval-comm-beta+

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.

Flags: needinfo?(jorgk)

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.

Flags: needinfo?(jorgk)

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.

Depends on: 1542317

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

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: