982993 - Arrow panels don't have enough padding.

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: Example 1 - Nightly - Remember password

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

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Attachment: Example 2 - Nightly - Allow addon install

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Attachment: Example 3 - Aurora - Remember password

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Here's a regression range via mozregression:

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

Comment 5

[Mike Conley (:mconley) (:⚙️)]

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

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Bug 961727 looks immediately suspect.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

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

Comment 8

[u428464]

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.

Comment 9

[Mike de Boer [:mikedeboer]]

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.

Comment 10

[Stephen Horlander [:shorlander]]

(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?

Comment 11

[Mike de Boer [:mikedeboer]]

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.

Comment 12

[Stephen Horlander [:shorlander]]

(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.

Comment 13

[Mike de Boer [:mikedeboer]]

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

Comment 14

[:Gijs (out for now; he/him)]

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

Comment 15

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Just gonna go for it.

Comment 17

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: restore additional padding for notification panels on Windows

Comment 18

[Mike Conley (:mconley) (:⚙️)]

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!

Comment 19

[Mike de Boer [:mikedeboer]]

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

Comment 20

[Ed Morley [:emorley]]

Mike, would you mind fixing your username set in your hgrc? It's currently of the form:
Name Surname ([e-mail])
...whereas it needs to be:
Name Surname <[e-mail]>

Thanks :-)

Comment 21

[Mike de Boer [:mikedeboer]]

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

Comment 22

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

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

Comment 23

[:Gijs (out for now; he/him)]

Mike, should this be uplifted?

Comment 24

[Mike de Boer [:mikedeboer]]

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

Comment 25

[Mike de Boer [:mikedeboer]]

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

Comment 26

[Dão Gottwald [:dao]]

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.

Comment 27

[:Gijs (out for now; he/him)]

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...

Comment 28

[Dão Gottwald [:dao]]

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.

Comment 29

[Thomas Stache]

Attachment: Bookmarks arrow panel on Windows 7 has to little padding.

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

Comment 30

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2: increase panel padding back to 10px. This backs out changes made in bug 961727 and deps

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

Comment 31

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.1: increase panel padding back to 10px. This backs out changes made in bug 961727 and deps

Comment 32

[Dão Gottwald [:dao]]

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

Comment 33

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.2: increase panel padding back to 10px. This backs out changes made in bug 961727 and deps

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

Comment 34

[Matthew N. [:MattN]]

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

Comment 35

[:Gijs (out for now; he/him)]

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?

Comment 36

[Carsten Book [:Tomcat]]

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

Comment 37

[Mike de Boer [:mikedeboer]]

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.

Comment 38

[:Gijs (out for now; he/him)]

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