1491660 - [de-xbl] Migrate statusbar and statusbarpanel to custom element.

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Comment 1

[Arshad Khan [:arshad]]

Attachment: statusbar.patch

What to do in case of role and display attribute? Other than the role attribute, I think the element is working well. I still have to run mozmill tests tho.

Comment 2

[Arshad Khan [:arshad]]

Attachment: statusbarpanel.patch

While running this, I get 
`JavaScript error: chrome://messenger/content/generalBindings.js, line 68: NotSupportedError: Operation is not supported` error in conosle. Can you tell what's wrong?

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 9009451 [details] [diff] [review]
statusbarpanel.patch

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

Looks ok in general, but 2 space indent please.
Patched don't apply so I won't try them out now

::: common/bindings/generalBindings.js
@@ +4,4 @@
>  
>  /* global MozXULElement */
>  
> +const updateAttributes = (node) => {

please make this an internal helper instead.

Comment 4

[Arshad Khan [:arshad]]

Attachment: statusbarpanel.patch

Comment 5

[Arshad Khan [:arshad]]

Attachment: statusbar.patch

to be put before statusbarpanel.patch

Comment 6

[Arshad Khan [:arshad]]

Attachment: statusbarpanel.patch

Comment 7

[Arshad Khan [:arshad]]

Attachment: statusbar.patch

Comment 8

[Arshad Khan [:arshad]]

Attachment: statusbarpanel.patch

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9011456 [details] [diff] [review]
statusbar.patch

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

Doesn't apply anymore, and please use 2 space indent.

Might make sense to fold these two patches into one

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 9011457 [details] [diff] [review]
statusbarpanel.patch

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

::: common/bindings/generalBindings.js
@@ +15,3 @@
>          this.appendChild(
>              MozXULElement.parseXULToFragment(`
> +            <label class="statusbarpanel-label" crop="right" flex="1"></label>

where are the crop and flex attributes coming from? I don't see them in the original

@@ +26,5 @@
> +    attributeChangedCallback() {
> +        updateAttributes();
> +    }
> +
> +

double empty line

@@ +29,5 @@
> +
> +
> +    updateAttributes() {
> +        this.hasAttribute("label") &&
> +            this.querySelector(".statusbarpanel-label").setAttribute("value", node.getAttribute("label"));

same questions as for the other patch - why can't it be set if it's not present?

and use this style instead
if (this.hasAttribute("label")) {
  this.querySelector(".statusbarpanel-label").setAttribute("value", node.getAttribute("label"));
}

Comment 11

[Arshad Khan [:arshad]]

> where are the crop and flex attributes coming from? I don't see them in the
> original
https://searchfox.org/comm-central/source/common/bindings/generalBindings.xml#25


> same questions as for the other patch - why can't it be set if it's not
> present?
if parent doesn't have the label attribute then parentNode.getAttribute("label") will return undefined, but statusbarpanel-label's value attribute will be set to emptyString(""). There can be cases where css is appied to element having an attribute and we might not want to have those styles unless the atrribute has some valid value. In xbl this case doesn't happen, but in custom elements we are manually setting attributes.. 

I hope it makes sense, I know it getting very nit picky here..

Comment 12

[Arshad Khan [:arshad]]

Attachment: statusbar-statusbarpanel.patch

NotSupportError is thrown from line69/70 of generalBindings.js and I think it happens when custom elements are appended to xul elements or vice versa.. Other than that I think this patch works..

Comment 13

[Arshad Khan [:arshad]]

Attachment: statusbar-statusbarpanel.patch

Comment 14

[Magnus Melin [:mkmelin]]

Comment on attachment 9011828 [details] [diff] [review]
statusbar-statusbarpanel.patch

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

::: common/bindings/generalBindings.js
@@ +14,5 @@
> +    this.appendChild(
> +      MozXULElement.parseXULToFragment(`
> +      <label class="statusbarpanel-label" crop="right" flex="1"></label>
> +      `)
> +    );

Appending in the constructor was the cause for the errors, right? So you'd need to move it to connecteCallback

Although, this looks like a good candidate to drop parseXULToFragment for, and do it like for mail-headerfield with the text directly in the element. Then you don't need to do all the attribute observing and forwarding either

@@ +28,5 @@
> +
> +  updateAttributes() {
> +    if (this.hasAttribute("label")) {
> +      this.querySelector(".statusbarpanel-label").setAttribute("value", this.getAttribute("label"));
> +    }

IF you were to need this, isn't this slightly wrong? You'd need to remove the attribute instead if it was now null/undefined ?

Comment 15

[Arshad Khan [:arshad]]

(In reply to Magnus Melin [:mkmelin] from comment #14)
> Comment on attachment 9011828 [details] [diff] [review]
> statusbar-statusbarpanel.patch
> 
> Review of attachment 9011828 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: common/bindings/generalBindings.js
> @@ +14,5 @@
> > +    this.appendChild(
> > +      MozXULElement.parseXULToFragment(`
> > +      <label class="statusbarpanel-label" crop="right" flex="1"></label>
> > +      `)
> > +    );
> 
> Appending in the constructor was the cause for the errors, right? So you'd
> need to move it to connecteCallback

yes updating it.. got blocked by mohave issue

> Although, this looks like a good candidate to drop parseXULToFragment for,
> and do it like for mail-headerfield with the text directly in the element.
> Then you don't need to do all the attribute observing and forwarding either

correct.. lemme try that out..

> @@ +28,5 @@
> > +
> > +  updateAttributes() {
> > +    if (this.hasAttribute("label")) {
> > +      this.querySelector(".statusbarpanel-label").setAttribute("value", this.getAttribute("label"));
> > +    }
> 
> IF you were to need this, isn't this slightly wrong? You'd need to remove
> the attribute instead if it was now null/undefined ?

If the attribute are never set then what's the need to write code for removing them in case they are null/undefined

Comment 16

[Arshad Khan [:arshad]]

Attachment: statusbar-statusbarpanel.patch

Comment 17

[Arshad Khan [:arshad]]

i got scared by the errors that i faced when i dropped parseXULtoFragment.. i ll try it again but as of now i let it be as it is.

Comment 18

[Magnus Melin [:mkmelin]]

(In reply to Arshad Khan [:arshad] from comment #15)
> If the attribute are never set then what's the need to write code for
> removing them in case they are null/undefined

I was thinking of the case where the attribute *was* set, and then later is removed

Comment 19

[Arshad Khan [:arshad]]

Attachment: statusbar-statusbarpanel.patch

Comment 20

[Arshad Khan [:arshad]]

Comment on attachment 9014304 [details] [diff] [review]
statusbar-statusbarpanel.patch

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

::: common/bindings/generalBindings.js
@@ +54,5 @@
> +    return this.getAttribute("src");
> +  }
> +
> +  _updateAttributes() {
> +    if (!this.isConnected || !this.areChildrenAppended) {

areChildrenAppended makes sure that below code is executed only after connectedCallback is called.

Comment 21

[Arshad Khan [:arshad]]

Comment on attachment 9014304 [details] [diff] [review]
statusbar-statusbarpanel.patch

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

areChildrenAppended makes sure that below code is executed only after connectedCallback is called.

Comment 22

[Arshad Khan [:arshad]]

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

Comment 23

[Magnus Melin [:mkmelin]]

Comment on attachment 9014304 [details] [diff] [review]
statusbar-statusbarpanel.patch

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

Can you update it to use this._labelElement?

::: common/bindings/generalBindings.js
@@ +16,5 @@
> +    statusbarpanelLabel.setAttribute("flex", "1");
> +
> +    this.appendChild(statusbarpanelLabel);
> +
> +    this.areChildrenAppended = true;

I think the approach I saw elsewhere is that you instead here do

this._labelElement = statusbarpanelLabel;

Then you can check if (!this._labelElement) return; later and you don't have to query it again

Comment 24

[Arshad Khan [:arshad]]

Attachment: statusbar-statusbarpanel.patch

Comment 25

[Magnus Melin [:mkmelin]]

Comment on attachment 9014390 [details] [diff] [review]
statusbar-statusbarpanel.patch

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

::: common/bindings/generalBindings.js
@@ +18,5 @@
> +    this._labelElement = statusbarpanelLabel;
> +
> +    this.appendChild(statusbarpanelLabel);
> +
> +    this.areChildrenAppended = true;

remove this leftover from previous iteration

@@ +41,5 @@
> +  set image(val) {
> +    this.setAttribute("image", val);
> +    return val;
> +  }
> +

looks to me the whole image thing is unused? can you verify, and remove if so?

@@ +79,5 @@
> +  connectedCallback() {
> +    const statusbarpanel = document.createElement("statusbarpanel");
> +
> +    const resizer = document.createElement("resizer");
> +    resizer.setAttribute("dir", "bottomend");

there's some problem with the resizer. or at least how lightning integrates with it. the resizer is now  to the right of the "Today Pane ^" button. it should of course be in the bottom corner of the window

Comment 26

[Arshad Khan [:arshad]]

I need some info on https://searchfox.org/comm-central/source/common/bindings/generalBindings.xml#24 . When will be label appended? Not sure where the label inside `<children></children>` goes.. Looking at devtools in TB, it looks like when no children is present inside statusbarpanel then label is appended else label is not appended..

Comment 27

[Arshad Khan [:arshad]]

Attachment: statusbar-statusbarpanel_1.patch

Comment 28

[Arshad Khan [:arshad]]

Comment on attachment 9015017 [details] [diff] [review]
statusbar-statusbarpanel_1.patch

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

::: common/bindings/generalBindings.js
@@ +9,5 @@
> +    return ["label", "crop"];
> +  }
> +
> +  connectedCallback() {
> +    if (this.hasChildNodes()) {

Equivalent of <children><label /></children>.. if there are children, then dont append label else append label

@@ +26,5 @@
> +
> +    this._updateAttributes();
> +  }
> +
> +  attributeChangedCallback() {

removed src and image setters and getters, I couldn't find any code that uses them..

Comment 29

[Magnus Melin [:mkmelin]]

Comment on attachment 9015017 [details] [diff] [review]
statusbar-statusbarpanel_1.patch

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

::: common/bindings/generalBindings.js
@@ +10,5 @@
> +  }
> +
> +  connectedCallback() {
> +    if (this.hasChildNodes()) {
> +      this._labelElement = null;

So if there's a resizer in the statusbarpanel it's not possible to set label? That doesn't sound right

Comment 30

[Arshad Khan [:arshad]]

(In reply to Magnus Melin [:mkmelin] from comment #29)
> Comment on attachment 9015017 [details] [diff] [review]
> statusbar-statusbarpanel_1.patch
> 
> Review of attachment 9015017 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: common/bindings/generalBindings.js
> @@ +10,5 @@
> > +  }
> > +
> > +  connectedCallback() {
> > +    if (this.hasChildNodes()) {
> > +      this._labelElement = null;
> 
> So if there's a resizer in the statusbarpanel it's not possible to set
> label? That doesn't sound right

I would suggest you to check in devtools on Thudnerbird.. calendar event dialog is one place where you can see everythin in action.. I think <children><label/></children> means add label if there are no children.. I know it doesn't make sense but i arried at this conclusion after inspecting in devtools..

Comment 31

[Arshad Khan [:arshad]]

Attachment: statusbar-statusbarpane_2l.patch

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

Comment 32

[Magnus Melin [:mkmelin]]

(In reply to Arshad Khan [:arshad] from comment #30)
> <children><label/></children> means add label if there are no children.. 

Dunno about that. But at the very least this requires some comment in the code. 
So how would the label be updated in case there are children?

Comment 33

[Magnus Melin [:mkmelin]]

(In reply to Magnus Melin [:mkmelin] from comment #25)
> there's some problem with the resizer. or at least how lightning integrates
> with it. the resizer is now  to the right of the "Today Pane ^" button. it
> should of course be in the bottom corner of the window

This is still not working

Comment 34

[Magnus Melin [:mkmelin]]

Comment on attachment 9015078 [details] [diff] [review]
statusbar-statusbarpane_2l.patch

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

(Also see the other comments)

::: common/bindings/generalBindings.js
@@ +14,5 @@
> +      this._labelElement = null;
> +      return;
> +    }
> +
> +    const statusbarpanelLabel = document.createElement("label");

for these cases, maybe it's perferable to directly assign it and don't bother with the tmp statusbarpanelLabel variable

this._labelElement = document.createElement("label");

@@ +31,5 @@
> +    this._updateAttributes();
> +  }
> +
> +  set label(val) {
> +    this.setAttribute("label", val);

if val is null, removeAttribute

Comment 35

[Arshad Khan [:arshad]]

(In reply to Magnus Melin [:mkmelin] from comment #33)
> (In reply to Magnus Melin [:mkmelin] from comment #25)
> > there's some problem with the resizer. or at least how lightning integrates
> > with it. the resizer is now  to the right of the "Today Pane ^" button. it
> > should of course be in the bottom corner of the window
> 
> This is still not working

On mac it is hidden. It would be great if you can attach screenshots for linux.

Comment 36

[Arshad Khan [:arshad]]

> @@ +31,5 @@
> > +    this._updateAttributes();
> > +  }
> > +
> > +  set label(val) {
> > +    this.setAttribute("label", val);
> 
> if val is null, removeAttribute

xbl bindings dont remove attribute if val is null. Doing this has one caveat, the css are not written thinking about the case where attributes will be removed. The bug in statuspanel, where the click at the bottom was happening to statuspanel instead of treenode-children element, was because css was not written expecting statuspanel will exist without label attr so statuspanel without label attr was not hidden like statuspanel[label=""] was hidden, that's why click was firing at statuspanel. It also added a UI glitch as a small rect white box was showing because of the padding on elment with no content.

Removing the attr for now, but if such bugs come again in picture then we have to deal with them.

Comment 37

[Arshad Khan [:arshad]]

(In reply to Magnus Melin [:mkmelin] from comment #33)
> (In reply to Magnus Melin [:mkmelin] from comment #25)
> > there's some problem with the resizer. or at least how lightning integrates
> > with it. the resizer is now  to the right of the "Today Pane ^" button. it
> > should of course be in the bottom corner of the window
> 
> This is still not working

This is because of overlay.. the resizer is added at extreme right of the content before overlay content are added.. 

Figuring out some way to tackle this..

Comment 38

[Arshad Khan [:arshad]]

Attachment: statusbar-statusbarpanel_3.patch

The only clean solution for having resizer at right, when using overlay is to use the insertBefore attribute. I have checked other overrlay using sidebar and it doens't look like there will be any other case where resizer wont be at extreme-right. Lemme know if you any?

Comment 39

[Magnus Melin [:mkmelin]]

Comment on attachment 9015542 [details] [diff] [review]
statusbar-statusbarpanel_3.patch

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

Seems to work, thx

::: common/bindings/generalBindings.js
@@ +12,5 @@
> +  connectedCallback() {
> +    if (this.hasChildNodes()) {
> +      this._labelElement = null;
> +      return;
> +    }

this wondering about this

Comment 40

[Magnus Melin [:mkmelin]]

Comment on attachment 9015542 [details] [diff] [review]
statusbar-statusbarpanel_3.patch

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

We discussed this, and Arshad will look into if the statusbarpanels with children should be something else (hbox perhaps) so that this CE isn't only doing useful stuff in part the cases.

Comment 41

[Arshad Khan [:arshad]]

Attachment: statusbar-statusbarpanel_4.patch

Comment 42

[Arshad Khan [:arshad]]

Attachment: statusbar-statusbarpanel_5.patch

Comment 43

[Arshad Khan [:arshad]]

Attachment: statusbar-statusbarpanel.patch

Comment 44

[Magnus Melin [:mkmelin]]

Comment on attachment 9016609 [details] [diff] [review]
statusbar-statusbarpanel.patch

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

::: calendar/base/content/dialogs/calendar-event-dialog.xul
@@ +579,5 @@
>    <statusbar class="chromeclass-status" id="status-bar">
>      <statusbarpanel id="status-text"
>                      flex="1"/>
> +    <hbox class="statusbarpanel"
> +          id="status-privacy"

please always put id attribute first

if all the attrs fit nicely on one line (80ch) you can do that too

::: common/bindings/generalBindings.js
@@ +14,5 @@
> +    this._labelElement.setAttribute("class", "statusbarpanel-label");
> +    this._labelElement.setAttribute("crop", "right");
> +    this._labelElement.setAttribute("flex", "1");
> +
> +    this.appendChild(this._labelElement);

now that it's only ever a label inside this, can't we just move the content inline and drop the sub <label>?

Given that, maybe the few usages can also be <hbox>es and we don't need this as a CE?
And the same consideration for statusbar

Comment 45

[Arshad Khan [:arshad]]

Attachment: statusbar-statusbarpanel.patch

Comment 46

[Arshad Khan [:arshad]]

Attachment: statusbar-statusbarpanel.patch

Comment 47

[Arshad Khan [:arshad]]

Attachment: statusbar-statusbarpanel.patch

I am still observign label attr, i cann instead remove label attr and modify everything so that innstead of label they depends uponn textContent.. but maybe it might not look cleeann.. what do you think?

Comment 48

[Magnus Melin [:mkmelin]]

Comment on attachment 9017138 [details] [diff] [review]
statusbar-statusbarpanel.patch

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

Please also verify this is working: https://searchfox.org/comm-central/source/mail/base/content/FilterListDialog.js#40 (or adjust to make it work)

::: calendar/lightning/content/messenger-overlay-sidebar.xul
@@ +222,5 @@
> +          align="center"
> +          flex="1"
> +          collapsed="true"
> +          pack="start"
> +          insertbefore="statusbar-resizerpanel">

I don't think you should rely on ids from a CE, and also we should avoid adding reliance on xul-only features like insertbefore. Probably just open code the resizer?

::: common/bindings/generalBindings.js
@@ +17,5 @@
> +    this._updateAttributes();
> +  }
> +
> +  set label(val) {
> +    this.setAttribute("label", val);

I think null should => removeAttribute

@@ +33,5 @@
> +
> +class MozStatusbar extends MozXULElement {
> +  connectedCallback() {
> +    const statusbarpanel = document.createElement("hbox");
> +    statusbarpanel.setAttribute("id", "statusbar-resizerpanel");

I don't think you should set ids for elements in a CE. It removes the generalization, and if you were to put need two elements things wouldn't work anymore

Comment 49

[Magnus Melin [:mkmelin]]

(In reply to Arshad Khan [:arshad] from comment #47)
> Created attachment 9017145 [details] [diff] [review]
> statusbar-statusbarpanel.patch
> 
> I am still observign label attr, i cann instead remove label attr and modify
> everything so that innstead of label they depends uponn textContent.. but
> maybe it might not look cleeann.. what do you think?

I think the current thing is ok. 
But what about getting rid of these as custom elements completely? Is there any usage left that can't easily be adjusted?

Comment 50

[Arshad Khan [:arshad]]

(In reply to Magnus Melin [:mkmelin] from comment #49)
> (In reply to Arshad Khan [:arshad] from comment #47)
> > Created attachment 9017145 [details] [diff] [review]
> > statusbar-statusbarpanel.patch
> > 
> > I am still observign label attr, i cann instead remove label attr and modify
> > everything so that innstead of label they depends uponn textContent.. but
> > maybe it might not look cleeann.. what do you think?
> 
> I think the current thing is ok. 
> But what about getting rid of these as custom elements completely? Is there
> any usage left that can't easily be adjusted?

tbh i dont like the object approach, I cann show it if you want but i am very sure it will look not as clean as this approach.

Comment 51

[Magnus Melin [:mkmelin]]

Not sure what you meant, but I meant that couldn't it all quite easily be hboxes etc?

Comment 52

[Arshad Khan [:arshad]]

Attachment: statusbar-statusbarpanel_WIP.patch

you want somethingn like this, right? the only issue is to put resizer inn overlay and if there is no overlay for statusbar element then put it inside the main file.. I am workinng onn it.. just wannted to let you see what i am doign.

Comment 53

[Magnus Melin [:mkmelin]]

Comment on attachment 9017205 [details] [diff] [review]
statusbar-statusbarpanel_WIP.patch

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

yeah

Comment 54

[Arshad Khan [:arshad]]

Attachment: statusbar-statusbarpanel_1.patch

Comment 55

[Arshad Khan [:arshad]]

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

Comment 56

[Arshad Khan [:arshad]]

with correct tip - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=30148c8e6058a9d54b5e3c3354a3999b4b4ecc79

Comment 57

[Arshad Khan [:arshad]]

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

Comment 58

[Arshad Khan [:arshad]]

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

Comment 59

[Magnus Melin [:mkmelin]]

Comment on attachment 9018608 [details] [diff] [review]
statusbar-statusbarpanel_1.patch

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

::: editor/ui/composer/content/editor.xul
@@ +379,5 @@
> +      <hbox id="structToolbar" class="statusbarpanel" flex="1" pack="end">
> +        <label id="structSpacer" value="" flex="1"/>
> +      </hbox>
> +      <statusbarpanel id="offline-status" class="statusbarpanel-iconic"/>
> +      <statusbarpanel class="statusbar-resizerpanel">

this has children so could just be an hbox, right?
same thing for all the statusbar-resizerpanel in the patch

Comment 60

[Arshad Khan [:arshad]]

Attachment: statusbar-statusbarpanel.patch

Comment 61

[Magnus Melin [:mkmelin]]

Comment on attachment 9018902 [details] [diff] [review]
statusbar-statusbarpanel.patch

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

Looks good except for one thing: the event dialogs in lightning don't have a resizer showing anymore (but are resizeable).

r=mkmelin for the mail bits, but someone from lightning should look at this too

Comment 62

[Magnus Melin [:mkmelin]]

Comment on attachment 9018902 [details] [diff] [review]
statusbar-statusbarpanel.patch

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

Oh, and please add a comment in the hg commit message describing the change. I.e. that statusbarpanels won't have children. If they do, that's just a hbox w/ class "statusbarpanel".

Comment 63

[Arshad Khan [:arshad]]

Attachment: statusbar-statusbarpanel.patch

Comment 64

[Arshad Khan [:arshad]]

Attachment: statusbar-statusbarpanel.patch

Comment 65

[Magnus Melin [:mkmelin]]

Comment on attachment 9018998 [details] [diff] [review]
statusbar-statusbarpanel.patch

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

Yes the resizer is also there now. r=mkmelin

Comment 66

[[:MakeMyDay]]

Comment on attachment 9018999 [details] [diff] [review]
statusbar-statusbarpanel.patch

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

Thanks, r+wc.

::: calendar/base/content/dialogs/calendar-event-dialog.xul
@@ +576,5 @@
>  
>    <!-- the iframe is inserted here dynamically in the "load" handler function -->
>  
> +  <hbox id="status-bar" class="statusbar chromeclass-status">
> +    <statusbarpanel id="status-text" flex="1"/>

You can remove this, we don't need this panel.

::: calendar/providers/gdata/content/gdata-event-dialog.xul
@@ +25,5 @@
>      <hbox id="gdata-status-privacy-default-box"
>            insertbefore="status-privacy-public-box,status-privacy-private-box"
>            privacy="DEFAULT"
>            provider="gdata"/>
> +  </hbox>

Can you move that into gdata-event-dialog.js? Create an element for the inner hbox in JS and append it to document.getElementById("status-privacy") as the first child, so that we can retain backward compatibility here.

Comment 67

[Arshad Khan [:arshad]]

Attachment: statusbar-statusbarpanel_1.patch

Comment 68

[Arshad Khan [:arshad]]

Attachment: Screenshot 2018-10-29 at 5.17.02 AM.png

Hey, I have addressed your comments but removing the statusbarpanel, the calendar event dialog statusbar looks like this.. Earlier the test was in middle but now it is at left.. Do you want it in center or is it ok to have it like this?

Comment 69

[Arshad Khan [:arshad]]

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

Comment 70

[[:MakeMyDay]]

Comment on attachment 9020656 [details] [diff] [review]
statusbar-statusbarpanel_1.patch

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

Yes, moving the panels from the middle to the start is fine. But please check the comment below.

::: calendar/providers/gdata/content/gdata-event-dialog.js
@@ +32,5 @@
> +
> +
> +const gdataStatusPrivacyHbox = document.createElement("hbox");
> +gdataStatusPrivacyHbox.setAttribute("id", "gdata-status-privacy-default-box");
> +gdataStatusPrivacyHbox.setAttribute("insertbefore", "status-privacy-public-box,status-privacy-private-box");

Have you checked that this is working as intended and makes the new node the first child of it's parent?

I would assume you need to remove this and use insertBefore (and using the first child of the parent as reference) instead of appendChild below, since the latter adds a child as the last of its parent.

Comment 71

[Arshad Khan [:arshad]]

Attachment: statusbar-statusbarpanel_2.patch

Is this ok? I thought you meant first child for overlay element.. 

document.querySelector("#status-privacy") gives the element from calendar-event-dialog. I am interested in knowing that is it possible to get reference of the element in overlay before getting overlaid.

Comment 72

[Arshad Khan [:arshad]]

in other words, i am interested in knowing what is the lifecycle of overlay?

Comment 73

[[:MakeMyDay]]

Comment on attachment 9020748 [details] [diff] [review]
statusbar-statusbarpanel_2.patch

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

You shouldn't worry too much about how overlays work in detail since they are going away (and its use in TB has been removed already). Probably the points in time when things are loaded have changed with the extension loader anyway. Probably Geoff knows best whats the current status is. That gdata-event-dialog.js  file is loaded from the gdata-event-dialog.xul hasn't changed, though. So anything in the overlay definition is available from the js file.

::: calendar/providers/gdata/content/gdata-event-dialog.js
@@ +36,5 @@
> +gdataStatusPrivacyHbox.setAttribute("privacy", "DEFAULT");
> +gdataStatusPrivacyHbox.setAttribute("provider", "gdata");
> +
> +const statusPrivacy = document.getElementById("status-privacy");
> +statusPrivacy.insertBefore(gdataStatusPrivacyHbox, statusPrivacy.firstChild);

Yes, this is correct in general. Unfortunately, my previous comment was not fully accurate, since the first child of "status-privacy" is (and schould stay) the label, so you should replace statusPrivacy.firstChild with the node with id "status-privacy-public-box".

(See [1] how insertBefore works in overlays, but you don't need to reimplement the whatever is first approach of the previous implementation)

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/insertbefore

Comment 74

[Arshad Khan [:arshad]]

Attachment: statusbar-statusbarpanel_3.patch

Actually I knew the position where the hbox had to be appended.. but I thought you might want something different so I followed your comment.

Comment 75

[[:MakeMyDay]]

Comment on attachment 9021642 [details] [diff] [review]
statusbar-statusbarpanel_3.patch

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

Thanks, looks good.

Comment 76

[Jorg K (CEST = GMT+2)]

I don't understand this checkin message (enhanced with a few articles 'a', 'the'):
Statusbarpanel elements won't have children. If they do, then instead of a custom element an hbox is used with the statusbarpanel class.

So they won't/don't have children? And then they do? I'll remove the comment or you can provide a better one. Final try:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c930474a665049d701193fb74d2c7e07ea03926d

Comment 77

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/87943872543e
Remove statusbar and statusbarpanel bindings. r=mkmelin,MakeMyDay

Comment 78

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+2) from comment #76)
> I don't understand this checkin message 

<statusbarpanel> used to sometimes have children, and sometimes not. For these two cases it did different things - for the case where children existed it was only about styling. Setting label for such an element was a no-op. Now if we have a statusbarpanel with children it's instead an <hbox class="statusbarpanel">