Closed
Bug 1491660
Opened 6 years ago
Closed 6 years ago
[de-xbl] Migrate statusbar and statusbarpanel to custom element.
Categories
(Thunderbird :: General, task)
Thunderbird
General
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 65.0
People
(Reporter: arshad, Assigned: arshad)
References
Details
Attachments
(2 files, 28 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
arshad
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → arshdkhn1
Assignee | ||
Comment 1•6 years ago
|
||
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.
Attachment #9009443 -
Flags: feedback?(mkmelin+mozilla)
Assignee | ||
Updated•6 years ago
|
Summary: [de-xbl] Migrate statusbar to custom element. → [de-xbl] Migrate statusbar and statusbarpanel to custom element.
Assignee | ||
Comment 2•6 years ago
|
||
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?
Attachment #9009451 -
Flags: feedback?(mkmelin+mozilla)
Assignee | ||
Updated•6 years ago
|
Blocks: tb-war-on-xbl
Comment 3•6 years ago
|
||
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.
Attachment #9009451 -
Flags: feedback?(mkmelin+mozilla)
Updated•6 years ago
|
Attachment #9009443 -
Flags: feedback?(mkmelin+mozilla)
Updated•6 years ago
|
Attachment #9009443 -
Attachment is obsolete: true
Updated•6 years ago
|
Attachment #9009451 -
Attachment is obsolete: true
Assignee | ||
Comment 4•6 years ago
|
||
Assignee | ||
Comment 5•6 years ago
|
||
to be put before statusbarpanel.patch
Assignee | ||
Updated•6 years ago
|
Attachment #9011453 -
Attachment is obsolete: true
Assignee | ||
Comment 6•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Attachment #9011454 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #9011455 -
Attachment is obsolete: true
Assignee | ||
Comment 7•6 years ago
|
||
Assignee | ||
Comment 8•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•6 years ago
|
Attachment #9011456 -
Flags: review?(mkmelin+mozilla)
Comment 9•6 years ago
|
||
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
Attachment #9011456 -
Flags: review?(mkmelin+mozilla)
Comment 10•6 years ago
|
||
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"));
}
Assignee | ||
Comment 11•6 years ago
|
||
> 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..
Assignee | ||
Comment 12•6 years ago
|
||
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..
Attachment #9011456 -
Attachment is obsolete: true
Attachment #9011457 -
Attachment is obsolete: true
Assignee | ||
Comment 13•6 years ago
|
||
Attachment #9011800 -
Attachment is obsolete: true
Attachment #9011828 -
Flags: feedback?(mkmelin+mozilla)
Comment 14•6 years ago
|
||
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 ?
Attachment #9011828 -
Flags: feedback?(mkmelin+mozilla) → feedback-
Assignee | ||
Comment 15•6 years ago
|
||
(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
Assignee | ||
Comment 16•6 years ago
|
||
Attachment #9011828 -
Attachment is obsolete: true
Assignee | ||
Comment 17•6 years ago
|
||
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•6 years ago
|
||
(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
Assignee | ||
Comment 19•6 years ago
|
||
Attachment #9012883 -
Attachment is obsolete: true
Assignee | ||
Comment 20•6 years ago
|
||
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.
Attachment #9014304 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 21•6 years ago
|
||
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.
Assignee | ||
Comment 22•6 years ago
|
||
Comment 23•6 years ago
|
||
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
Attachment #9014304 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 24•6 years ago
|
||
Attachment #9014304 -
Attachment is obsolete: true
Attachment #9014390 -
Flags: review?(mkmelin+mozilla)
Comment 25•6 years ago
|
||
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
Attachment #9014390 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 26•6 years ago
|
||
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..
Assignee | ||
Comment 27•6 years ago
|
||
Attachment #9014390 -
Attachment is obsolete: true
Assignee | ||
Comment 28•6 years ago
|
||
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..
Attachment #9015017 -
Flags: review?(mkmelin+mozilla)
Comment 29•6 years ago
|
||
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
Assignee | ||
Comment 30•6 years ago
|
||
(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..
Assignee | ||
Comment 31•6 years ago
|
||
Attachment #9015017 -
Attachment is obsolete: true
Attachment #9015017 -
Flags: review?(mkmelin+mozilla)
Attachment #9015078 -
Flags: review?(mkmelin+mozilla)
Comment 32•6 years ago
|
||
(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•6 years ago
|
||
(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•6 years ago
|
||
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
Attachment #9015078 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 35•6 years ago
|
||
(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.
Assignee | ||
Comment 36•6 years ago
|
||
> @@ +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.
Assignee | ||
Comment 37•6 years ago
|
||
(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..
Assignee | ||
Comment 38•6 years ago
|
||
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?
Attachment #9015078 -
Attachment is obsolete: true
Attachment #9015542 -
Flags: review?(mkmelin+mozilla)
Comment 39•6 years ago
|
||
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•6 years ago
|
||
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.
Attachment #9015542 -
Flags: review?(mkmelin+mozilla) → review-
Assignee | ||
Comment 41•6 years ago
|
||
Attachment #9015542 -
Attachment is obsolete: true
Attachment #9016607 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 42•6 years ago
|
||
Attachment #9016607 -
Attachment is obsolete: true
Attachment #9016607 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 43•6 years ago
|
||
Attachment #9016608 -
Attachment is obsolete: true
Attachment #9016609 -
Flags: review?(mkmelin+mozilla)
Comment 44•6 years ago
|
||
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
Attachment #9016609 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 45•6 years ago
|
||
Attachment #9016609 -
Attachment is obsolete: true
Assignee | ||
Comment 46•6 years ago
|
||
Attachment #9017136 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #9017138 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 47•6 years ago
|
||
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?
Attachment #9017138 -
Attachment is obsolete: true
Attachment #9017138 -
Flags: review?(mkmelin+mozilla)
Flags: needinfo?(mkmelin+mozilla)
Attachment #9017145 -
Flags: review?(mkmelin+mozilla)
Comment 48•6 years ago
|
||
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
Attachment #9017138 -
Attachment is obsolete: false
Comment 49•6 years ago
|
||
(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?
Flags: needinfo?(mkmelin+mozilla)
Assignee | ||
Comment 50•6 years ago
|
||
(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•6 years ago
|
||
Not sure what you meant, but I meant that couldn't it all quite easily be hboxes etc?
Assignee | ||
Comment 52•6 years ago
|
||
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.
Attachment #9017138 -
Attachment is obsolete: true
Attachment #9017145 -
Attachment is obsolete: true
Attachment #9017145 -
Flags: review?(mkmelin+mozilla)
Attachment #9017205 -
Flags: feedback?(mkmelin+mozilla)
Comment 53•6 years ago
|
||
Comment on attachment 9017205 [details] [diff] [review]
statusbar-statusbarpanel_WIP.patch
Review of attachment 9017205 [details] [diff] [review]:
-----------------------------------------------------------------
yeah
Attachment #9017205 -
Flags: feedback?(mkmelin+mozilla) → feedback+
Assignee | ||
Comment 54•6 years ago
|
||
Attachment #9017205 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #9018608 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 55•6 years ago
|
||
Assignee | ||
Comment 56•6 years ago
|
||
Assignee | ||
Comment 57•6 years ago
|
||
Assignee | ||
Comment 58•6 years ago
|
||
Comment 59•6 years ago
|
||
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
Assignee | ||
Comment 60•6 years ago
|
||
Attachment #9018608 -
Attachment is obsolete: true
Attachment #9018608 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Updated•6 years ago
|
Attachment #9018902 -
Flags: review?(mkmelin+mozilla)
Comment 61•6 years ago
|
||
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
Attachment #9018902 -
Flags: review?(mkmelin+mozilla) → review+
Comment 62•6 years ago
|
||
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".
Assignee | ||
Comment 63•6 years ago
|
||
Attachment #9018902 -
Attachment is obsolete: true
Attachment #9018998 -
Flags: feedback?(mkmelin+mozilla)
Assignee | ||
Comment 64•6 years ago
|
||
Attachment #9018998 -
Attachment is obsolete: true
Attachment #9018998 -
Flags: feedback?(mkmelin+mozilla)
Attachment #9018999 -
Flags: review+
Attachment #9018999 -
Flags: feedback?(mkmelin+mozilla)
Comment 65•6 years ago
|
||
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
Attachment #9018998 -
Attachment is obsolete: false
Attachment #9018998 -
Flags: review?(makemyday)
Updated•6 years ago
|
Attachment #9018998 -
Attachment is obsolete: true
Attachment #9018998 -
Flags: review?(makemyday)
Updated•6 years ago
|
Attachment #9018999 -
Flags: feedback?(mkmelin+mozilla) → review?(makemyday)
Comment 66•6 years ago
|
||
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.
Attachment #9018999 -
Flags: review?(makemyday) → review+
Assignee | ||
Comment 67•6 years ago
|
||
Attachment #9018999 -
Attachment is obsolete: true
Assignee | ||
Comment 68•6 years ago
|
||
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?
Attachment #9020658 -
Flags: feedback?(makemyday)
Assignee | ||
Updated•6 years ago
|
Attachment #9020656 -
Flags: review+
Assignee | ||
Comment 69•6 years ago
|
||
Comment 70•6 years ago
|
||
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.
Assignee | ||
Comment 71•6 years ago
|
||
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.
Attachment #9020656 -
Attachment is obsolete: true
Flags: needinfo?(makemyday)
Assignee | ||
Comment 72•6 years ago
|
||
in other words, i am interested in knowing what is the lifecycle of overlay?
Comment 73•6 years ago
|
||
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
Updated•6 years ago
|
Flags: needinfo?(makemyday)
Assignee | ||
Comment 74•6 years ago
|
||
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.
Attachment #9020748 -
Attachment is obsolete: true
Attachment #9021642 -
Flags: review+
Attachment #9021642 -
Flags: feedback?(makemyday)
Comment 75•6 years ago
|
||
Comment on attachment 9021642 [details] [diff] [review]
statusbar-statusbarpanel_3.patch
Review of attachment 9021642 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks, looks good.
Attachment #9021642 -
Flags: feedback?(makemyday)
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 76•6 years ago
|
||
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•6 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/87943872543e
Remove statusbar and statusbarpanel bindings. r=mkmelin,MakeMyDay
Updated•6 years ago
|
Target Milestone: --- → Thunderbird 65.0
Comment 78•6 years ago
|
||
(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">
Assignee | ||
Updated•6 years ago
|
Attachment #9020658 -
Flags: feedback?(makemyday)
Updated•5 years ago
|
Type: enhancement → task
You need to log in
before you can comment on or make changes to this bug.
Description
•