1213421 - Setting long body text in notification causes text to wrap

Status: RESOLVED FIXED

(Toolkit Graveyard :: Notifications and Alerts, defect)

Description

[Peter deHaan [:pdehaan]]

Attachment: Capture (1).PNG

Steps to reproduce:
0. Use Windows (tested on Win 10).
1. https://people.mozilla.org/~ewong2/push-notification-test/
2. Set a long title and body text.
3. Push "pop Notification" button.

Actual results:
see screenshot.

Expected results:
truncate the text (similar to what we do in OS X).

Comment 1

[Peter deHaan [:pdehaan]]

It looks like if your body string doesn't have linebreaks, then the notification grows very wide and unmanageable.

Similarly, if your title is long, it doesn't truncate or wrap on Windows (where it truncates on OS X).

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

The title now wraps on Windows and Linux as of bug 1201397.

As for truncating, I worry that we will be over-engineering a solution here. The notification was implemented in a way that it expands to show the notification text without overflowing the containers or otherwise looking broken (besides looking somewhat "large").

We should WONTFIX this bug, and rely on websites to provide a proper user experience for their users. Sites like Facebook and Gmail will not place tons of text in their notification because it will not be readable in the timeout provided. As sites need to get permission before notifications can be shown, and the ability to revoke the permission directly from the notification, bad actors are sufficiently dissuaded.

Further, there may be a good use-case for longer than anticipated text within the notification that we may not know of today.

Comment 3

[Matthew N. [:MattN]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)
> and the ability to revoke the permission directly from the
> notification, bad actors are sufficiently dissuaded.

Do we enforce that the popup isn't vertically larger than the screen so that the gear and [X] are both displayed? Spammers have used tricks like that for window.alert(…) before. I think putting some high cap could be useful if websites make mistakes and send un-truncated text and so the controls of at least one notification are fully visible.

If text-overflow:ellipsis just worked on multi-line text then this would be fairly straightforward. A hack with a gradient for a solid background notification could also work.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Truncating to 1000 characters would be fine with me, but this starts getting complicated as we would have to start worrying about various resolutions if we want to guarantee that the gear and [X] are visible on-screen (640x480 for example).

This is not the same problem as window.alert since the website has to get permission first. While true that a bug on the website may cause more text to get displayed than intended, there is very low drive-by probability and the alert auto-closes.

Comment 5

[ewong]

NI: UX for input.

Comment 6

[Edwin Wong [:edwong]]

:phlsa to provide character limits

Comment 7

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

I played around with this for a little and 200 chars seems to be a good character limit for the body that can still largely be read in the time a notification shows up.

The title should just have overflow:ellipsis so that it never shows more than one line.

Comment 9

[Matthew N. [:MattN]]

sec-low from bug 1220519 (probably due to DOS)

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

I didn't add an ellipsis to the truncation of the body text since label[crop=end] doesn't support multiline text and neither does text-overflow:ellipsis.

I didn't add a new string like "%S ..." because we want to get this patch uplifted and this will break string freeze. We can do a follow-up that will not get uplifted but will include the ellipsis using the stringbundle approach.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2

It turns out that nsContextMenu does something similar for the "Seach engineName for '%S...'" string, so I copied that code over to alert.js to put an ellipsis in the body text. See http://mxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#1782

I tested the context menu code in a Hebrew build and confirmed that it works with RTL text.

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 8684949 [details] [diff] [review]
Patch v2

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

::: toolkit/components/alerts/resources/content/alert.js
@@ +10,5 @@
>  const NS_ALERT_TOP = 4;
>  
>  const WINDOW_MARGIN = 10;
>  
> +const BODY_TEXT_LIMIT = 200;

Nit: group this with WINDOW_MARGIN and sort alphabetically since I don't think this will get grouped with anything related.

@@ +108,5 @@
> +          // If the JS character after our truncation point is a trail surrogate,
> +          // include it in the truncated string to avoid splitting a surrogate pair.
> +          let truncLength = BODY_TEXT_LIMIT;
> +          let truncChar = bodyText[BODY_TEXT_LIMIT].charCodeAt(0);
> +          if (truncChar >= 0xDC00 && truncChar <= 0xDFFF) {

Seems like this would be nice in a helper but I don't know where would be good off-hand. At a minimum there should be a comment that this was copied from nsContextMenu and ideally a comment there pointing here too.

@@ +115,5 @@
> +          bodyTextLabel.textContent = bodyText.substring(0, BODY_TEXT_LIMIT) +
> +                                      ellipsis;
> +        }
> +
> +        if (bodyText.length > BODY_TEXT_LIMIT) {

Why not fold this into the `if` body above for the same condition?

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/697fb66c4494f8df72f4e8e5e97d7b842e0a0cfe
Bug 1213421 - Truncate long body and title text in notifications. r=MattN

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/3d8143254eac173858c97f6e0d3ae9a48be3dcbc
Bug 1213421 - Follow-up, set the bodyText even when text is not being truncated. r=MattN

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Follow-up

I just noticed this in my local build. Pushed it so it wouldn't break the next weekday Nightly build.

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/697fb66c4494
https://hg.mozilla.org/mozilla-central/rev/3d8143254eac

Comment 17

[Matthew N. [:MattN]]

Comment on attachment 8687566 [details] [diff] [review]
Follow-up

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

We should probably have a simple test for this (in a follow-up) if we don't already.

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Follow-up with tests for title, body, and image

Wasn't sure if you wanted a separate bug, but the patch is quite simple so I just kept it in this bug.

Comment 19

[Matthew N. [:MattN]]

Comment on attachment 8689181 [details] [diff] [review]
Follow-up with tests for title, body, and image

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

It's okay here since the first part wasn't uplifted yet. Can you request uplift on both?

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/24c5d435977da94d3986c89085496d8ddef6f1b2
Bug 1213421 - Add a test that verifies the body text of a notification is present. r=MattN

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Roll-up patch for Aurora44

Approval Request Comment
[Feature/regressing bug #]: new feature work for push/desktop notifications
[User impact if declined]: push notifications will look too large if sites send too much text for them
[Describe test coverage new/current, TreeHerder]: automated tests and manual testing
[Risks and why]: low risk, been on nightly for a few days now with heavy QA testing
[String/UUID change made/needed]: none

Comment 22

[Carsten Book [:Tomcat]]

sorry had to back this out for surfacing leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=5811620&repo=fx-team that started with this push

Comment 23

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/9a49075671cb

Comment 24

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8689500 [details] [diff] [review]
Roll-up patch for Aurora44

Please re-nominate for uplift once the reasons for backout from fx are fixed and addressed.

Comment 25

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/d516d82f29a2636c5834c7f3c932025b8e85a8ad
Bug 1213421 - Add a test that verifies the text of a notification is present. r=MattN

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d516d82f29a2

Comment 27

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Roll-up patch for Aurora44 v2

Approval Request Comment
[Feature/regressing bug #]: new feature work for push/desktop notifications
[User impact if declined]: push notifications will look too large if sites send too much text for them
[Describe test coverage new/current, TreeHerder]: automated tests and manual testing
[Risks and why]: low risk, been on nightly for a few days now with heavy QA testing
[String/UUID change made/needed]: none

Comment 28

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8691980 [details] [diff] [review]
Roll-up patch for Aurora44 v2

Push notifications is planned for FF44 and this fix has been in Nightly for a few days now, seems safe to uplift to Aurora44.

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ea94de8a09f0

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/ea94de8a09f0