[de-xbl] convert the attachmentitem binding to <richlistitem is="attachment-item">
Categories
(Thunderbird :: General, task)
Tracking
(thunderbird68 fixed, thunderbird69 fixed)
People
(Reporter: mkmelin, Assigned: aleca)
References
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
aleca
:
review+
jorgk-bmo
:
approval-comm-beta+
|
Details | Diff | Splinter Review |
Convert the attachmentitem binding to <richlistitem is="attachment-item">
Should be done after bug 1523607 is done.
Reporter | ||
Comment 1•5 years ago
|
||
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
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Reporter | ||
Comment 3•5 years ago
|
||
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?
Assignee | ||
Comment 4•5 years ago
|
||
You're right, that container wasn't necessary.
Reporter | ||
Comment 5•5 years ago
|
||
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?
Assignee | ||
Comment 6•5 years ago
|
||
(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.
Updated•5 years ago
|
Comment 7•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=25d5ebb60386dcd2221496ea893f748e3f837653
to avoid surprises before landing this, some other stuff also in the try.
Assignee | ||
Comment 8•5 years ago
|
||
(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•5 years ago
|
||
Yes, this makes
mozmake SOLO_TEST=attachment/test-attachment-menus.js mozmill-one
fail big time :-(
Assignee | ||
Comment 10•5 years ago
|
||
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•5 years ago
|
||
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.
Assignee | ||
Comment 12•5 years ago
|
||
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
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Comment 13•5 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0f52c1933b47
[de-xbl] convert attachmentitem binding to richlistitem. r=mkmelin
Comment 16•5 years ago
|
||
(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•5 years ago
|
||
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.
Reporter | ||
Comment 18•5 years ago
|
||
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.
Reporter | ||
Updated•5 years ago
|
Comment 19•5 years ago
|
||
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.
Updated•5 years ago
|
Comment 20•5 years ago
|
||
TB 68 beta 2:
https://hg.mozilla.org/releases/comm-beta/rev/72c5f4b9c7201d1f96a330f8703032e9961d8265
Description
•