Closed Bug 1547947 Opened 1 year ago Closed 1 year ago

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

Categories

(Thunderbird :: General, task)

task
Not set

Tracking

(thunderbird68 fixed, thunderbird69 fixed)

RESOLVED FIXED
Thunderbird 69.0
Tracking Status
thunderbird68 --- fixed
thunderbird69 --- fixed

People

(Reporter: mkmelin, Assigned: aleca)

References

Details

Attachments

(1 file, 3 obsolete files)

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.

Assignee: nobody → alessandro
Blocks: 1533360
Attached patch dexbl-attachmentitem.patch (obsolete) — Splinter Review
Attachment #9069529 - Flags: review?(mkmelin+mozilla)
Status: NEW → ASSIGNED
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?
Attached patch dexbl-attachmentitem.patch (obsolete) — Splinter Review

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

Attachment #9069529 - Attachment is obsolete: true
Attachment #9069529 - Flags: review?(mkmelin+mozilla)
Attachment #9069780 - Flags: review?(mkmelin+mozilla)
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?
Attachment #9069780 - Flags: review?(mkmelin+mozilla) → review+

(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.

(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 👍

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

Flags: needinfo?(alessandro)
Keywords: checkin-needed
Attached patch dexbl-attachmentitem.patch (obsolete) — Splinter Review

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.

Attachment #9069780 - Attachment is obsolete: true
Flags: needinfo?(alessandro)

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.

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

Attachment #9070473 - Attachment is obsolete: true
Attachment #9071454 - Flags: review+
Keywords: checkin-needed
Target Milestone: --- → Thunderbird 69.0

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

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Keywords: checkin-needed
Resolution: --- → FIXED

This will be uplifted to next beta?

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

(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.

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.

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.

Attachment #9071454 - Flags: approval-comm-beta?
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.
Attachment #9071454 - Flags: approval-comm-beta? → approval-comm-beta+
Regressions: 1562200
You need to log in before you can comment on or make changes to this bug.