1489795 - [de-xbl] Remove statuspanel binding.

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, task)

Comment 1

[Arshad Khan [:arshad]]

binding link - https://searchfox.org/comm-central/source/mail/base/content/tabmail.xml#2942

Comment 2

[Arshad Khan [:arshad]]

Attachment: statuspanel.patch

Comment 3

[Arshad Khan [:arshad]]

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?

Comment 4

[Arshad Khan [:arshad]]

Attachment: statuspanel.patch

Comment 5

[Arshad Khan [:arshad]]

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.

Comment 6

[Magnus Melin [:mkmelin]]

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?

Comment 7

[Arshad Khan [:arshad]]

Attachment: statuspanel.patch

Comment 8

[Arshad Khan [:arshad]]

Attachment: statuspanel.patch

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.

Comment 9

[Arshad Khan [:arshad]]

they dont self close elements**

Comment 10

[Magnus Melin [:mkmelin]]

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.

Comment 11

[Arshad Khan [:arshad]]

Attachment: statuspanel.patch

Comment 12

[Magnus Melin [:mkmelin]]

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?

Comment 13

[Richard Marti (:Paenglab)]

Maybe bug 1444891 could be good to look how this is done in m-c.

Comment 14

[Arshad Khan [:arshad]]

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?

Comment 15

[Magnus Melin [:mkmelin]]

Yes underscore naming for private is good

Comment 16

[Magnus Melin [:mkmelin]]

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;
}

Comment 17

[Arshad Khan [:arshad]]

Attachment: statuspanel.patch

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

Comment 18

[Arshad Khan [:arshad]]

Attachment: statuspanel.patch

Comment 19

[Arshad Khan [:arshad]]

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

Comment 20

[Arshad Khan [:arshad]]

Attachment: statuspanel.patch

Comment 21

[Arshad Khan [:arshad]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=bf00ea76632667b22307a5b45e73941b126205f8

Comment 22

[Magnus Melin [:mkmelin]]

Comment on attachment 9014297 [details] [diff] [review]
statuspanel.patch

Review of attachment 9014297 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, r=mkmelin

Comment 23

[Geoff Lankow (:darktrojan)]

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

[Magnus Melin [:mkmelin]]

How do you actually run single calendar tests? I'd have expected 
make mozmill-one SOLO_TEST=testBasicFunctionality.js

... or calendar/testBasicFunctionality.js

Comment 25

[Arshad Khan [:arshad]]

if you are in obj... dir then - `make -C calendar/test/mozmill SOLO_TEST=testBasicFunctionality.js mozmill-one`

Comment 26

[Richard Marti (:Paenglab)]

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

[Magnus Melin [:mkmelin]]

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.

Comment 28

[Arshad Khan [:arshad]]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b4b14a46f3f8
Migrate statuspanel to custom element. r=mkmelin

Comment 30

[Jorg K (CEST = GMT+2)]

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

Comment 31

[Arshad Khan [:arshad]]

(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

[Jorg K (CEST = GMT+2)]

(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 ;-)

Comment 33

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f386902fa732
Backed out changeset b4b14a46f3f8 for additional test failures in testBasicFunctionality.js. a=backout

Comment 34

[Arshad Khan [:arshad]]

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

Comment 35

[Arshad Khan [:arshad]]

Attachment: statuspanel.patch

Comment 36

[Arshad Khan [:arshad]]

Attachment: statuspanel.patch

Comment 37

[Arshad Khan [:arshad]]

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

Comment 38

[Arshad Khan [:arshad]]

Attachment: statuspanel_1.patch

Comment 39

[Arshad Khan [:arshad]]

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

Comment 40

[Arshad Khan [:arshad]]

[statuspanel_1.patch] - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9e26de5e796ffe2c54681fdb6e52e8e9e2307a3d

Comment 41

[Arshad Khan [:arshad]]

Attachment: statuspanel_inline.patch

This patch inlines statuspanel and doesn't uses custom element.. TBH I felt CE approach easier and better to look at.

Comment 42

[Arshad Khan [:arshad]]

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

Comment 43

[Arshad Khan [:arshad]]

statuspanel_inline.patch - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=733836f6436664a2042dae820e304198965303b0

Comment 44

[Arshad Khan [:arshad]]

Magnus, which approach do you think is better?

Comment 45

[Arshad Khan [:arshad]]

Attachment: statuspanel_inline_1.patch

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=26865994940b9d969c3c1fc9f58262c9a51c9ee8

Comment 46

[Arshad Khan [:arshad]]

Attachment: statuspanel_inline_2.patch

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=14b9eacae45e59f9d9237572f48191548c4486a2

Comment 47

[Arshad Khan [:arshad]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=924968e4fcbed18cc9200e39821a4e47ee5f92b2

Comment 48

[Magnus Melin [:mkmelin]]

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

Comment 49

[Arshad Khan [:arshad]]

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

Comment 50

[Magnus Melin [:mkmelin]]

Let's do the CE version

Comment 51

[Arshad Khan [:arshad]]

Attachment: statuspanel.patch

Comment 52

[Arshad Khan [:arshad]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0aafe597df35d65f18e4c7664e8122171bd42e6d

Comment 53

[Jorg K (CEST = GMT+2)]

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

Comment 54

[Arshad Khan [:arshad]]

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

Comment 55

[Arshad Khan [:arshad]]

the change makes sure that test passes, which were failing in the patch that was granted review.

Comment 56

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/16c71c05cc06
Migrate statuspanel binding to custom element. r=mkmelin