Surgeon general's warning: Excessively long text in notifications may be hazardous to your screen real estate

ASSIGNED
Unassigned

Status

()

ASSIGNED
8 years ago
2 years ago

People

(Reporter: WeirdAl, Unassigned)

Tracking

1.9.2 Branch
x86
Windows XP
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

8 years ago
See attachment.  710 pixels wide, for a notification box!
(Reporter)

Comment 1

8 years ago
Created attachment 453870 [details]
screenshot of notification box, 710 px wide
(Reporter)

Updated

8 years ago
Version: unspecified → 3.1

Comment 2

8 years ago
I like that summary title. ;-)

That's probably bug 123440, which introduced the slide-in notification to avoid modal dialogs. Maybe there is some way to wrap this to some maximum width?

Comment 4

8 years ago
Ok, trying the naive approach didn't work out. I've simply set a max-width for .alertText (and .alertTextBox, and #alertNotification), but it would restrict the size of the slide-in window. It's the right class, a display: none will hide the string completely.

The issue is that in the neighboring alert.js height and width of the box are explicitly adjusted (line 138, sizeToContent) and apparently override any CSS definition. So, that's probably where a maximum width has to be ensured.

The other question is: Which maximum width is appropriate? A fixed pixel value, a percentage of the window width? Make it a constant or rather a preference setting, and which value or which default?

Comment 5

8 years ago
(In reply to comment #4)
> .alertText (and .alertTextBox, and #alertNotification), but it would restrict

s/would/&n't/ of course.
(Reporter)

Updated

8 years ago
Component: Mail Window Front End → XUL Widgets
Product: Thunderbird → Toolkit
QA Contact: front-end → xul.widgets
Version: 3.1 → 1.9.2 Branch
(Reporter)

Comment 6

8 years ago
Moving to Toolkit > XUL Widgets, as the alerts service does appear to be in the Toolkit's domain.

Comment 7

8 years ago
The https://developer.mozilla.org/en/XUL_Tutorial/Adding_Labels_and_Images may hold the clue why the label won't wrap. It is added as a value="" attribute in alert.js, which by that definition prohibits wrapping. Apparently the content would have to be added in a text node below <label> along with a max-width CSS definition in order to enforce wrapping beyond a certain size.

Comment 8

8 years ago
Possible idea to implement this - in alert.js, replace (line #83):
> - document.getElementById('alertTextLabel').setAttribute('value', window.arguments[2]);
> + document.getElementById('alertTextLabel').childNodes[0].nodeValue = window.arguments[2];

Which would likely require to have a dummy text node [0] in alert.xul (#60):
> - <label id="alertTextLabel" class="alertText plain"/>
> + <label id="alertTextLabel" class="alertText plain">&nbsp;</label>

Then somehow add the maximum-width restriction dynamically or per CSS.
(Reporter)

Comment 9

8 years ago
Created attachment 456392 [details] [diff] [review]
first pass

I had to make an additional change in order to make this testable.  I set the alert window as the subject of the observer notification.  There may be other, better ways to do that.

The mochitest failed in my debug build, because the alert window didn't close within 15 seconds.  I'm not sure what to do about that... Shawn, what do you think?
Attachment #456392 - Flags: review?(sdwilsh)
Comment on attachment 456392 [details] [diff] [review]
first pass

It's a little odd returning the alert window, but I think it doesn't hurt (and is useful for testing).

r=sdwilsh
Attachment #456392 - Flags: review?(sdwilsh) → review+

Updated

8 years ago
Assignee: nobody → ajvincent
Status: NEW → ASSIGNED
(Reporter)

Comment 11

8 years ago
Comment on attachment 456392 [details] [diff] [review]
first pass

Requesting super-review.  Please note this patch causes a Mochitest failure which can be fixed in a few different ways, but I'm not sure what to do about it.

I see a few possibilities:
(1) Extend the time limit
(2) Remove the time limit
(3) Bring the alert window in faster, and make it fade away faster.

There's also the not-so-obvious question of a really, really long text sent to the alerts service.  I made one case tolerable in this patch, but suppose the text were thirty to fifty times the length of this message?  Then you have an alert window as tall or taller than the main window itself - and text so long no one can possibly read it all before it goes away.
Attachment #456392 - Flags: superreview?(robert.bugzilla)

Comment 12

8 years ago
To comment on your last paragraph: If the message is large enough that it
would fill the entire screen, it's probably impossible to read it within the
few seconds it slides in and out anyway... ;-)

Comment 13

8 years ago
^ meaning, with or without your patch, won't make a difference.
(Reporter)

Comment 14

8 years ago
(In reply to comment #12)
> To comment on your last paragraph: If the message is large enough that it
> would fill the entire screen, it's probably impossible to read it within the
> few seconds it slides in and out anyway... ;-)

That's my point:  it's an unsolved problem.
(In reply to comment #11)
> Requesting super-review.  Please note this patch causes a Mochitest failure
> which can be fixed in a few different ways, but I'm not sure what to do about
> it.
Er, how is it timing out?

(In reply to comment #14)
> That's my point:  it's an unsolved problem.
I wouldn't worry about it here.
(Reporter)

Comment 16

8 years ago
Comment on attachment 456392 [details] [diff] [review]
first pass

>+++ b/toolkit/components/alerts/test/test_long_alerts.html
>+const kTimeoutSeconds = 15;
>+function reaper() {
>+  ok(false,"Observer is notified within " + kTimeoutSeconds + " seconds")
>+  SimpleTest.finish();
>+}
>+
>+const Cc = Components.classes;
>+const Ci = Components.interfaces;
>+try {
>+  SimpleTest.waitForExplicitFinish();
>+  gReaperTimeoutId = setTimeout(reaper, kTimeoutSeconds * 1000);

The observer that would clear gReaperTimeoutId didn't fire soon enough, Shawn.  That's what I mean.
(In reply to comment #16)
> The observer that would clear gReaperTimeoutId didn't fire soon enough, Shawn. 
> That's what I mean.
Hmm, can't we just rely on the test harness timing out if it doesn't notify?
(Reporter)

Comment 18

8 years ago
(In reply to comment #17)
> Hmm, can't we just rely on the test harness timing out if it doesn't notify?

Honestly, I do not know.  I'm not sure how long SimpleTest.waitForExplicitFinish means to wait for.
15 seconds, I think, which should be plenty long enough for this test.

Comment 20

8 years ago
Hi! I'd like to add one aspect to this bug (that hopefully fits in here): I use my taskbar vertically on the right side of my screen (which is not uncommon for sub-notebook users, afaik). For this configuration, the slide-in notification slides in _horizontally_ from the right side. It seems to use the same speed as for vertical scrolling, but since it is much longer in this direction (especially with long texts), moving in and out takes very long. Since I get those messages quite frequently due to a bad connection, and since they cover roughly 1/2 of the free screen's width, it is quite annoying.

Some ideas for resolving: make it faster for horizontal slide direction / slide in vertically regardless of taskbar position / option to disable slide animation (just fade in-fade out, like new-mail notification does) / option to disable those notifications completely without editing config files, see http://kb.mozillazine.org/Thunderbird_3.1_-_New_Features_and_Changes#Connection_Notifications .
Comment on attachment 456392 [details] [diff] [review]
first pass

>diff --git a/toolkit/components/alerts/resources/content/alert.js b/toolkit/components/alerts/resources/content/alert.js
>--- a/toolkit/components/alerts/resources/content/alert.js
>+++ b/toolkit/components/alerts/resources/content/alert.js
>@@ -217,18 +219,18 @@ function animateCloseAlert()
>     setTimeout(animateCloseAlert, gSlideTime);
>   }
>   else
>     closeAlert();
> }
> 
> function closeAlert() {
>   if (gAlertListener)
>-    gAlertListener.observe(null, "alertfinished", gAlertCookie); 
>+    gAlertListener.observe(window, "alertfinished", gAlertCookie); 
>   window.close(); 
> }
> 
> function onAlertClick()
> {
>   if (gAlertListener && gAlertTextClickable)
>-    gAlertListener.observe(null, "alertclickcallback", gAlertCookie);
>+    gAlertListener.observe(window, "alertclickcallback", gAlertCookie);
I once wrote a test for the alert service that just used a WindowOpenListener to get and test the state of an alert Window... though it might be useful I don't see changing the API for this.

Also, please verify that the tests behaves properly on Mac OS X, etc. since it uses a native implementation iirc.

>diff --git a/toolkit/components/alerts/test/test_long_alerts.html b/toolkit/components/alerts/test/test_long_alerts.html
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/alerts/test/test_long_alerts.html
I think this should be renamed to test_wide_alerts.html since long can be confused with length of time and other lengths.

>@@ -0,0 +1,69 @@
>+<!DOCTYPE HTML>
>+<html>
>+<head>
>+  <title>Test for Alerts Service</title>
>+  <script type="text/javascript" src="/MochiKit/packed.js"></script>
>+  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script> 
>+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
>+</head>
>+<body>
>+<p id="display"></p>
>+
>+Alerts service mochitest<br/>
>+
>+Did an alert appear anywhere?<br/>
>+If so, the test will finish once the alert disappears. If not, the test will time out.<br/>
>+
>+<pre id="test">
>+<script class="testbody" type="text/javascript">
>+netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
>+
>+var gReaperTimeoutId;
>+var observer = {
Use a WindowOpenListener instead.

>+  observe: function (aSubject, aTopic, aData) {
>+    if (aTopic != "alertclickcallback") // Did someone click the alert while running mochitests?...
>+      is(aTopic, "alertfinished", "Checking the topic for a finished notification");
>+
Seems strange to continue for the case you are protecting for here. Perhaps an early return?

>+    is(aData, "foobarcookie", "Checking whether the alert cookie was passed correctly");
>+
>+    netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
>+    ok(aSubject instanceof Ci.nsIDOMWindowInternal,
>+       "We should have a window's width to measure");
>+    ok(aSubject.outerWidth < 600, "The window should not be excessively wide.");
I understand wanting a bit of slack for the test but 600 is way larger than the 400 you set in the css... can't you get the textbox and check against its width?

>diff --git a/toolkit/themes/gnomestripe/global/alerts/alert.css b/toolkit/themes/gnomestripe/global/alerts/alert.css
>--- a/toolkit/themes/gnomestripe/global/alerts/alert.css
>+++ b/toolkit/themes/gnomestripe/global/alerts/alert.css
>@@ -76,16 +76,20 @@ label {
>   cursor: inherit;
> }
> 
> .alertText[clickable="true"] {
>   color: -moz-nativehyperlinktext;
>   text-decoration: underline;
> }
> 
>+.alertTextBox {
>+  max-width: 400px;
>+}
>+
Please check with UX for the optimal max-width. faaborg should be able to provide this.

sr=me with the above addressed.
Attachment #456392 - Flags: superreview?(robert.bugzilla) → superreview+
Alex Faaborg (to prevent confusion with Alex Vincent),

We need a max-width for alerts and I was hoping you could help out. Thanks
you mean the toast alerts, right?  I would try to keep those panels under 320 wide, including an icon on the left side.
Keywords: uiwanted
(Reporter)

Comment 24

8 years ago
Created attachment 461700 [details] [diff] [review]
patch, v2 (test still doesn't verify the right thing)

Updated patch to sr comments.  Unfortunately, I find the test for window size fires when the window first appears, not when it reaches its maximum size.

I haven't yet tested on the Mac, as requested by sr.

Help wanted.
Attachment #456392 - Attachment is obsolete: true
(Reporter)

Comment 25

8 years ago
Created attachment 461722 [details] [diff] [review]
patch, v2.1

Problem solved, using nsITimer.
(Reporter)

Updated

8 years ago
Attachment #461700 - Attachment is obsolete: true
(Reporter)

Comment 26

8 years ago
Comment on attachment 461722 [details] [diff] [review]
patch, v2.1

re-requesting sr, as the patch changed significantly since last sr+.
Attachment #461722 - Flags: superreview?(robert.bugzilla)
(Reporter)

Comment 27

8 years ago
Regarding the Mac OS question:  I just reran the test on 10.5, and it says the alerts service does not exist.  Running test_alerts.html, which I did not modify, says the same thing.  I don't think there's anything I can do about that. :)
If you don't have growl installed, that will be the case.  If you do, it'll fail every time because we don't have a DOM window.
(Reporter)

Comment 29

8 years ago
Comment on attachment 461722 [details] [diff] [review]
patch, v2.1

Per rs's request, transferring super-review request to neil.

Background:  This patch has received r+, sr+ before, but there were enough changes based on sr comments that I felt a second sr pass was justifiable.
Attachment #461722 - Flags: superreview?(robert.bugzilla) → superreview?(neil)
Comment on attachment 461722 [details] [diff] [review]
patch, v2.1

I did see one bug, and as yet I have no ideas as to what the cause/fix is:

If you set the pref ui.alertNotificationOrigin to 4, then the alert is supposed to slide in from the top. But with the patch, it unfolds. There is actually a nice big comment in alert.js warning about this!

>+      document.getElementById('alertTextLabel')
>+              .appendChild(document.createTextNode(window.arguments[2]));
Use .textContent, it's much easier.

>diff --git a/toolkit/themes/gnomestripe/global/alerts/alert.css b/toolkit/themes/gnomestripe/global/alerts/alert.css
I don't like the idea of repeating the max-width in each theme. Can you not set it directly in the XUL?

>+  max-width: 280px;
[Personally I would have gone for 29em, because that's what's used in  commonDialog.xul, but that makes testing too difficult!]
(Reporter)

Comment 31

8 years ago
Created attachment 465368 [details] [diff] [review]
patch, v2.2

updated to reflect Neil's comments.

> If you set the pref ui.alertNotificationOrigin to 4, then the alert is supposed
> to slide in from the top. But with the patch, it unfolds. There is actually a
> nice big comment in alert.js warning about this!

I don't know how that happens here either.  The setting of a max-width shouldn't affect that.

I do see the effect, though.  Possible layout bug that this uncovers?
Attachment #461722 - Attachment is obsolete: true
Attachment #465368 - Flags: superreview?(neil)
Attachment #461722 - Flags: superreview?(neil)
Comment on attachment 465368 [details] [diff] [review]
patch, v2.2

>+    <vbox id="alertTextBox" class="alertTextBox" maxwidth="280px">
Nit: attributes don't take units, they're always pixels.

I think that unfolding problem is a layout bug of some sort, but I'm not sure it would be a good idea to take this patch without at least a workaround.
And the workaround is (drumroll please):

Use script to set the alertBox's minHeight to its actual height.
Comment on attachment 465368 [details] [diff] [review]
patch, v2.2

Minusing as per comments 30-33.
Attachment #465368 - Flags: superreview?(neil) → superreview-
(Reporter)

Comment 35

6 years ago
I'm not sure this bug still applies... haven't we already truncated alert box lengths?
Assignee: ajvincent → nobody
Bulk move to Toolkit::Notifications and Alerts

Filter on notifications-and-alerts-component.
Component: XUL Widgets → Notifications and Alerts

Comment 37

2 years ago
Created attachment 8776185 [details]
notification.mp4

The notification window increases horizontally when longer text without space is added in the body field.

Steps to Reproduce:
1. Go to https://people.mozilla.org/~ewong2/push-notification-test/
2. Add 70 characters without space in the body field.
3. Click on [Pop Notification] button
4. Accept permissions (if you did not already) and check the notification window size

Actual Result:
The more text is added in to the body field, the notification window grows horizontally. 

Expected Result:
Text should wrap and notification window should grow only vertically.
You need to log in before you can comment on or make changes to this bug.