Mike Conley (:mconley) (:⚙️) Reporter
	Description • 11 years ago

I just noticed this today - doorhangers on Windows 7 seem to not have enough padding.

I just checked, and this bug is on Aurora too. :/ Something tells me this is Australis related, and I'd like to get it on the radar because it looks pretty bad.

Examples:

Aurora: https://www.dropbox.com/s/pdje57x3v8cl90r/Screenshot%202014-03-12%2022.17.51.png
Nightly: https://www.dropbox.com/s/wmp16igjsjouuaa/Screenshot%202014-03-12%2021.48.17.png
Nightly: https://www.dropbox.com/s/djme6my9gxcc90f/Screenshot%202014-03-12%2021.52.45.png

At least the remember-password doorhanger and addon-install related doorhangers appear to be affected. Looks pretty rough.

Marking as a P2 because shipping with this is probably not something we want to do.

	Mike Conley (:mconley) (:⚙️) Reporter
	Comment 1 • 11 years ago

Thought it'd be best to upload these to Bugzilla instead of relying on Dropbox.

	Mike Conley (:mconley) (:⚙️) Reporter
	Comment 4 • 11 years ago

Here's a regression range via mozregression:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1f170f9fead0&tochange=1e9f169c9715

	Mike Conley (:mconley) (:⚙️) Reporter
	Comment 5 • 11 years ago

to be more precise - that's the first bad push on mozilla-central, which is a merge, and contains the bad changeset.

	Mike Conley (:mconley) (:⚙️) Reporter
	Comment 6 • 11 years ago

Bug 961727 looks immediately suspect.

	Mike Conley (:mconley) (:⚙️) Reporter
	Comment 7 • 11 years ago

Yep, switching from 4px back to 10px looks quite a bit better.

	u428464
	Comment 8 • 11 years ago

This has been already handled partly in bug 972525. The next step would be to redesign all the panels (buttons, spacing, etc.), but it's not planned for Australis V1 AFAIK.

	Mike de Boer [:mikedeboer] Assignee
	Comment 9 • 11 years ago

Mike, your aurora build looks kinda out-of-date, because I don't see the uplifted patch from bug 972525 active. Were you using mozregression?

This is by design, so if you don't mind, I'll mark this as invalid.

	Stephen Horlander [:shorlander]
	Comment 10 • 11 years ago

(In reply to Mike de Boer [:mikedeboer] from comment #9)
> Mike, your aurora build looks kinda out-of-date, because I don't see the
> uplifted patch from bug 972525 active. Were you using mozregression?
> 
> This is by design, so if you don't mind, I'll mark this as invalid.

Why was removing all of the padding by design?

	Mike de Boer [:mikedeboer] Assignee
	Comment 11 • 11 years ago

Well, I *thought* it was... I changed the padding of all panels on Windows to be 4px instead of 10px. This was part of the Win8 toolkit theme adjustments.

But if you want the doorhanger panels to have 10px padding again, this issue should be reopened.

	Stephen Horlander [:shorlander]
	Comment 12 • 11 years ago

(In reply to Mike de Boer [:mikedeboer] from comment #11)
> Well, I *thought* it was... I changed the padding of all panels on Windows
> to be 4px instead of 10px. This was part of the Win8 toolkit theme
> adjustments.
> 
> But if you want the doorhanger panels to have 10px padding again, this issue
> should be reopened.

Removing/reducing the padding from the panels with listviews and grids was intentional. a) because we need to maximize space and b) because the listview and grid items have their own build in space.

The content in the identity panel doorhangers is a little different. The top is usually informational/visual so doesn't need as much condensed space and also doesn't have any items with built in padding.

	Mike de Boer [:mikedeboer] Assignee
	Comment 13 • 11 years ago

In other words, you want this changed. Alright, clear.

	:Gijs (he/him)
	Comment 14 • 11 years ago

Is this really P2, though? Like, if the aurora issue is fixed, this doesn't seem as terrible.

	Mike Conley (:mconley) (:⚙️) Reporter
	Comment 15 • 11 years ago

Dunno - seemed like a big deal at the time, but I was sleep deprived.

This is what I'm seeing on Aurora (up to date): http://i.imgur.com/HlJ7UpR.png

I don't think we should ship with this, but I'd be OK knocking this down to a P3+ or P3.

	Mike Conley (:mconley) (:⚙️) Reporter
	Comment 16 • 11 years ago

Just gonna go for it.

	Mike Conley (:mconley) (:⚙️) Reporter
	Comment 18 • 11 years ago

Comment on attachment 8390556 [details] [diff] [review]
Patch v1: restore additional padding for notification panels on Windows

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

LGTM! Thanks for the quick turnaround!

	Mike de Boer [:mikedeboer] Assignee
	Comment 19 • 11 years ago

Pushed to fx-team: https://hg.mozilla.org/integration/fx-team/rev/990a0fc998fa

	Ed Morley [:emorley]
	Comment 20 • 11 years ago

