Closed Bug 1485024 Opened Last year Closed Last year

[de-xbl] Remove lightning-notification-bar binding

Categories

(Calendar :: General, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: arshad, Assigned: arshad)

References

Details

Attachments

(1 file, 23 obsolete files)

9.81 KB, patch
Details | Diff | Splinter Review
No description provided.
Assignee: nobody → arshdkhn1
Component: Untriaged → General
Product: Thunderbird → Calendar
Attached patch lightning-notification-bar.patch (obsolete) — Splinter Review
Attachment #9002776 - Flags: review?(philipp)
Few formatting changes has sneaked in. I ll remove them in next patch.
Comment on attachment 9002776 [details] [diff] [review]
lightning-notification-bar.patch

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

Thanks for working on this. But please limit your changes to the scope of the bug and don't add masses of code style fixes. Changes like adding spaces between function and the following parenthesis will let the linter fail. Also the intendation changes are not wanted. To be on the safe side, you might consider to run eslint before asking for review.
Attachment #9002776 - Flags: feedback-
Attached patch lightning-notification-bar.patch (obsolete) — Splinter Review
Attached patch lightning-notification-bar.patch (obsolete) — Splinter Review
Attachment #9002776 - Attachment is obsolete: true
Attachment #9002941 - Attachment is obsolete: true
Attachment #9002776 - Flags: review?(philipp)
Attached patch lightning-notification-bar.patch (obsolete) — Splinter Review
Attachment #9002944 - Attachment is obsolete: true
Attachment #9003099 - Flags: review?(philipp)
Attachment #9003099 - Flags: review?(makemyday)
Comment on attachment 9003099 [details] [diff] [review]
lightning-notification-bar.patch

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

