1547947 - [de-xbl] convert the attachmentitem binding to <richlistitem is="attachment-item">

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Magnus Melin [:mkmelin]]

Convert the attachmentitem binding to <richlistitem is="attachment-item">

https://searchfox.org/comm-central/rev/e1fe9fabd33e870f1471a889d27f8652641660e0/mail/base/content/mailWidgets.xml#384

Should be done after bug 1523607 is done.

Comment 1

[Magnus Melin [:mkmelin]]

Actually seems there is very little use and could just inline it in https://searchfox.org/comm-central/rev/947d80f302ed9b1a1360d01996bd7fb0d8281580/mail/base/content/mailWidgets.xml#182

Comment 2

[Alessandro Castellani [:aleca]]

Attachment: dexbl-attachmentitem.patch

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 9069529 [details] [diff] [review]
dexbl-attachmentitem.patch

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

::: mail/base/content/mailWidgets.js
@@ +1364,5 @@
> +    });
> +
> +    let container = this.ownerDocument.createXULElement("hbox");
> +    container.setAttribute("flex", "1");
> +    container.classList.add("attachmentcell-content");

do we need this inner hbox? can't the attachmentItem richlistitem in it self work as the box?

Comment 4

[Alessandro Castellani [:aleca]]

Attachment: dexbl-attachmentitem.patch

You're right, that container wasn't necessary.

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 9069780 [details] [diff] [review]
dexbl-attachmentitem.patch

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

r=mkmelin

::: mail/base/content/attachmentList.css
@@ +67,5 @@
>  }
> +
> +.attachmentItem > * {
> +  pointer-events: none;
> +}

is this needed?

Comment 6

[Alessandro Castellani [:aleca]]

(In reply to Magnus Melin [:mkmelin] from comment #5)

::: mail/base/content/attachmentList.css
@@ +67,5 @@

}

.attachmentItem > * {
pointer-events: none;
}

is this needed?

Yes, otherwise the double click event can randomly come from one of the internal elements (text, icon, etc.)
This prevents event propagation to child elements and guarantee that the even is triggered only by the expected target.

Comment 7

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=25d5ebb60386dcd2221496ea893f748e3f837653
to avoid surprises before landing this, some other stuff also in the try.

Comment 8

[Alessandro Castellani [:aleca]]

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

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=25d5ebb60386dcd2221496ea893f748e3f837653
to avoid surprises before landing this, some other stuff also in the try.

Thanks for taking care of the push to try 👍

Comment 9

[Jorg K (CEST = GMT+2)]

Yes, this makes
mozmake SOLO_TEST=attachment/test-attachment-menus.js mozmill-one
fail big time :-(

Comment 10

[Alessandro Castellani [:aleca]]

Attachment: dexbl-attachmentitem.patch

Thanks for the feedback Jorgk.

This updated patch fixes the issue for the test-attachment-size.js tests, but still, the test-attachment-menus.js tests keep failing.
The failures are all related to the attachmentItemContext and attachmentListContext popup menu not showing up, which is driving me crazy because those context menus appear when I interact with the attachment list.

Anyway, I'll keep troubleshooting this section and try to understand why it fails.

Comment 11

[Jorg K (CEST = GMT+2)]

You might want to also enable the four tests from bug 1555912 and see what's happening to them since their failure comes from bug 1512432.

Comment 12

[Alessandro Castellani [:aleca]]

Attachment: dexbl-attachmentitem.patch

I fixed the failing tests and enabled the four tests from bug 1555912.
Here's the try-push: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=24579e4ff10e50a534897ecdac260bcee42d33ed

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0f52c1933b47
[de-xbl] convert attachmentitem binding to richlistitem. r=mkmelin

Comment 14

[Wayne Mery (:wsmwk)]

This will be uplifted to next beta?

Comment 15

[Jorg K (CEST = GMT+2)]

Why would/should it be uplifted? De-XBL changes are not backported.

Comment 16

[Wayne Mery (:wsmwk)]

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

Why would/should it be uplifted? De-XBL changes are not backported.

Because according to Magnus this blocks bug 1533360 - which we want fixed in 68. Beyond that, I don't know - I don't follow the de-xbl effort and don't even know its target version.

Comment 17

[Jorg K (CEST = GMT+2)]

Well, you have a point. If we need these de-XBL changes to fix a11y issues, we might backport them. That needs to be done with caution. And first we need to prove that the a11y issue is actually fixed on trunk.

For the record, I've spent most of last week trying do untangle de-XBL/XBL issues to get chat working semi-properly in TB 68 and ended up returning everything to the original XBL code via backouts.

Comment 18

[Magnus Melin [:mkmelin]]

Yeah we should uplift this one so the attachment area is xbl free. Too much oddness debugging the mix and the potentially breaking it again.

Comment 19

[Jorg K (CEST = GMT+2)]

Comment on attachment 9071454 [details] [diff] [review]
dexbl-attachmentitem.patch

Sure, seems to fix bug 1533360. Patch almost applies to C-B, I can fix the conflict myself.

Comment 20

[Jorg K (CEST = GMT+2)]

TB 68 beta 2:
https://hg.mozilla.org/releases/comm-beta/rev/72c5f4b9c7201d1f96a330f8703032e9961d8265