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

Status: RESOLVED FIXED

(Calendar :: General, enhancement)

Comment 1

[Arshad Khan [:arshad]]

Attachment: lightning-notification-bar.patch

Comment 2

[Arshad Khan [:arshad]]

Few formatting changes has sneaked in. I ll remove them in next patch.

Comment 3

[[:MakeMyDay]]

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.

Comment 4

[Arshad Khan [:arshad]]

Attachment: lightning-notification-bar.patch

Comment 5

[Arshad Khan [:arshad]]

Attachment: lightning-notification-bar.patch

Comment 6

[Arshad Khan [:arshad]]

Attachment: lightning-notification-bar.patch

Comment 7

[Tim Nguyen :ntim]

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 ?

Comment 8

[[:MakeMyDay]]

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

Comment 9

[Magnus Melin [:mkmelin]]

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.

Comment 10

[Arshad Khan [:arshad]]

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?

Comment 11

[Magnus Melin [:mkmelin]]

Without seeing what you changed, how could anyone guess?

Comment 12

[Arshad Khan [:arshad]]

Attachment: lightning-notification-bar.patch

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.

Comment 13

[Arshad Khan [:arshad]]

oops! the patch is not right. I am gonna upload a new one. apologies!

Comment 14

[Magnus Melin [:mkmelin]]

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

Comment 15

[Arshad Khan [:arshad]]

Attachment: lightning-notification-bar.patch

updated the patch.

Comment 16

[Magnus Melin [:mkmelin]]

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

Comment 17

[[:MakeMyDay]]

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.

Comment 18

[Magnus Melin [:mkmelin]]

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

Comment 19

[[:MakeMyDay]]

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.

Comment 20

[Richard Marti (:Paenglab)]

And notificationbox doesn't support toolbarbuttons.

Comment 21

[Magnus Melin [:mkmelin]]

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

Comment 22

[Arshad Khan [:arshad]]

Attachment: lightning-notification-bar.patch

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.

Comment 23

[Arshad Khan [:arshad]]

Please dont mind the extra code style changes, it was just an experiment patch..

Comment 24

[Arshad Khan [:arshad]]

Attachment: lightning-notification-bar.patch

Comment 25

[Arshad Khan [:arshad]]

Attachment: lightning-notification-bar.patch

Comment 26

[Arshad Khan [:arshad]]

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 27

[Tim Nguyen :ntim]

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 28

[Arshad Khan [:arshad]]

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 29

[Arshad Khan [:arshad]]

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 30

[Magnus Melin [:mkmelin]]

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

Comment 31

[Arshad Khan [:arshad]]

Attachment: lightning-notification-bar.patch

Comment 32

[Arshad Khan [:arshad]]

Attachment: lightning-notification-bar.patch

Comment 33

[Magnus Melin [:mkmelin]]

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

Comment 34

[Arshad Khan [:arshad]]

Attachment: lightning-notification-bar.patch

Comment 35

[Arshad Khan [:arshad]]

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

Comment 36

[Arshad Khan [:arshad]]

Attachment: lightning-notification-bar_inline.patch

inline approach for lightning-notification-bar

Comment 37

[Arshad Khan [:arshad]]

lightning-notification-bar-inline - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=78ed224d7427220c5d757ddc7abdbc90d6786bd2

Comment 38

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

Comment 39

[Arshad Khan [:arshad]]

Attachment: lightning-notification-bar_inline_1.patch

Comment 40

[Arshad Khan [:arshad]]

Attachment: lightning-notification-bar_inline.patch

Comment 41

[Arshad Khan [:arshad]]

Attachment: lightning-notification-bar_inline.patch

Comment 42

[Arshad Khan [:arshad]]

Attachment: lightning-notification-bar_inline_3.patch

Comment 43

[Arshad Khan [:arshad]]

lightning-notification-bar-inline_3.patch - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=978c563c7ec43f794294e0651452c341a1ae75cf

Comment 44

[Arshad Khan [:arshad]]

Attachment: lightning-notification-bar_inline.patch

Added block comment.

Comment 45

[Arshad Khan [:arshad]]

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 46

[[:MakeMyDay]]

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.

Comment 47

[Arshad Khan [:arshad]]

I hope removing anonid dont fail tests..

Comment 48

[Arshad Khan [:arshad]]

Attachment: lightning-notification-bar_inline.patch

Comment 49

[Arshad Khan [:arshad]]

Attachment: lightning-notification-bar_inline_5.patch

I hope the commit msg is explanatory.

Comment 50

[Arshad Khan [:arshad]]

Attachment: lightning-notification-bar_inline_6.patch

Comment 51

[Arshad Khan [:arshad]]

lightning-notification-bar_inline_6.patch - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=40219f2fdb571a5db561777bd4756dd30df95ce6

Comment 52

[Arshad Khan [:arshad]]

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.

Comment 53

[Jorg K (CEST = GMT+2)]

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)

Comment 54

[Jorg K (CEST = GMT+2)]

Yes, we can't accept it if linting isn't green.

Comment 55

[Arshad Khan [:arshad]]

Attachment: lightning-notification-bar_inline_7.patch

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

Comment 56

[Arshad Khan [:arshad]]

Attachment: lightning-notification-bar_inline_8.patch

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

Comment 57

[Arshad Khan [:arshad]]

Attachment: lightning-notification-bar_inline_9.patch

Comment 58

[Arshad Khan [:arshad]]

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

Comment 59

[Arshad Khan [:arshad]]

lightning-notification-patch-inline_9 : https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=686f2b2c91e8d566b68de8369a3c71a2313a86c0

Comment 60

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/cf7feed01d0d
Remove lightning-notification-bar binding and inline it. r=MakeMyDay DONTBUILD