[de-xbl] Remove attachment-list-base and its consumers' binding.
Categories
(Thunderbird :: Mail Window Front End, task)
Tracking
(Not tracked)
People
(Reporter: arshad, Assigned: arshad)
References
Details
Attachments
(1 file, 18 obsolete files)
(deleted),
patch
|
arshad
:
review+
|
Details | Diff | Splinter Review |
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 1•6 years ago
|
||
Assignee | ||
Comment 2•6 years ago
|
||
Magnus, I went with the approach of using is
attr that we discussed in last meeting. I am getting a NotSupportedError: Operation is not supported
from the line customElements.define("attachmentlist", MozAttachmentlist, { extends: "richlistbox" });
. This is strange and I am not sure why it is happening. I am attaching the patch which combines the attachmentlist-vertical && attachmentlist-horizontal into single ce and depends upon orient attr to determine the case.
Assignee | ||
Comment 4•6 years ago
|
||
Comment on attachment 9042713 [details] [diff] [review] attachmentlist-elems_(NSError).patch Review of attachment 9042713 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/mailWidgets.js @@ +674,5 @@ > + for (let child of this._childNodes) { child.width = width + border; } > + } > +} > + > +customElements.define("attachmentlist", MozAttachmentlist, { extends: "richlistbox" }); This line throws NotSupportedOperation Error.
Assignee | ||
Comment 5•6 years ago
|
||
Comment 6•6 years ago
|
||
Comment on attachment 9042713 [details] [diff] [review] attachmentlist-elems_(NSError).patch Review of attachment 9042713 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/mailWidgets.js @@ +312,5 @@ > +// <richlistbox> is lazily set up through setElementCreationCallback, > +// i.e. put into customElements the first time it's really seen. > +// Create a fake to ensure richlistbox exists in customElements, since otherwise > +// we can't extend it. Then make sure this fake doesn't stay around. > +if (!customElements.get("richlistbox")) { This shouldn't be necessary anymore, as richlistbox.js is now loaded eagerly: https://searchfox.org/mozilla-central/rev/43bf9e7c8acf042617a1ebc5d2ce540d78f0ce91/toolkit/content/customElements.js#389 @@ +674,5 @@ > + for (let child of this._childNodes) { child.width = width + border; } > + } > +} > + > +customElements.define("attachmentlist", MozAttachmentlist, { extends: "richlistbox" }); Can you try putting a dash in the name? I know we loosened the restriction for dashes for xul elements in general, but maybe that's not extending to customized built-ins?
Assignee | ||
Comment 7•6 years ago
|
||
Can you try putting a dash in the name? I know we loosened the restriction
for dashes for xul elements in general, but maybe that's not extending to
customized built-ins?
Yes, it was the dash.
Assignee | ||
Comment 8•6 years ago
|
||
The only issue with this patch is the removal of selected attribute(https://searchfox.org/comm-central/source/mozilla/toolkit/content/widgets/richlistbox.xml#83) by this line(https://searchfox.org/comm-central/source/mozilla/toolkit/content/widgets/richlistbox.js#485). click(https://searchfox.org/comm-central/source/mozilla/toolkit/content/widgets/richlistbox.js#878) handler is called after mousedown event. Ideally it should not remove the current selected item but something is going wrong here. Other than that, the patch seems to be working fine for vertical and horizontal attachmentList.
Assignee | ||
Comment 9•6 years ago
|
||
Comment on attachment 9044660 [details] [diff] [review] attachmentlist-elems.patch Review of attachment 9044660 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/mailWidgets.js @@ +771,5 @@ > + item.setAttribute("context", this.getAttribute("itemcontext")); > + item.attachment = attachment; > + > + if (this.orient === "horizontal") { > + this.attachmentListWrapper.insertBefore(item, this.getItemAtIndex(index)); This makes sure that richlistitem elements are appended inside scrollbox>hbox. This tries to cover up for the childrent tag functionality in the xbl binding.
Assignee | ||
Comment 10•6 years ago
|
||
Assignee | ||
Comment 11•6 years ago
|
||
Assignee | ||
Comment 12•6 years ago
|
||
Comment on attachment 9044873 [details] [diff] [review] attachmentlist-elems.patch There are a few other issues with this patch. The attachmentList element doesn't get focus event due to which there are differences in the UI when richlistitem is selected/clicked. Paenglab, could you please take a look why the attachmentList is not getting focussed? If you open an email having some attachment, and then if you click on any of the attachment, normally the attachmentItem should have a white dotted outline and hightlight background, but only when attachmentList is in focus. Not sure why attachmentItem is not getting the focussed.
Comment 13•6 years ago
|
||
Sorry, I know too less about the CE. What I can say is, that the richlistitems don't stay selected="true" and that now in the message pane's attachmentlist the attachmentlist-wrapper is now a for the DOM visible children of the richlistbox. This makes selectors like richlistbox > richlistitem no more working, they'd need now to be richlistbox > attachmentlist-wrapper > richlistitem. But this can not be the solution because we'd need to double all rules of richlistbox.css and other files for this list.
Assignee | ||
Comment 14•6 years ago
|
||
Paenglab, mkmelin, some of the keyboard shortcuts are broken on the trunk as well.
Comment 15•6 years ago
|
||
Comment on attachment 9044982 [details] [diff] [review] attachmentlist-elems.patch When you click on a attachmentitem and then use the cursor keys, the attachmentitems keep the selected state. ui-r- because the attachmentitems in the message pane's don't have a focus selected state. This is because the toolkit richlist.css still has the sibling selector ( > ): richlistbox:focus > richlistitem... Isn't it possible to make the richlistbox orient=horizontal without the attachmentlist-wrapper? Then we don't need this box.
Assignee | ||
Comment 16•6 years ago
|
||
(In reply to Richard Marti (:Paenglab) from comment #15)
Comment on attachment 9044982 [details] [diff] [review]
attachmentlist-elems.patchWhen you click on a attachmentitem and then use the cursor keys, the
attachmentitems keep the selected state.ui-r- because the attachmentitems in the message pane's don't have a focus
selected state. This is because the toolkit richlist.css still has the
sibling selector ( > ): richlistbox:focus > richlistitem...Isn't it possible to make the richlistbox orient=horizontal without the
attachmentlist-wrapper? Then we don't need this box.
hmm why was scrollbox their in the first place?
Comment 17•6 years ago
|
||
Comment on attachment 9044982 [details] [diff] [review] attachmentlist-elems.patch Review of attachment 9044982 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/mailWidgets.js @@ +486,5 @@ > customElements.define("mail-emailheaderfield", MozMailEmailheaderfield); > + > +/** > + * Attachmentlist class extends richlistbox. This component shows the attachemnts attached > + * to a mail in horizontal layout and in vertical layout, while sending an email to someone. typo. perhaps /** * The MozAttachmentlist widget lists attachments for a mail. This is typically * used to show attachments while writing a new mail as well as when reading * mails. * It has two layouts, which you can set by orient="horizontal" and * orient="vertical" respectively. */ @@ +493,5 @@ > +class MozAttachmentlist extends MozElements.RichListBox { > + constructor() { > + super(); > + > + const isNoModifierKeyPressed = event => ( As I wrote, I think you should just use event.getModifierState() But please don't pass any parameter to that, as that's deprecated @@ +504,5 @@ > + > + /** > + * The spacebar should work just like the arrow keys, except that the > + * focused element doesn't change, so use moveByOffset here. > + */ please use // style for these internal comments @@ +542,5 @@ > + } > + }); > + > + /** > + * make sure we keep the focus... Capitalize. Also make this comment // style @@ +620,5 @@ > + child => child.setAttribute("context", > + this.getAttribute("itemcontext") > + ) > + ); > + lots of lines here. children.filter(child => !child.hasAttribute("context")).forEach( child => child.setAttribute("context", this.getAttribute("itemcontext")); ); ::: mail/base/content/msgAttachmentView.inc.xul @@ +100,5 @@ > defaultset="attachmentSaveAll"/> > </toolbox> > </hbox> > <richlistbox id="attachmentList" > + is="attachment-list" please put this like this: <richlistbox is="attachment-list" id="attachmentList" ::: mail/components/compose/content/messengercompose.xul @@ +2019,5 @@ > context="" > oncommand="attachmentBucketCloseButtonOnCommand();"/> > </hbox> > <richlistbox id="attachmentBucket" > + is="attachment-list" same here
Comment 19•6 years ago
|
||
I notice I was wrong about getModifierState, must have thought about something else.
Comment 20•6 years ago
|
||
for the "any", at least as used in this code the intention probably never was that this would require no other keys pressed, so I think we can just simplify and handle the key straight off
Assignee | ||
Comment 21•6 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #20)
for the "any", at least as used in this code the intention probably never was that this would require no other keys pressed, so I think we can just simplify and handle the key straight off
Did you mean that I should not test the noModifierKeyPressed state?
Assignee | ||
Comment 24•6 years ago
|
||
Hopefully this will add hightlight background for linux.
Comment 26•6 years ago
|
||
Comment on attachment 9045280 [details] [diff] [review] attachmentlist-elems.patch Yes, this looks now good.
Assignee | ||
Comment 27•6 years ago
|
||
Magnus, we can work on the selected attr issue later because that's something where we need fx guys help. Once this lands I ll open a new bug for the selected attr issue.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 28•6 years ago
|
||
Comment on attachment 9045280 [details] [diff] [review] attachmentlist-elems.patch changin the review to mkmelin as this is mail stuff
Comment 29•6 years ago
|
||
Comment on attachment 9045280 [details] [diff] [review] attachmentlist-elems.patch Review of attachment 9045280 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/mailWidgets.js @@ +579,5 @@ > + }); > + } > + } > + > + connectedCallback() { you're not calling super.connectedCallback() here, which is also using delayConnectedCallback() which the child presumably also should use then. https://searchfox.org/comm-central/source/mozilla/toolkit/content/widgets/richlistbox.js#147 @@ +601,5 @@ > + <scrollbox flex="1" style="overflow: auto;"> > + <hbox flex="1" class="attachmentlist-wrapper"> > + </hbox> > + </scrollbox> > + `)); didn't try this yet, but looks wrong to have this here? how would the children even end up inside the wrapper?
Assignee | ||
Comment 30•6 years ago
|
||
Comment on attachment 9045280 [details] [diff] [review] attachmentlist-elems.patch Review of attachment 9045280 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/mailWidgets.js @@ +778,5 @@ > + item.setAttribute("context", this.getAttribute("itemcontext")); > + item.attachment = attachment; > + > + if (this.orient === "horizontal") { > + this.attachmentListWrapper.insertBefore(item, this.getItemAtIndex(index)); element will added like this. ::: mail/base/content/msgHdrView.js @@ +2632,5 @@ > var list = document.getElementById("attachmentList"); > list.clearSelection(); > > + while (list.attachmentListWrapper.hasChildNodes()) > + list.attachmentListWrapper.lastChild.remove(); See this.
Assignee | ||
Comment 31•6 years ago
|
||
Assignee | ||
Comment 32•6 years ago
|
||
Comment 33•6 years ago
|
||
Comment on attachment 9048031 [details] [diff] [review] attachmentlist-elems.patch Review of attachment 9048031 [details] [diff] [review]: ----------------------------------------------------------------- Write a new message, add two attachments, hit DOWN. I get JavaScript error: chrome://messenger/content/mailWidgets.js, line 864: TypeError: box is null (which is in the added code). The selection is also a mess. Once I click one of the attachments, the selection doesn't stick. ::: mail/base/content/mailWidgets.js @@ +644,5 @@ > + event.preventDefault(); > + }); > + > + this.addEventListener("keypress", (event) => { > + if (event.keyCode != KeyEvent.DOM_VK_RETURN) { please use a switch for the keypress handling, instead of 5 listeners @@ +728,5 @@ > + children > + .filter(child => !child.hasAttribute("context")) > + .forEach( > + child => child.setAttribute("context", this.getAttribute("itemcontext") > + )); indention off here. shouls align with .forEach @@ +874,5 @@ > + > + scrollToIndex(index) { > + let box = this.scrollbox; > + let item = this.getItemAtIndex(index); > + if (!item) { return; } please put return on it's own line @@ +885,5 @@ > + } > + > + insertItemAt(index, attachment, name) { > + const XULNS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"; > + let item = this.ownerDocument.createElementNS(XULNS, "richlistitem"); just use this.ownerDocument.createXULElement @@ +965,5 @@ > + > + if (this._childNodes.length == 0) { > + return 0; > + } else if (this._childNodes.length <= itemsPerRow) { > + return 1; no else after return @@ +978,5 @@ > + _setImageSize() { > + let size = this.sizes[this.view] || 16; > + > + for (let i = 0; i < this._childNodes.length; i++) { this._childNodes[i].imageSize = size; } > + } expand to 3 lines @@ +991,5 @@ > + let border = this._childNodes[0].boxObject.width - > + this._childNodes[0].clientWidth; > + > + for (let child of this._childNodes) { width = Math.max(width, child.scrollWidth); } > + for (let child of this._childNodes) { child.width = width + border; } these too ::: mail/base/content/msgAttachmentView.inc.xul @@ +100,5 @@ > defaultset="attachmentSaveAll"/> > </toolbox> > </hbox> > + <richlistbox is="attachment-list" > + id="attachmentList" please move it to <richlistbox is="attachment-list" id="attachmentList" And the same thing elsewhere (is and id on the same line) ::: mail/base/content/msgHdrView.js @@ +2625,5 @@ > var list = document.getElementById("attachmentList"); > list.clearSelection(); > > + while (list.attachmentListWrapper.hasChildNodes()) > + list.attachmentListWrapper.lastChild.remove(); please add braces for this loop
Assignee | ||
Comment 34•6 years ago
|
||
The selection is also a mess. Once I click one of the attachments, the selection doesn't stick.
This is the issue that I want to tackle in next bug.
Assignee | ||
Comment 35•6 years ago
|
||
Write a new message, add two attachments, hit DOWN. I get JavaScript error: chrome://messenger/content/mailWidgets.js, line 864: TypeError: box is null
(which is in the added code).
If you se the console without this patch, you will find the same error their with xbl implementation. The reason is vertical orient attachmentList dont have scrollbox.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 36•6 years ago
|
||
Assignee | ||
Comment 37•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Comment 39•6 years ago
|
||
Comment on attachment 9049262 [details] [diff] [review] attachmentlist-elems.patch Review of attachment 9049262 [details] [diff] [review]: ----------------------------------------------------------------- The focus bug is pretty bad, but alright, we can do that in a follow-up. You didn't address the review comment about eventlisteners. Please just attach one of each kind, then use a switch. Perhaps also move attaching them to the connectedCallback() the connectedCallback() also needs this.hasChildNodes() bailing ::: mail/base/content/mailWidgets.js @@ +982,5 @@ > + /** > + * Only used by attachmentlist with horizontal orient. > + */ > + setOptimumWidth() { > + if (this._childNodes.length == 0) { return; } please put this on three rows
Assignee | ||
Comment 40•6 years ago
|
||
Comment 41•6 years ago
|
||
Comment on attachment 9050162 [details] [diff] [review] attachmentlist-elems.patch Review of attachment 9050162 [details] [diff] [review]: ----------------------------------------------------------------- r=mkmelin with the comments below fixed. And do file the followup for focus problem ::: mail/base/content/mailWidgets.js @@ +650,5 @@ > + * > + * @extends {MozElements.RichListBox} > + */ > + > +class MozAttachmentlist extends MozElements.RichListBox { nit: no empty line between documentation and class @@ +725,5 @@ > + return; > + } > + > + this.sizes = { small: 16, large: 32, tile: 32 }; > + this.messenger = Cc["@mozilla.org/messenger;1"].createInstance(Ci.nsIMessenger); messenger needs to be set up in the constructor, otherwise someone might create the object and try appending stuff to it before messenger is set up -> things blow up @@ +854,5 @@ > + } > + } > + > + // If we get here, something is very wrong. > + Cu.reportError("Couldn't get index of first visible row for attachmentlist!\n"); I think we can remove this
Assignee | ||
Comment 42•6 years ago
|
||
Assignee | ||
Comment 43•6 years ago
|
||
Comment 44•6 years ago
|
||
You need to wait for bug 1533085 because 14 attachment-related (sub)tests are currently switched off. The problem should be fixed with an upcoming M-C merge.
Assignee | ||
Comment 46•6 years ago
|
||
Comment 47•6 years ago
|
||
the mozmill attachment/test-attachment.js failures look very much related to this bug
Updated•6 years ago
|
Assignee | ||
Comment 50•6 years ago
|
||
Assignee | ||
Comment 51•6 years ago
|
||
Assignee | ||
Comment 52•5 years ago
|
||
Comment 53•5 years ago
|
||
Comment on attachment 9061223 [details] [diff] [review] attachmentlist-elems.patch Review of attachment 9061223 [details] [diff] [review]: ----------------------------------------------------------------- This seems ready to land. Let's address the comments in a followup ::: mail/base/content/mailWidgets.js @@ +1229,5 @@ > + } > + > + _addChildren() { > + this.appendChild(MozXULElement.parseXULToFragment(` > + <scrollbox flex="1" style="overflow: auto;"> wonder if the list shouldn't be wrapped in a scrollbox instead... @@ +1241,5 @@ > + return this.querySelector("scrollbox"); > + } > + > + get attachmentListWrapper() { > + return this.querySelector("hbox.attachmentlist-wrapper"); do we need the attachmentListWrapper? @@ +1368,5 @@ > + } > + box.scrollTop = item.getBoundingClientRect().y - box.getBoundingClientRect().y; > + } > + > + appendItem(attachment, name) { I think this isn't quite correct, the setup in connectedCallback could be prevented since there are now two paths to this._addChildren. In practice we don't do that atm, but it should still be fixed
Updated•5 years ago
|
Comment 54•5 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/4192b7f68d58
Migrate attachmentlist-* bindings to custom element. r=mkmelin
Updated•5 years ago
|
Comment 55•5 years ago
|
||
Backout:
https://hg.mozilla.org/comm-central/rev/0880f94b2f3155c2500c1eabaca0e246a4ddd66a
Backed out changeset 4192b7f68d58 (bug 1523607) for test failures. a=backout DONTBUILD
This breaks mozmill/composition/test-attachment.js | test-attachment.js::test_attachment_reordering. This test doesn't run on Linux, therefore you didn't see the failure in the try run. Always run at least Linux and Mac on try, at least during the final stages.
Comment 56•5 years ago
|
||
this patch, as is, will regress this change:
https://hg.mozilla.org/comm-central/diff/d167921e42eba41654952c307cec2f0fcaa022d2/mail/base/content/mailWidgets.xml
Assignee | ||
Comment 58•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Comment 59•5 years ago
|
||
Maybe we give this another quick review:
https://bugzilla.mozilla.org/attachment.cgi?oldid=9061223&action=interdiff&newid=9065978&headers=1
Looks OK to me, mostly removal on .scrollbox
and .attachmentListWrapper
. Interdiff confused on messengercompose.xul.
Assignee | ||
Comment 60•5 years ago
|
||
https://bugzilla.mozilla.org/attachment.cgi?oldid=9061223&action=interdiff&newid=9065978&headers=1 Magnus, could you please give it a quick look?
Comment 61•5 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/38204c5426e0
Migrate attachmentlist-* bindings to custom element. r=mkmelin
Comment 62•5 years ago
|
||
Comment on attachment 9065978 [details] [diff] [review] attachmentlist-elems.patch Review of attachment 9065978 [details] [diff] [review]: ----------------------------------------------------------------- Looks pretty good, tough I find dragging around attachments in compose result in JavaScript error: chrome://messenger/content/messengercompose/MsgComposeCommands.js, line 5909: TypeError: target.matches is not a function ::: mail/base/content/mailWidgets.js @@ +1134,5 @@ > + constructor() { > + super(); > + > + this.messenger = Cc["@mozilla.org/messenger;1"].createInstance(Ci.nsIMessenger); > + this._setupEventListeners(); _setupEventListeners() is only called once, so I'd inline it ::: mail/base/content/msgHdrView.js @@ +2560,5 @@ > > if (updateFocus) > attachmentList.focus(); > + > + attachmentList.selectItem(attachmentList.firstChild); not sure the first item is supposed to get selected. did the old code do this?
Assignee | ||
Comment 63•5 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #62)
Comment on attachment 9065978 [details] [diff] [review]
attachmentlist-elems.patchReview of attachment 9065978 [details] [diff] [review]:
Looks pretty good, tough I find dragging around attachments in compose
result inJavaScript error:
chrome://messenger/content/messengercompose/MsgComposeCommands.js, line
5909: TypeError: target.matches is not a function
this is not related to this patch. it was already there and it has to do with some richlistbox stuff.
::: mail/base/content/mailWidgets.js
@@ +1134,5 @@
- constructor() {
- super();
- this.messenger = Cc["@mozilla.org/messenger;1"].createInstance(Ci.nsIMessenger);
- this._setupEventListeners();
_setupEventListeners() is only called once, so I'd inline it
all right, I ll do that in Bug 1547699.
::: mail/base/content/msgHdrView.js
@@ +2560,5 @@if (updateFocus) attachmentList.focus();
- attachmentList.selectItem(attachmentList.firstChild);
not sure the first item is supposed to get selected. did the old code do
this?
I guess I added it because when you open the attachmentlist with horzontal orient then the first attachmentlist need to be selected. I ll check it again in Bug 1547699.
Comment 64•5 years ago
|
||
(In reply to Arshad Khan [:arshad] from comment #63)
JavaScript error:
chrome://messenger/content/messengercompose/MsgComposeCommands.js, line
5909: TypeError: target.matches is not a function
this is not related to this patch. it was already there and it has to do with some richlistbox stuff.
Confirmed. We should fix it though. Also, you can't drag an attachment to the end to be the last item in the list.
Updated•5 years ago
|
Comment 65•5 years ago
|
||
You forgot to remove https://searchfox.org/comm-central/source/common/bindings/richlistbox.xml, right?
Assignee | ||
Comment 66•5 years ago
|
||
yes i ll remove it in Bug 1547699.
Description
•