Closed Bug 615481 Opened 9 years ago Closed 9 years ago

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

Categories

(Toolkit :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: faaborg, Assigned: Margaret)

References

Details

(Whiteboard: [target-betaN][doorhanger])

Attachments

(1 file, 2 obsolete files)

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).
Product: Firefox → Toolkit
QA Contact: general → general
Summary: Arrowpanel notifications need a maximum width → Arrowpanel (popup) notifications need a maximum width
blocking2.0: --- → betaN+
Summary: Arrowpanel (popup) notifications need a maximum width → Doorhanger (popup) notifications need a maximum width and the text needs to wrap if necessary
Duplicate of this bug: 614926
Assignee: nobody → margaret.leibovic
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #494100 - Flags: review?(gavin.sharp)
Attached patch patch v2 (obsolete) — Splinter Review
Magically simpler patch that uses xbl:text.
Attachment #494100 - Attachment is obsolete: true
Attachment #494133 - Flags: review?(gavin.sharp)
Attachment #494100 - Flags: review?(gavin.sharp)
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?
Attachment #494133 - Flags: review?(gavin.sharp) → review-
The learn more changes could be moved over to bug 615483.
(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?
>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
Attached patch patch v3Splinter Review
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.
Attachment #494133 - Attachment is obsolete: true
Attachment #494517 - Flags: review?(gavin.sharp)
Whiteboard: [needs review gavin]
Attachment #494517 - Flags: review?(gavin.sharp) → review+
Whiteboard: [needs review gavin] → [can land]
http://hg.mozilla.org/mozilla-central/rev/2df08dfa1b22
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [can land]
Depends on: 617270
Whiteboard: [target-betaN]
Seems to work. What will happen with a very long domain name in bug 614926?
(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.
Whiteboard: [target-betaN] → [target-betaN][doorhanger]
(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?
Depends on: 660279
You need to log in before you can comment on or make changes to this bug.