Arshad Khan [:arshad] Assignee
	Description • 6 years ago

      No description provided.

	Arshad Khan [:arshad] Assignee
	Comment 1 • 6 years ago

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

	Arshad Khan [:arshad] 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?

	Arshad Khan [:arshad] 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.

	Magnus Melin [:mkmelin]
	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?

	Arshad Khan [:arshad] 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.

	Arshad Khan [:arshad] Assignee
	Comment 9 • 6 years ago

they dont self close elements**

	Magnus Melin [:mkmelin]
	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.

	Magnus Melin [:mkmelin]
	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?

	Richard Marti (:Paenglab)
	Comment 13 • 6 years ago

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

	Arshad Khan [:arshad] 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?

	Magnus Melin [:mkmelin]
	Comment 15 • 6 years ago

Yes underscore naming for private is good

	Magnus Melin [:mkmelin]
	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;
}

	Arshad Khan [:arshad] 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..

	Arshad Khan [:arshad] 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..

	Arshad Khan [:arshad] Assignee
	Comment 21 • 6 years ago

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

	Magnus Melin [:mkmelin]
	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

	Geoff Lankow (:darktrojan)
	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; }'".

	Magnus Melin [:mkmelin]
	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

	Arshad Khan [:arshad] Assignee
	Comment 25 • 6 years ago

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

	Richard Marti (:Paenglab)
	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?

	Magnus Melin [:mkmelin]
	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.

	Arshad Khan [:arshad] 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

	Pulsebot
	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

	Jorg K (CEST = GMT+2)
	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 :-(((

	Arshad Khan [:arshad] 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 :(

	Jorg K (CEST = GMT+2)
	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 ;-)

	Pulsebot
	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

	Arshad Khan [:arshad] 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..

	Arshad Khan [:arshad] 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..

	Arshad Khan [:arshad] 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 ..

	Arshad Khan [:arshad] Assignee
	Comment 40 • 6 years ago

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

	Arshad Khan [:arshad] 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.

	Arshad Khan [:arshad] 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..

	Arshad Khan [:arshad] Assignee
	Comment 43 • 6 years ago

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

	Arshad Khan [:arshad] Assignee
	Comment 44 • 6 years ago

Magnus, which approach do you think is better?

	Arshad Khan [:arshad] Assignee
	Comment 45 • 6 years ago

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

	Arshad Khan [:arshad] Assignee
	Comment 46 • 6 years ago

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

	Arshad Khan [:arshad] Assignee
	Comment 47 • 6 years ago

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

	Magnus Melin [:mkmelin]
	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

	Arshad Khan [:arshad] 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?

	Magnus Melin [:mkmelin]
	Comment 50 • 6 years ago

Let's do the CE version

	Arshad Khan [:arshad] Assignee
	Comment 52 • 6 years ago

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

	Jorg K (CEST = GMT+2)
	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

	Arshad Khan [:arshad] 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..

	Arshad Khan [:arshad] Assignee
	Comment 55 • 6 years ago

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

	Pulsebot
	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