797690 - Add a notificationbox to the Event dialog

Status: RESOLVED FIXED

(Calendar :: Dialogs, defect)

Description

[Philipp Kewisch [:Fallen][☁️📆]]

We have a few potential error messages we are currently using popup dialogs for, it would be nice to use a notificationbox for this instead.

https://developer.mozilla.org/en-US/docs/XUL/notificationbox

Comment 1

[Markus Adrario [:Taraman]]

What is the status on this bug?

I am just about to implement a notificationbox for bug 160821
If you already have worked on this, maybe I can use an existing box - so we don't do double work.

Else, I can implement the notification box and you can work on the conversion of the messages.

Comment 2

[Markus Adrario [:Taraman]]

Maybe [1] is a candidate for the notificationbox.

[1]: http://mxr.mozilla.org/comm-central/source/calendar/base/src/calIcsParser.js#234

Comment 3

[Markus Adrario [:Taraman]]

Attachment: Patch V1

This Patch adds a notificationBox at the top of the dialog as well as functions to show and remove notifications.

Comment 4

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 792369 [details] [diff] [review]
Patch V1

I think you want JosiahOne to review this.

Comment 5

[Markus Adrario [:Taraman]]

Thanks for the hint.

We should update the calendar module-owner page...

Comment 6

[Josiah Bruner [:jsbruner]]

Actually I'm not related to Calendar in any way, I work in /mail primarily. If no one else if more capable I could do the ui-review, but please upload a screenshot of the UI, since I don't build Lightning at all.

Comment 7

[Josiah Bruner [:jsbruner]]

is more*

Comment 8

[Josiah Bruner [:jsbruner]]

So apparently this behavior is expected. indexOf is only checking the index of a specific tab type.

Therefore closing as INVALID.

Comment 9

[Richard Marti (:Paenglab)]

Josiah, you took the wrong bug.

Comment 10

[Josiah Bruner [:jsbruner]]

Gar, stupid Bugzilla always looking the same. Sorry about that guys.

Comment 11

[Markus Adrario [:Taraman]]

Whoa, you made me jump in the morning. ;-)
The screenshot will take a moment because of bug 906344. I don't have a running build at the moment.

Comment 12

[Markus Adrario [:Taraman]]

Attachment: Screenshot of the notificationBox

Comment 13

[Josiah Bruner [:jsbruner]]

Comment on attachment 793472 [details]
Screenshot of the notificationBox

So I have two concerns with this. Plus the fact that I have never used this before helps with a first-time user perspective. 

1. "item" seems inconsistent, it doesn't really explain what we want the user to change. Perhaps switch to "event" or something similar. 

2. You tell the user to use the "custom rules", but you do indicate *what* those rules are. Could you please point tell the user where they would set this. For example, I have absolutely no idea by just looking at this where I would go to do what the message suggests. 


Lastly, I assume this message pop-up does *not* move the rest of the window contents downward. If it does, please change that as well to an overlap.

Comment 14

[Josiah Bruner [:jsbruner]]

Oh wait. Looks like the message content is from another bug. In that case as long as it doesn't move the window contents I'm fine with it.

Comment 15

[Markus Adrario [:Taraman]]

You are right, the text is from another bug and just as an example, since this bug just adds the box.
But I will take yout comments into account for that other bug.

And, yes, it moves the content down, resizing the description box.
I had in mind not to overlay the title-field, but thinking about it, I see no problem in that, since the user will click it away anyway when he has read it.

SO I will change that.

Comment 16

[Markus Adrario [:Taraman]]

Now I need some help.

How do I prevent the notification from moving the rest of the content.

My Idea was a <stack>, but if I place the <notificationbox> on top of that stack, it prevents all clicks from reaching the controls, even if it is not shown.
The dialog is still controllable with keys though.

Can anyone point me to a solution?

Comment 17

[Josiah Bruner [:jsbruner]]

You'll likely have to link it to some CSS and add the 'position: absolute;' rule.

Comment 18

[Markus Adrario [:Taraman]]

Unfortunately this just makes the grid static without any resizing, but it still moves down when the notification appears.

I also had a look at Firefox' global notification bar, which I thought has this behaviour, but also found no clue. :-(

Comment 19

[Markus Adrario [:Taraman]]

Attachment: Patch V1.1

mousethrough="always" on the notificationbox did the trick.
Thx philipp.

This made the patch rather big, because of the indentation changes.
I also used this to fix the indentation in some places where only 2 spaces ISO 4 were used.

the only optical change is that the box now overlaps the "title" field in the dialog, so I don't attach a new screenshot.

Comment 20

[Josiah Bruner [:jsbruner]]

I would still prefer to see another screenshot if possible. Basically it would be good to know how much of the title field it covers? All of it? Half?

Comment 21

[Markus Adrario [:Taraman]]

Attachment: Screenshot of the notificationBox

Comment 22

[Josiah Bruner [:jsbruner]]

Sorry to make things this difficult, but I'm really not liking that any better. Since it completely covers the field, the user now has no real idea on where their title field went...

I tried thinking of alternatives but none really where any more helpful:

A. Moving the notification to the bottom of the window. That would help, except now the user probably won't notice it. So that's out.

B. The other choice was making the notification's background semi-transparent, so the user can see the field, but that makes the text hard to read. Also out.

So we're back to what you had before. Which I guess will have to work.

So please switch back to the version that shifts content downward, but also animate the sliding of the window content and the expanding of the notification. Essentially do exactly what the add-on manager does in TB when you download an add-on.

If the timing is equal, everything should slide elegantly, which will keep users from getting annoyed by a jarring shift.

Thanks!

Comment 23

[Markus Adrario [:Taraman]]

Attachment: Patch V1.2

That is now exactly the way it was. ;-)
The animation is the standard behaviour of the notificationbox.

I'll mail you a .xpi for nightly with two added buttons in the dialog to test the notification.
So you can get a better Idea what happens - should have done that right away...

Comment 24

[Josiah Bruner [:jsbruner]]

Comment on attachment 793961 [details] [diff] [review]
Patch V1.2

Looks good. Thanks! The add-on was very helpful.

Comment 25

[Philipp Kewisch [:Fallen][☁️📆]]

Comment on attachment 793961 [details] [diff] [review]
Patch V1.2

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

I like the patch, short and concise. r=philipp

::: calendar/base/content/dialogs/calendar-event-dialog.js
@@ +2161,5 @@
> +            case "critical":
> +                priority = notificationBox.PRIORITY_CRITICAL_MEDIUM;
> +                break;
> +            default:
> +                priority = notificationBox.PRIORITY_WARNING_MEDIUM;

If you like, you could condense this a bit:

const prioMap = {
  "info": notificationBox.PRIORITY_INFO_MEDIUM,
  "critical": notificationBox.PRIORITY_CRITICAL_MEDIUM
};
let priority = prioMap[aPriority] || notificationBox.PRIORITY_WARNING_MEDIUM;

Comment 26

[Markus Adrario [:Taraman]]

Attachment: Patch V1.3

I put in the proposed change from philipp and added the commit-message.

Do I get the approval here, or should I mark checkin-needed?

Comment 27

[Josiah Bruner [:jsbruner]]

(In reply to Markus Adrario [:Taraman] from comment #26)
> Created attachment 794750 [details] [diff] [review]
> Patch V1.3
> 
> I put in the proposed change from philipp and added the commit-message.
> 
> Do I get the approval here, or should I mark checkin-needed?

checkin-needed is fine. Someone will eventually land this for you then.

Thanks Markus!

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/a18d0ae56b8a