Mike, would you mind fixing your username set in your hgrc? It's currently of the form:
Name Surname (foo@bar.com)
...whereas it needs to be:
Name Surname <foo@bar.com>

Thanks :-)

	Mike de Boer [:mikedeboer] Assignee
	Comment 21 • 11 years ago

Yes, my apologies! I don't know how that happened :S

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 22 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/990a0fc998fa

	:Gijs (he/him)
	Comment 23 • 11 years ago

Mike, should this be uplifted?

	Mike de Boer [:mikedeboer] Assignee
	Comment 24 • 11 years ago

Comment on attachment 8390556 [details] [diff] [review]
Patch v1: restore additional padding for notification panels on Windows

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 961727
User impact if declined: Some users might perceive the reduced padding of 4px in the doorhanger panels to be a bit cramped; this patch restores the situation back to 10px padding. IOW: less OMGCHANGE
Testing completed (on m-c, etc.): landed on m-c.
Risk to taking this patch (and alternatives if risky): minor
String or IDL/UUID changes made by this patch: n/a

	Mike de Boer [:mikedeboer] Assignee
	Comment 25 • 11 years ago

Pushed to Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/2646567bbc7c

	Dão Gottwald [:dao]
	Comment 26 • 10 years ago

Comment on attachment 8390556 [details] [diff] [review]
Patch v1: restore additional padding for notification panels on Windows

We should revert bug 961727 and special-case the listviews to conform with this:

(In reply to Stephen Horlander [:shorlander] from comment #12)
> Removing/reducing the padding from the panels with listviews and grids was
> intentional. a) because we need to maximize space and b) because the
> listview and grid items have their own build in space.
> 
> The content in the identity panel doorhangers is a little different. The top
> is usually informational/visual so doesn't need as much condensed space and
> also doesn't have any items with built in padding.

	:Gijs (he/him)
	Comment 27 • 10 years ago

This was reopened - is that for code cleanup, or is there actual brokenness on 29/30 right now? If not, does this still need to be P2? We should do the code cleanup regardless, but it'd help to know if this really needs to be a priority while we're focusing on shipping 29...

	Dão Gottwald [:dao]
	Comment 28 • 10 years ago

This is not just cleanup. Bug 961727 affected all arrow panels and as it turns out, this was undesired. #notification-popup is only one of many arrow panel instances. This needs to be fixed in popup.css rather than only for #notification-popup in browser.css.

	Thomas Stache
	Comment 29 • 10 years ago

The "Bookmark this page" arrow panel exhibits the remaining problem for instance.

	Mike de Boer [:mikedeboer] Assignee
	Comment 30 • 10 years ago

Updates made to arrowpanel unit tests were improvements, so there's no need to revert that too.

	Dão Gottwald [:dao]
	Comment 32 • 10 years ago

Comment on attachment 8394167 [details] [diff] [review]
Patch 2.1: increase panel padding back to 10px. This backs out changes made in bug 961727 and deps

> /* Notification icon box */
>-#notification-popup > .panel-arrowcontainer > .panel-arrowcontent {
>-  padding: 10px;
>-}
>-
>-#notification-popup .popup-notification-closebutton {
>-  -moz-margin-end: -14px;
>-  margin-top: -10px;
>-}
>-
>-#notification-popup .panel-promo-box {
>-  margin: 10px -10px -10px;
>-}
> 
> #notification-popup-box {
>   position: relative;

nit: remove the blank line after the comment, like it was before

	Mike de Boer [:mikedeboer] Assignee
	Comment 33 • 10 years ago

w/ nit addressed. Carrying over r=Dão.

	Matthew N. [:MattN]
	Comment 34 • 10 years ago

Comment on attachment 8394252 [details] [diff] [review]
Patch 2.2: increase panel padding back to 10px. This backs out changes made in bug 961727 and deps

https://hg.mozilla.org/integration/fx-team/rev/f80d4b08b289

	:Gijs (he/him)
	Comment 35 • 10 years ago

Doesn't this patch mean that now all the "list" style panels will have more padding again, too? Did you check that doesn't break assumptions of later patches, or are you going to bring the subview-panels and so on back to 4px separately?

	Carsten Book [:Tomcat]
	Comment 36 • 10 years ago

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

	Mike de Boer [:mikedeboer] Assignee
	Comment 37 • 10 years ago

Comment on attachment 8394252 [details] [diff] [review]
Patch 2.2: increase panel padding back to 10px. This backs out changes made in bug 961727 and deps

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis, bug 961727.
User impact if declined: User will notice too little padding for arrow panels on Windows. The decision taken to reduce padding for all arrow panels was wrong in hindsight, thus needs to be reverted.
Testing completed (on m-c, etc.): landed on m-c.
Risk to taking this patch (and alternatives if risky): minor
String or IDL/UUID changes made by this patch: n/a.

	:Gijs (he/him)
	Comment 38 • 10 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/469c940825b5
https://hg.mozilla.org/releases/mozilla-beta/rev/c7cbc594208e