Closed
Bug 1489795
Opened 6 years ago
Closed 6 years ago
[de-xbl] Remove statuspanel binding.
Categories
(Thunderbird :: Mail Window Front End, task)
Thunderbird
Mail Window Front End
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 64.0
People
(Reporter: arshad, Assigned: arshad)
References
Details
Attachments
(1 file, 14 obsolete files)
5.54 KB,
patch
|
mkmelin
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 1•6 years ago
|
||
binding link - https://searchfox.org/comm-central/source/mail/base/content/tabmail.xml#2942
Assignee | ||
Comment 2•6 years ago
|
||
Assignee | ||
Comment 3•6 years ago
|
||
Comment on attachment 9007484 [details] [diff] [review] statuspanel.patch MozXULElement is not defined in tabmail.js. It seems odd becaused I was able to create some custom elements by calling customElements.js from mailGlue.js. I can add `ChromeUtils.import("resource://gre/modules/CustomElementsListener.jsm", null);` to tabmail.js but I wonder why it is not defined?
Flags: needinfo?(mkmelin+mozilla)
Assignee | ||
Comment 4•6 years ago
|
||
Attachment #9007484 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #9007544 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 5•6 years ago
|
||
i don't know why I was able to get MozXULElement while I was working on mail-field. After seeing your comment on mail-field patch I tested again but this time I also got MozXULElement not defined error.. As a solution, I am adding MozXULElement definition in our customElement file.
Flags: needinfo?(mkmelin+mozilla)
Comment 6•6 years ago
|
||
Comment on attachment 9007544 [details] [diff] [review] statuspanel.patch Review of attachment 9007544 [details] [diff] [review]: ----------------------------------------------------------------- Will need to adjust this for bug 1490269 ::: mail/base/content/tabmail.js @@ +13,5 @@ > + connectedCallback() { > + this.appendChild( > + MozXULElement.parseXULToFragment(` > + <hbox class="statuspanel-inner"> > + <label class="statuspanel-label" flex="1" crop="end"> <label ...... /> no?
Updated•6 years ago
|
Attachment #9007544 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 7•6 years ago
|
||
Attachment #9007544 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #9009441 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 8•6 years ago
|
||
I saw some customelement patches of fx guys, they dont self elements. I haven't confirmed it yet whether they are intentionally avoiding that or not. I think there is a reason behind this so I am reverting it back to separate closing tag.
Attachment #9009441 -
Attachment is obsolete: true
Attachment #9009441 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 9•6 years ago
|
||
they dont self close elements**
Assignee | ||
Updated•6 years ago
|
Summary: [de-xbl] Migrate statuspanel to custom element. → [de-xbl] Migrate statuspanel and statuspanelbar to custom element.
Assignee | ||
Updated•6 years ago
|
Summary: [de-xbl] Migrate statuspanel and statuspanelbar to custom element. → [de-xbl] Migrate statuspanel to custom element.
Comment 10•6 years ago
|
||
Ah yes good point. I assume it's in preparation for using the elements in an html document. Custom Elements can't be self-closing.
Assignee | ||
Comment 11•6 years ago
|
||
Attachment #9009446 -
Attachment is obsolete: true
Attachment #9012152 -
Flags: review?(mkmelin+mozilla)
Comment 12•6 years ago
|
||
Comment on attachment 9012152 [details] [diff] [review] statuspanel.patch Review of attachment 9012152 [details] [diff] [review]: ----------------------------------------------------------------- [Arshad says this is not ready yet] ::: mail/base/content/tabmail.js @@ +42,5 @@ > + this._updateAttributes(); > + } > + > + _updateAttributes() { > + if (this.hasAttribute("label")) { what was the reason for this checks? needed?
Attachment #9012152 -
Flags: review?(mkmelin+mozilla)
Comment 13•6 years ago
|
||
Maybe bug 1444891 could be good to look how this is done in m-c.
Assignee | ||
Comment 14•6 years ago
|
||
Comment on attachment 9012152 [details] [diff] [review] statuspanel.patch Review of attachment 9012152 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/tabmail.js @@ +21,5 @@ > + ); > + } > + > + connectedCallback() { > + this._updateAttributes(); I think _methodName for a private like method is better naming scheme.. What do you say Magnus?
Attachment #9012152 -
Flags: review?(mkmelin+mozilla)
Comment 15•6 years ago
|
||
Yes underscore naming for private is good
Comment 16•6 years ago
|
||
Comment on attachment 9012152 [details] [diff] [review] statuspanel.patch Review of attachment 9012152 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/tabmail.js @@ +1,1 @@ > +/** This is not the place to put this. It's been in tabmail.xml wrongly I think as e.g. the composition window has nothing to do with tabmail. I suggest putting it in a separate file statuspanel.js @@ +17,5 @@ > + <hbox class="statuspanel-inner"> > + <label class="statuspanel-label" flex="1" crop="end"></label> > + </hbox> > + `) > + ); always do this in the connectedCallback instead, right? @@ +42,5 @@ > + this._updateAttributes(); > + } > + > + _updateAttributes() { > + if (this.hasAttribute("label")) { This check is wrong (for the remove attr case). I think you should use this instead to protect accessing children not yet existing (the attributeChangedCallback call triggered after constructor) if (this.isConnected) { return; }
Attachment #9012152 -
Flags: review?(mkmelin+mozilla) → review-
Assignee | ||
Comment 17•6 years ago
|
||
this.isConnected is false when the custom element is not added to dom.. In statuspanel case, as soon as line like <statuspanel .../>, statuspanel has parent node and isConnected is true, so to guard the updateAttributes code to get called from the very first call of attributeChangedCallback, I am creating an extra variable to check whether children are appended or not..
Attachment #9012152 -
Attachment is obsolete: true
Attachment #9014047 -
Flags: feedback?(mkmelin+mozilla)
Assignee | ||
Comment 18•6 years ago
|
||
Attachment #9014047 -
Attachment is obsolete: true
Attachment #9014047 -
Flags: feedback?(mkmelin+mozilla)
Attachment #9014099 -
Flags: feedback?(mkmelin+mozilla)
Assignee | ||
Comment 19•6 years ago
|
||
Comment on attachment 9014099 [details] [diff] [review] statuspanel.patch Review of attachment 9014099 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/messenger.xul @@ +740,4 @@ > </panel> > > <notificationbox id="mail-notification-box" notificationside="bottom"/> > + <statuspanel id="statusbar-display"/> removed the label because that triggers attributeChangedCallback before connectedCallbac is called..
Assignee | ||
Comment 20•6 years ago
|
||
Attachment #9014099 -
Attachment is obsolete: true
Attachment #9014099 -
Flags: feedback?(mkmelin+mozilla)
Attachment #9014297 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 21•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=bf00ea76632667b22307a5b45e73941b126205f8
Comment 22•6 years ago
|
||
Comment on attachment 9014297 [details] [diff] [review] statuspanel.patch Review of attachment 9014297 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, r=mkmelin
Attachment #9014297 -
Flags: review?(mkmelin+mozilla) → review+
Updated•6 years ago
|
Keywords: checkin-needed
Comment 23•6 years ago
|
||
I don't think that Try run is clean. It's failing at testBasicFunctionality.js:106 with the message "Timeout waiting for modal dialog to open." instead of the existing problem at testBasicFunctionality.js:97 with the message "Timeout exceeded for '() => { return menulist.value == aValue; }'".
Comment 24•6 years ago
|
||
How do you actually run single calendar tests? I'd have expected make mozmill-one SOLO_TEST=testBasicFunctionality.js ... or calendar/testBasicFunctionality.js
Assignee | ||
Comment 25•6 years ago
|
||
if you are in obj... dir then - `make -C calendar/test/mozmill SOLO_TEST=testBasicFunctionality.js mozmill-one`
Comment 26•6 years ago
|
||
Only a shy question. How can the statuspanel make a failure in calendar when it only shows links when the statusbar is hidden?
Comment 27•6 years ago
|
||
The test fails at https://searchfox.org/comm-central/source/calendar/test/mozmill/testBasicFunctionality.js#106 I assume the assumptions (height?) for the doubleclick the line earlier are now somehow different.
Assignee | ||
Comment 28•6 years ago
|
||
strange, all tests passed locally for me.. TEST-START | /Users/arshadkhan/Documents/hg_repos/thunderbird/source/obj-x86_64-apple-darwin18.0.0/_tests/mozmill/stage/testBasicFunctionality.js | setupModule TEST-PASS | /Users/arshadkhan/Documents/hg_repos/thunderbird/source/obj-x86_64-apple-darwin18.0.0/_tests/mozmill/stage/testBasicFunctionality.js | testBasicFunctionality.js::setupModule TEST-START | /Users/arshadkhan/Documents/hg_repos/thunderbird/source/obj-x86_64-apple-darwin18.0.0/_tests/mozmill/stage/testBasicFunctionality.js | testSmokeTest JavaScript error: chrome://global/content/bindings/radio.xml, line 29: TypeError: control.radioChildConstructed is not a function TEST-START | /Users/arshadkhan/Documents/hg_repos/thunderbird/source/obj-x86_64-apple-darwin18.0.0/_tests/mozmill/stage/testBasicFunctionality.js | teardownTest TEST-PASS | /Users/arshadkhan/Documents/hg_repos/thunderbird/source/obj-x86_64-apple-darwin18.0.0/_tests/mozmill/stage/testBasicFunctionality.js | testBasicFunctionality.js::teardownTest TEST-PASS | /Users/arshadkhan/Documents/hg_repos/thunderbird/source/obj-x86_64-apple-darwin18.0.0/_tests/mozmill/stage/testBasicFunctionality.js | testBasicFunctionality.js::testSmokeTest INFO Passed: 3 INFO Failed: 0 INFO Skipped: 0 SUMMARY-PASS | testBasicFunctionality.js::setupModule SUMMARY-PASS | testBasicFunctionality.js::teardownTest SUMMARY-PASS | testBasicFunctionality.js::testSmokeTest
Comment 29•6 years ago
|
||
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/b4b14a46f3f8 Migrate statuspanel to custom element. r=mkmelin
Updated•6 years ago
|
Target Milestone: --- → Thunderbird 64.0
Comment 30•6 years ago
|
||
Sorry, but why don't you CLEAR checkin-needed??? I saw the test failure and recognised it as known. Sigh. Do I have to read any bug from beginning to end before handing anything :-(((
Assignee | ||
Comment 31•6 years ago
|
||
(In reply to Jorg K (GMT+2) from comment #30) > Sorry, but why don't you CLEAR checkin-needed??? > > I saw the test failure and recognised it as known. Sigh. Do I have to read > any bug from beginning to end before handing anything :-((( I am sorry, I ll take care of this from next time.. sorry again :(
Comment 32•6 years ago
|
||
(In reply to Geoff Lankow (:darktrojan) from comment #23) > I don't think that Try run is clean. It's failing at > testBasicFunctionality.js:106 with the message "Timeout waiting for modal > dialog to open." instead of the existing problem at > testBasicFunctionality.js:97 with the message "Timeout exceeded for '() => { > return menulist.value == aValue; }'". Maybe you (Geoff) should fix the test or disable it, so there is no in-depth knowledge required as to what is pre-existing ;-)
Updated•6 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Thunderbird 64.0 → ---
Comment 33•6 years ago
|
||
Backout by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/f386902fa732 Backed out changeset b4b14a46f3f8 for additional test failures in testBasicFunctionality.js. a=backout
Assignee | ||
Comment 34•6 years ago
|
||
(In reply to Richard Marti (:Paenglab) from comment #26) > Only a shy question. How can the statuspanel make a failure in calendar when > it only shows links when the statusbar is hidden? At the bottom left, is the statuspanel with zero opacity so the click that test is trying occurs at statuspanel.. See https://i.postimg.cc/3Nm6dSBs/Screenshot_2018-10-05_at_6.42.08_PM.png, I add a double click eventlistener on statuspanel. I wonder why does xul statuspanel didn't get clicked..
Assignee | ||
Comment 35•6 years ago
|
||
Attachment #9014297 -
Attachment is obsolete: true
Assignee | ||
Comment 36•6 years ago
|
||
Attachment #9014873 -
Attachment is obsolete: true
Assignee | ||
Comment 37•6 years ago
|
||
Comment on attachment 9014874 [details] [diff] [review] statuspanel.patch Review of attachment 9014874 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/messenger.css @@ +642,4 @@ > statuspanel[label=""] { > opacity: 0; > pointer-events: none; > + visibility: hidden; I guess, the display="xul:hbox"[1] attribute on statuspanelbinding automatically adds this behaviour to the element.. that's why you will see the statuspanel has 0 height and 0 width when there is empty label attr on statuspanel... [1]: https://searchfox.org/comm-central/source/mail/base/content/tabmail.xml#2929 For custom elements, we need to either give 0 height and 0 width via css or just hide it for no label attr case and empty label attr case..
Assignee | ||
Comment 38•6 years ago
|
||
Attachment #9014874 -
Attachment is obsolete: true
Assignee | ||
Comment 39•6 years ago
|
||
Comment on attachment 9014881 [details] [diff] [review] statuspanel_1.patch Review of attachment 9014881 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/messenger.xul @@ +740,4 @@ > </panel> > > <notificationbox id="mail-notification-box" notificationside="bottom"/> > + <statuspanel id="statusbar-display"/> the failed test case was due to the removal of label, which caused the statuspanel to to visible ..
Assignee | ||
Comment 40•6 years ago
|
||
[statuspanel_1.patch] - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9e26de5e796ffe2c54681fdb6e52e8e9e2307a3d
Assignee | ||
Comment 41•6 years ago
|
||
This patch inlines statuspanel and doesn't uses custom element.. TBH I felt CE approach easier and better to look at.
Assignee | ||
Comment 42•6 years ago
|
||
Comment on attachment 9014910 [details] [diff] [review] statuspanel_inline.patch Review of attachment 9014910 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/mailWindow.js @@ +44,5 @@ > + }, > + get label() { > + return this.panel.getAttribute("label"); > + }, > + removeAttribute(attr) { One con of this approach is you wont be able to dynamically chnage the attributes of label element by changing attributes of parent box#statuspanel..
Assignee | ||
Comment 43•6 years ago
|
||
statuspanel_inline.patch - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=733836f6436664a2042dae820e304198965303b0
Assignee | ||
Comment 44•6 years ago
|
||
Magnus, which approach do you think is better?
Flags: needinfo?(mkmelin+mozilla)
Assignee | ||
Updated•6 years ago
|
Summary: [de-xbl] Migrate statuspanel to custom element. → [de-xbl] Remove statuspanel binding.
Assignee | ||
Comment 45•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=26865994940b9d969c3c1fc9f58262c9a51c9ee8
Attachment #9014910 -
Attachment is obsolete: true
Assignee | ||
Comment 46•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=14b9eacae45e59f9d9237572f48191548c4486a2
Attachment #9014881 -
Attachment is obsolete: true
Attachment #9015026 -
Attachment is obsolete: true
Attachment #9015080 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 47•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=924968e4fcbed18cc9200e39821a4e47ee5f92b2
Comment 48•6 years ago
|
||
Comment on attachment 9015080 [details] [diff] [review] statuspanel_inline_2.patch Review of attachment 9015080 [details] [diff] [review]: ----------------------------------------------------------------- While inlining is fairly simple, I think I like the CE version better. In particular, I don't like how the object is tied in that closely to the id. That is possible to parametrize but still... Then you still have an hbox with a label attribute, which is not nice either. ::: mail/base/content/messenger.xul @@ +743,5 @@ > + <box id="statuspanel" label=""> > + <hbox class="statuspanel-inner"> > + <label class="statuspanel-label" > + flex="1" > + crop="end"/> maybe just put it on one line since it fits nicely
Attachment #9015080 -
Flags: review?(mkmelin+mozilla) → feedback-
Assignee | ||
Comment 49•6 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #48) > Comment on attachment 9015080 [details] [diff] [review] > statuspanel_inline_2.patch > > Review of attachment 9015080 [details] [diff] [review]: > ----------------------------------------------------------------- > > While inlining is fairly simple, I think I like the CE version better. > > In particular, I don't like how the object is tied in that closely to the > id. That is possible to parametrize but still... Then you still have an hbox > with a label attribute, which is not nice either. > > ::: mail/base/content/messenger.xul > @@ +743,5 @@ > > + <box id="statuspanel" label=""> > > + <hbox class="statuspanel-inner"> > > + <label class="statuspanel-label" > > + flex="1" > > + crop="end"/> > > maybe just put it on one line since it fits nicely What should we go for ce or inlined object?
Assignee | ||
Comment 51•6 years ago
|
||
Attachment #9015080 -
Attachment is obsolete: true
Assignee | ||
Comment 52•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0aafe597df35d65f18e4c7664e8122171bd42e6d
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 53•6 years ago
|
||
Comment on attachment 9015498 [details] [diff] [review] statuspanel.patch Another review here? If I'm not mistaken, this has changed after the last review: https://bugzilla.mozilla.org/attachment.cgi?oldid=9014297&action=interdiff&newid=9015498&headers=1
Attachment #9015498 -
Flags: review?(mkmelin+mozilla)
Updated•6 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 54•6 years ago
|
||
(In reply to Jorg K (GMT+2) from comment #53) > Comment on attachment 9015498 [details] [diff] [review] > statuspanel.patch > > Another review here? If I'm not mistaken, this has changed after the last > review: > https://bugzilla.mozilla.org/attachment. > cgi?oldid=9014297&action=interdiff&newid=9015498&headers=1 that is a better(with less changes) approach than the appraoch used in patch which was reviewed..
Assignee | ||
Comment 55•6 years ago
|
||
the change makes sure that test passes, which were failing in the patch that was granted review.
Updated•6 years ago
|
Attachment #9015498 -
Flags: review?(mkmelin+mozilla) → review+
Updated•6 years ago
|
Keywords: checkin-needed
Comment 56•6 years ago
|
||
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/16c71c05cc06 Migrate statuspanel binding to custom element. r=mkmelin
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Updated•6 years ago
|
Target Milestone: --- → Thunderbird 64.0
Updated•5 years ago
|
Type: enhancement → task
You need to log in
before you can comment on or make changes to this bug.
Description
•