Closed Bug 1489795 Opened Last year Closed 11 months ago

[de-xbl] Remove statuspanel binding.

Categories

(Thunderbird :: Mail Window Front End, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 64.0

People

(Reporter: arshad, Assigned: arshad)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 14 obsolete files)

5.54 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
No description provided.
Assignee: nobody → arshdkhn1
Status: NEW → ASSIGNED
Attached patch statuspanel.patch (obsolete) — Splinter Review
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)
Attached patch statuspanel.patch (obsolete) — Splinter Review
Attachment #9007484 - Attachment is obsolete: true
Attachment #9007544 - Flags: review?(mkmelin+mozilla)
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 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?
Attachment #9007544 - Flags: review?(mkmelin+mozilla)
Attached patch statuspanel.patch (obsolete) — Splinter Review
Attachment #9007544 - Attachment is obsolete: true
Attachment #9009441 - Flags: review?(mkmelin+mozilla)
Attached patch statuspanel.patch (obsolete) — Splinter Review
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)
they dont self close elements**
Summary: [de-xbl] Migrate statuspanel to custom element. → [de-xbl] Migrate statuspanel and statuspanelbar to custom element.
Summary: [de-xbl] Migrate statuspanel and statuspanelbar to custom element. → [de-xbl] Migrate statuspanel to custom element.
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.
Attached patch statuspanel.patch (obsolete) — Splinter Review
Attachment #9009446 - Attachment is obsolete: true
Attachment #9012152 - Flags: review?(mkmelin+mozilla)
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)
Maybe bug 1444891 could be good to look how this is done in m-c.
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)
Yes underscore naming for private is good
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-
Attached patch statuspanel.patch (obsolete) — Splinter Review
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)
Attached patch statuspanel.patch (obsolete) — Splinter Review
Attachment #9014047 - Attachment is obsolete: true
Attachment #9014047 - Flags: feedback?(mkmelin+mozilla)
Attachment #9014099 - Flags: feedback?(mkmelin+mozilla)
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..
Attached patch statuspanel.patch (obsolete) — Splinter Review
Attachment #9014099 - Attachment is obsolete: true
Attachment #9014099 - Flags: feedback?(mkmelin+mozilla)
Attachment #9014297 - Flags: review?(mkmelin+mozilla)
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+
Keywords: checkin-needed
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; }'".
How do you actually run single calendar tests? I'd have expected 
make mozmill-one SOLO_TEST=testBasicFunctionality.js

... or calendar/testBasicFunctionality.js
if you are in obj... dir then - `make -C calendar/test/mozmill SOLO_TEST=testBasicFunctionality.js mozmill-one`
Only a shy question. How can the statuspanel make a failure in calendar when it only shows links when the statusbar is hidden?
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.
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
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/b4b14a46f3f8
Migrate statuspanel to custom element. r=mkmelin
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 64.0
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 :-(((
(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 :(
(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 ;-)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Thunderbird 64.0 → ---
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
(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..
Attached patch statuspanel.patch (obsolete) — Splinter Review
Attachment #9014297 - Attachment is obsolete: true
Attached patch statuspanel.patch (obsolete) — Splinter Review
Attachment #9014873 - Attachment is obsolete: true
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..
Attached patch statuspanel_1.patch (obsolete) — Splinter Review
Attachment #9014874 - Attachment is obsolete: true
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 ..
Attached patch statuspanel_inline.patch (obsolete) — Splinter Review
This patch inlines statuspanel and doesn't uses custom element.. TBH I felt CE approach easier and better to look at.
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..
Magnus, which approach do you think is better?
Flags: needinfo?(mkmelin+mozilla)
Summary: [de-xbl] Migrate statuspanel to custom element. → [de-xbl] Remove statuspanel binding.
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-
(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?
Let's do the CE version
Flags: needinfo?(mkmelin+mozilla)
Attachment #9015080 - Attachment is obsolete: true
Keywords: checkin-needed
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)
Keywords: checkin-needed
(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..
the change makes sure that test passes, which were failing in the patch that was granted review.
Attachment #9015498 - Flags: review?(mkmelin+mozilla) → review+
Keywords: checkin-needed
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: 11 months ago11 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 64.0
You need to log in before you can comment on or make changes to this bug.