::: calendar/lightning/content/lightning-widgets.js
@@ +4,5 @@
> +            MozXULElement.parseXULToFragment(`
> +            <image anonid="notification-image"></image>
> +            <description anonid="notification-description" class="msgNotificationBarText" flex="1" inherits="text=label"></description>
> +            <box anonid="notification-children">
> +                <children></children>

This will probably not work. <children> is a special XBL tag representing the children inside the tag. I don't think it works on custom elements.

It might just be easier to remove the binding and use a simple <notification> tag, which has a very similar markup, and is implemented in toolkit.

@@ +12,5 @@
> +
> +        this._setupEventListeners();
> +    }
> +
> +    _setupEventListeners() { }

_setupEventListeners doesn't seem to do anything useful. Can we remove it ?
This doesn't work for me, even if you removed the children tag (what you can do since it wasn'r predefinied explicitely beforhand). Is there anything recent from c-c or m-c needed to make this work (my local tree is about 10 days old)?

For me, none of the subitems defined in lightning-widgets.js are rendered. The only item, that is inside of it is the toolbox defined in [1] that was previously added to |notification-children|

[1] https://searchfox.org/comm-central/source/calendar/lightning/content/imip-bar-overlay.xul#64
Summary: [de-xbl] Convert lightning-notification-bar to custom element → [de-xbl] Convert lightning-notification-bar
Comment on attachment 9003099 [details] [diff] [review]
lightning-notification-bar.patch

Yes let's make this a <notification> instead. That also unifies it to what thunderbird is using normally.
Attachment #9003099 - Attachment is obsolete: true
Attachment #9003099 - Flags: review?(philipp)
Attachment #9003099 - Flags: review?(makemyday)
Summary: [de-xbl] Convert lightning-notification-bar → [de-xbl] Use notification in place of lightning-notification-bar
I tried using notification instead of lightning-notification-bar but it doesn't get rendered. I ll look at it again. Anyone know what I might be doing wrong?
Without seeing what you changed, how could anyone guess?
Attached patch lightning-notification-bar.patch (obsolete) — Splinter Review
Hey, forgot to add the patch while commenting last time. When I run this patch I dont see notification(#imip-bar) bar. It doesn't get rendered. I think this is due to some overlay issue or maybe attributes that arent supported on notification tag. Could you please take a look at it and tell me what's going wrong?

I tried changin lightning-notification-bar element in devtools to notification tag and it rendered correctly.
Flags: needinfo?(mkmelin+mozilla)
Attachment #9005177 - Flags: review?
oops! the patch is not right. I am gonna upload a new one. apologies!
Flags: needinfo?(mkmelin+mozilla)
Comment on attachment 9005177 [details] [diff] [review]
lightning-notification-bar.patch

Don't know what this is?

(And also, please use feedback? for wip patches, not review?)
Attachment #9005177 - Attachment is obsolete: true
Attachment #9005177 - Flags: review?
Attached patch lightning-notification-bar.patch (obsolete) — Splinter Review
updated the patch.
Flags: needinfo?(mkmelin+mozilla)
Well you're going to have to adapt all the code. It's not a direct substitution.

And, it's <notificationbox> you want to be looking at
https://searchfox.org/comm-central/search?q=notificationbox&case=false&regexp=false&path=mail
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/notificationbox
Flags: needinfo?(mkmelin+mozilla)
I think switching to notificationbox is not the appropriate replacement here, since that is a binding, too, and thus subject of converion itself sooner or later and the current binding is just a static extension of hbox, so simply converting the only place it is used to hbox and adding the child nodes is almost enough already.

On top of this, you just need to make sure the label is still set correctly (now to the description directly since xbl:inherit doesn't work anymore) and make sure the css formatting currently assigned to lightning-notification-bar is still applied.
(In reply to [:MakeMyDay] from comment #17)
> I think switching to notificationbox is not the appropriate replacement
> here, since that is a binding, too, and thus subject of converion itself
> sooner or later 

Sure, but it's pretty used in firefox so we don't have to worry about it. (Core is likely to create a replacement.)
Maybe, but almost half of the removals there require to adopt changes in c-c again - and always more or less without a comfortable timeframe to do so. I want to avoid that if possible - at least for calendar.

Apart from that, I wouldn't like to convert this bar into a dismissable bar - I'm not sure offhand if you can avoid that with a notificationbox.
And notificationbox doesn't support toolbarbuttons.
(In reply to [:MakeMyDay] from comment #19)
> Maybe, but almost half of the removals there require to adopt changes in c-c
> again - and always more or less without a comfortable timeframe to do so. I
> want to avoid that if possible - at least for calendar.

Eh, that comfortable time is *now*. That's what this bug is about, right? The minor adjustments later needed for a non-xbl <notificationbox> from core I'd estimate to be trivial.

> Apart from that, I wouldn't like to convert this bar into a dismissable bar
> - I'm not sure offhand if you can avoid that with a notificationbox.

I know you can prevent closing it (return true in the button callback), and css should be able to hide the [x] if desired.

Disadvantages with the current bar:
 - not consistent with the other notification bars e.g.
   * bolded text, others use plain
   * icon size looks to be larger than normal. possibly paddings too
   * background color not consistent with the rest of the notifications
 - placed inconsistently above the message it refers to. We could fix this independently of course too, but I think it should really be in a second notification beneath any remote content notification. At any rate, not before (above) the header of the message it's related to.

So while you could take baby-steps and first do comment 17, I think it is likely to be more time efficient to do the improvements at the same time. You'd get many of them for free and no need to more/adjust css that is likely to be removed anyway.
Attached patch lightning-notification-bar.patch (obsolete) — Splinter Review
I was just experimenting with custom element approach to make lightning-notification-bar patch. Xbl binding used children tag and inherit attr which were not supported by custom elements.. For inherit attribute, we can observe properties/attribute of customElement and for children either slot can be used(if we want placeholders) or just nesting will work.
Attachment #9005180 - Attachment is obsolete: true
Please dont mind the extra code style changes, it was just an experiment patch..
Status: NEW → ASSIGNED
Attached patch lightning-notification-bar.patch (obsolete) — Splinter Review
Attachment #9006829 - Attachment is obsolete: true
Attachment #9014365 - Flags: review?(mkmelin+mozilla)
Summary: [de-xbl] Use notification in place of lightning-notification-bar → [de-xbl] Convert lightning-notification-bar to custom element
Attached patch lightning-notification-bar.patch (obsolete) — Splinter Review
Attachment #9014365 - Attachment is obsolete: true
Attachment #9014365 - Flags: review?(mkmelin+mozilla)
Attachment #9014367 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9014367 [details] [diff] [review]
lightning-notification-bar.patch

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

::: calendar/lightning/content/lightning-widgets.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

4spaces tab here, due to eslint file.
Comment on attachment 9014367 [details] [diff] [review]
lightning-notification-bar.patch

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

::: common/content/customElements.js
@@ +8,4 @@
>  
>  for (let script of [
>    "chrome://messenger/content/mailWidgets.js",
> +  "chrome://lightning/content/lightning-widgets.js",

Shouldn't this be dynamically loaded by the calendar add-on/overlay ?
Comment on attachment 9006829 [details] [diff] [review]
lightning-notification-bar.patch

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

::: calendar/lightning/content/imip-bar-overlay.xul
@@ +312,4 @@
>          </lightning-notification-bar>
>        </vbox>
>      </vbox>
> +

@ntim, did you mean this?
Comment on attachment 9006829 [details] [diff] [review]
lightning-notification-bar.patch

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

::: calendar/lightning/content/imip-bar-overlay.xul
@@ +312,5 @@
>          </lightning-notification-bar>
>        </vbox>
>      </vbox>
> +
> +    <script type="application/javascript"

I meant this.
Comment on attachment 9014367 [details] [diff] [review]
lightning-notification-bar.patch

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

Should be loaded by lightning, not put in /common. Also, be reviewed by a calendar person

::: calendar/lightning/content/imip-bar-overlay.xul
@@ +62,5 @@
> +              -      and "ltnImipBar.executeAction('ACCEPTED');"
> +              //-->
> +            <toolbox id="imip-view-toolbox"
> +                    class="inline-toolbox"
> +                    defaulticonsize="small"

indention is off here

::: calendar/lightning/content/lightning-widgets.js
@@ +13,5 @@
> +        const description = document.createElement("description");
> +        description.setAttribute("class", "msgNotificationBarText");
> +        description.setAttribute("flex", "1");
> +
> +

nit: double empty line
Attachment #9014367 - Flags: review?(mkmelin+mozilla)
Attached patch lightning-notification-bar.patch (obsolete) — Splinter Review
Attachment #9014367 - Attachment is obsolete: true
Attachment #9014402 - Flags: review?(makemyday)
Attached patch lightning-notification-bar.patch (obsolete) — Splinter Review
Attachment #9014402 - Attachment is obsolete: true
Attachment #9014402 - Flags: review?(makemyday)
Attachment #9014407 - Flags: review?(makemyday)
Attachment #9014407 - Flags: feedback?(mkmelin+mozilla)
Comment on attachment 9014407 [details] [diff] [review]
lightning-notification-bar.patch

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

[easier to look at this with diff -uw]

::: calendar/lightning/content/imip-bar-overlay.xul
@@ +169,4 @@
>  
> +                <!-- accept recurrences -->
> +                <toolbarbutton id="imipAcceptRecurrencesButton"
> +                              tooltiptext="&lightning.imipbar.btnAcceptRecurrences2.tooltiptext;"

indention still off here

@@ +313,5 @@
>        </vbox>
>      </vbox>
> +
> +  <script type="application/javascript"
> +          src="chrome://lightning/content/lightning-widgets.js" />

should move to top of the file where the rest of the scripts are

::: calendar/lightning/content/lightning-widgets.js
@@ +19,5 @@
> +
> +        this._description = description;
> +
> +        this.areChildrenAppended = true;
> +

this is an old leftover, please remove

@@ +33,5 @@
> +            return;
> +        }
> +
> +        this._description.textContent =
> +            this.getAttribute("label") || "";

this would fit nicely on just one line
Attachment #9014407 - Flags: feedback?(mkmelin+mozilla) → feedback+
Attached patch lightning-notification-bar.patch (obsolete) — Splinter Review
Attachment #9014407 - Attachment is obsolete: true
Attachment #9014407 - Flags: review?(makemyday)
Attachment #9014735 - Flags: review?(makemyday)
Attachment #9014735 - Flags: feedback+
inline approach for lightning-notification-bar
Attachment #9014943 - Flags: feedback?(mkmelin+mozilla)
Summary: [de-xbl] Convert lightning-notification-bar to custom element → [de-xbl] Remove lightning-notification-bar binding
Attachment #9014943 - Flags: review?(makemyday)
Comment on attachment 9014943 [details] [diff] [review]
lightning-notification-bar_inline.patch

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

Thanks, this looks good in general, but please fix some nits.

::: calendar/lightning/content/imip-bar-overlay.xul
@@ +33,5 @@
> +                       class="msgNotificationBarText"
> +                       flex="1">
> +            &lightning.imipbar.description;
> +          </description>
> +          <box anonid="notification-children">

This additional box is not needed. Leave it out, there's no need to touch the toolbox code here.

::: calendar/lightning/content/imip-bar.js
@@ +6,4 @@
>  const { ltn } = ChromeUtils.import("resource://calendar/modules/ltnInvitationUtils.jsm", null);
>  ChromeUtils.import("resource://gre/modules/Preferences.jsm");
>  
> +const LightningNotificationBar = {

Can you make the name a little shorter like imipBar?

A general note: please add doc blocks if you introduce new objects or functions in calendar code. See http://usejsdoc.org/

@@ +8,5 @@
>  
> +const LightningNotificationBar = {
> +    get notificationBar() {
> +        return document.querySelector(".lightning-notification-bar");
> +    },

You can spare the getter call here and use document.querySelector in the setters directly since this is only used internally.

@@ +14,5 @@
> +        this.notificationBar.querySelector(".msgNotificationBarText")
> +            .textContent = val;
> +    },
> +    set collapsed(val) {
> +        this.notificationBar.setAttribute("collapsed", val);

Make it document.querySelector(".lightning-notification-bar").collapsed = {boolean} instead. You currently have one caller passing in a string and the other a boolean.

@@ +102,5 @@
>              let imipMethod = gMessageDisplay.displayedMessage.getStringProperty("imip_method");
>              cal.itip.initItemFromMsgData(itipItem, imipMethod, gMessageDisplay.displayedMessage);
>  
> +            let imipBar = LightningNotificationBar;
> +            imipBar.collapsed = "false";

No need to create a separate variable here. Also celaan up the collaped call here (see above).

@@ +198,4 @@
>       *                      in subscribed calendars
>       */
>      setupOptions: function(itipItem, rc, actionFunc, foundItems) {
> +        let imipBar = LightningNotificationBar;

Again, no need for a separate variable assignment here.

@@ +393,4 @@
>              return false;
>          }
>  
> +        let imipBar = LightningNotificationBar;

Here again.

@@ +435,5 @@
>                              }
>                          };
>                      } else {
> +                        imipBar.label =
> +                            cal.l10n.getLtnString("imipBarCounterErrorText");

Make this a one-liner. For calendar, we aim for lines not to excceed 100 characters.
Attachment #9014943 - Flags: feedback?(mkmelin+mozilla) → feedback+
Attachment #9014735 - Attachment is obsolete: true
Attachment #9014943 - Attachment is obsolete: true
Attachment #9014735 - Flags: review?(makemyday)
Attachment #9014943 - Flags: review?(makemyday)
Attachment #9015022 - Attachment is obsolete: true
Attachment #9015023 - Attachment is obsolete: true
Attachment #9015024 - Flags: review?(makemyday)
Attachment #9015024 - Attachment is obsolete: true
Attachment #9015024 - Flags: review?(makemyday)
Attachment #9015025 - Flags: review?(makemyday)
Added block comment.
Attachment #9015025 - Attachment is obsolete: true
Attachment #9015025 - Flags: review?(makemyday)
Attachment #9015030 - Flags: review?(makemyday)
Comment on attachment 9015030 [details] [diff] [review]
lightning-notification-bar_inline.patch

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

::: calendar/lightning/content/imip-bar.js
@@ +7,5 @@
>  ChromeUtils.import("resource://gre/modules/Preferences.jsm");
>  
>  /**
> + * This object helps in changing label and collapsed attribute of
> + * lightning notification bar easily.

Probably not the best comment. I ll be happy to change the comment.
Comment on attachment 9015030 [details] [diff] [review]
lightning-notification-bar_inline.patch

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

Thanks, your almost there. r+ with the commit message changed to something meaningful that in brief describes what the patch does and the below mentioned issue fixed (and the ? removed).

N.B.: such a conditional approval means that if you have uploaded a patch with the requested fixes, you can carry forward the r+ yourself and set checkin-needed keyword afterwards on the bug without asking for review again.

::: calendar/lightning/content/imip-bar-overlay.xul
@@ +29,5 @@
> +              insertbefore="msgHeaderView"
> +              label="&lightning.imipbar.description;">
> +          <image anonid="notification-image"/>
> +          <description anonid="notification-description"
> +                       class="msgNotificationBarText"

You can spare the declaration of anonid's for image and description - we don't make use of it.

::: calendar/lightning/content/imip-bar.js
@@ +7,5 @@
>  ChromeUtils.import("resource://gre/modules/Preferences.jsm");
>  
>  /**
> + * This object helps in changing label and collapsed attribute of
> + * lightning notification bar easily.

Maybe

Provides shortcuts to set label and collapse status for the imip-bar node

would be more precise? But I'm also fine with your proposal.
Attachment #9015030 - Flags: review?(makemyday) → review+
I hope removing anonid dont fail tests..
Attachment #9015030 - Attachment is obsolete: true
I hope the commit msg is explanatory.
Attachment #9015032 - Attachment is obsolete: true
Attachment #9015033 - Flags: review+
Attachment #9015033 - Flags: review+
Attachment #9015033 - Attachment is obsolete: true
Attachment #9015034 - Flags: review+
Keywords: checkin-needed
hey please look at the tests failed in the try run before pushing. I dont think this is breaking any test. The linting test fail is because there are not getters in the imipBar object.. if that's important then lemme know.
Flags: needinfo?(jorgk)
Had you rebased, you wouldn't have seen all those test failures. You didn't check the ES job, you have linting errors:
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/calendar/lightning/content/imip-bar.js:12:17 | Getter is not present. (accessor-pairs)
Keywords: checkin-needed
Yes, we can't accept it if linting isn't green.
Flags: needinfo?(jorgk)
Attachment #9015075 - Attachment is obsolete: true
Comment on attachment 9015081 [details] [diff] [review]
lightning-notification-bar_inline_9.patch

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

::: calendar/lightning/content/imip-bar.js
@@ +9,5 @@
>  /**
> + * Provides shortcuts to set label and collapsed attribute of imip-bar node.
> + */
> +const imipBar = {
> +    get bar() {

Sorry makemyday, but to get rid of lint error(No getters), I created this getters. To me it looks ok and better..
Keywords: checkin-needed
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/cf7feed01d0d
Remove lightning-notification-bar binding and inline it. r=MakeMyDay DONTBUILD
Status: ASSIGNED → RESOLVED
Closed: Last year
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 6.6
You need to log in before you can comment on or make changes to this bug.