615481 - Doorhanger (popup) notifications need a maximum width and the text needs to wrap if necessary

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Alex Faaborg [:faaborg] (Firefox UX)]

The IndexedDB prompts are getting rather wide (due to the longer sentence not wrapping).  We want to keep the icon and action button in the same glance-able area, so we should have a max width of 360 pixels (with the dialog getting taller if text wraps).

Comment 2

[:Margaret Leibovic]

Attachment: patch

To make the description not wrap, I had to set the textContent instead of the value. However, I couldn't get that to work with anonymous content, so I just made the description one of the dynamically generated elements.

This patch also fixes the alignment of the geolocation "Learn More..." link on OSX, which managed to break from some of the other doorhanger styling patches. I figured this was an ok place to throw in this fix, since I need to get rid of the toolkit margin styles on the description anyway.

Comment 3

[:Margaret Leibovic]

Attachment: patch v2

Magically simpler patch that uses xbl:text.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 494133 [details] [diff] [review]
patch v2

>diff --git a/browser/themes/winstripe/browser/browser.css b/browser/themes/winstripe/browser/browser.css

>+.popup-notification-description {
>+  max-width: 266px;

Don't think this change is needed (duplicates the rule in notification.css)

>diff --git a/toolkit/themes/pinstripe/global/notification.css b/toolkit/themes/pinstripe/global/notification.css

> .popup-notification-description {
>-  margin-bottom: 10px;
>-  -moz-margin-start: 5px;

This seems to break the alignment of the learn more link:
http://grab.by/7EWx (patch as is)
http://grab.by/7EWv (patch without this change)

Where does the 266px value come from?

Comment 5

[:Gavin Sharp [email: [e-mail]]]

The learn more changes could be moved over to bug 615483.

Comment 6

[:Margaret Leibovic]

(In reply to comment #4)
> Comment on attachment 494133 [details] [diff] [review]
> patch v2
> 
> >diff --git a/browser/themes/winstripe/browser/browser.css b/browser/themes/winstripe/browser/browser.css
> 
> >+.popup-notification-description {
> >+  max-width: 266px;
> 
> Don't think this change is needed (duplicates the rule in notification.css)

My bad. I just missed that when I was updating my patch.

> >diff --git a/toolkit/themes/pinstripe/global/notification.css b/toolkit/themes/pinstripe/global/notification.css
> 
> > .popup-notification-description {
> >-  margin-bottom: 10px;
> >-  -moz-margin-start: 5px;
> 
> This seems to break the alignment of the learn more link:
> http://grab.by/7EWx (patch as is)
> http://grab.by/7EWv (patch without this change)

I think a -moz-margin-start: -5px; rule should be added to the learn more link style to fix this, since it doesn't seem to make sense to add margin to the description for all of the notifications (and we don't add that margin in the other themes). However, this won't matter when we implement bug 615483, so I guess we can just leave it for now.

> Where does the 266px value come from?

In the bug description, Alex said that the notifications should have a max width of 360px. I got 266px = 360px - 20px (#notification-popup padding) - 64px (.popup-notification-icon width) - 10px (.popup-notification-icon padding). However, I just realized that there are also padding rules on the arrow panels themselves, and the #notification-popup padding is just in the winstripe theme, so 266px is probably wrong.

Alex, what do you think the max-width on the description should be?

Comment 7

[Alex Faaborg [:faaborg] (Firefox UX)]

>Alex, what do you think the max-width on the description should be?

how about 248, with 16 pixels of padding on either side of the icon, and to the right of the text/action button.

so:

16-64-16-248-16

Comment 8

[:Margaret Leibovic]

Attachment: patch v3

I was wrong about the #notification-popup padding in browser.css. That padding applies to the arrow panel, and it's going to be removed by bug 606343. The padding inside the panel is determined by the padding rule on the arrow panel content, so I updated that from 14px to 16px in popup.css (the styles don't exist for gnomestripe yet; that should be done in bug 604257).

The existing -moz-margin-end rule on the icon and the default -moz-margin-start rule on the description add up to 16px, so no changes are necessary to make that space correct.

I still got rid of the margin rules for .popup-notification-description in the pinstripe notification.css because they seem to just be there for the sake of the geolocation text link, and the -moz-margin-start rule makes the space between the icon and the description smaller than we want. However, if you want I can keep them in there until bug 615483 is fixed.

Comment 9

[:Margaret Leibovic]

http://hg.mozilla.org/mozilla-central/rev/2df08dfa1b22

Comment 10

[[:Aleksej]]

Seems to work. What will happen with a very long domain name in bug 614926?

Comment 11

[:Margaret Leibovic]

(In reply to comment #10)
> Seems to work. What will happen with a very long domain name in bug 614926?

I just tested this, and if there is a very long domain name, it won't wrap, so the notification will expand to accommodate its width. However, this seems like a pretty rare edge case, since most domain names wouldn't be that long. We could file a new bug about it.

Comment 12

[u279076]

(In reply to comment #11)
> (In reply to comment #10)
> > Seems to work. What will happen with a very long domain name in bug 614926?
> 
> I just tested this, and if there is a very long domain name, it won't wrap, so
> the notification will expand to accommodate its width. However, this seems like
> a pretty rare edge case, since most domain names wouldn't be that long. We
> could file a new bug about it.

Is there an example domain URL to test this case?