Closed Bug 982993 Opened 6 years ago Closed 6 years ago

Arrow panels don't have enough padding.

Categories

(Toolkit :: Themes, defect)

All
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox29 --- fixed
firefox30 --- fixed
firefox31 --- fixed

People

(Reporter: mconley, Assigned: mikedeboer)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [Australis:P3+][qa-])

Attachments

(6 files, 2 obsolete files)

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.
Thought it'd be best to upload these to Bugzilla instead of relying on Dropbox.
to be more precise - that's the first bad push on mozilla-central, which is a merge, and contains the bad changeset.
Bug 961727 looks immediately suspect.
Yep, switching from 4px back to 10px looks quite a bit better.
Blocks: 961727
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, 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.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → INVALID
(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?
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.
Flags: needinfo?(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.
Flags: needinfo?(shorlander)
In other words, you want this changed. Alright, clear.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Assignee: nobody → mdeboer
Status: REOPENED → ASSIGNED
Is this really P2, though? Like, if the aurora issue is fixed, this doesn't seem as terrible.
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.
Just gonna go for it.
Whiteboard: [Australis:P2] → [Australis:P3+]
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!
Attachment #8390556 - Flags: review?(mconley) → review+
Pushed to fx-team: https://hg.mozilla.org/integration/fx-team/rev/990a0fc998fa
Whiteboard: [Australis:P3+] → [Australis:P3+][fixed-in-fx-team]
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 :-)
Flags: needinfo?(mdeboer)
Yes, my apologies! I don't know how that happened :S
Flags: needinfo?(mdeboer)
https://hg.mozilla.org/mozilla-central/rev/990a0fc998fa
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3+][fixed-in-fx-team] → [Australis:P3+]
Target Milestone: --- → Firefox 30
Mike, should this be uplifted?
Flags: needinfo?(mdeboer)
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
Attachment #8390556 - Flags: approval-mozilla-aurora?
Flags: needinfo?(mdeboer)
Attachment #8390556 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #8390556 - Flags: review-
Status: RESOLVED → REOPENED
Component: Theme → Themes
Product: Firefox → Toolkit
Resolution: FIXED → ---
Target Milestone: Firefox 30 → ---
Summary: Doorhangers don't have enough padding. → Arrow panels don't have enough padding.
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...
Flags: needinfo?(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.
Flags: needinfo?(dao)
The "Bookmark this page" arrow panel exhibits the remaining problem for instance.
Updates made to arrowpanel unit tests were improvements, so there's no need to revert that too.
Attachment #8394162 - Flags: review?(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
Attachment #8394167 - Flags: review?(dao) → review+
w/ nit addressed. Carrying over r=Dão.
Attachment #8394167 - Attachment is obsolete: true
Attachment #8394252 - Flags: review+
Attachment #8394252 - Flags: checkin?
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
Attachment #8394252 - Flags: checkin? → checkin+
Status: REOPENED → ASSIGNED
Hardware: x86 → All
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?
https://hg.mozilla.org/mozilla-central/rev/f80d4b08b289
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
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.
Attachment #8394252 - Flags: approval-mozilla-beta?
Attachment #8394252 - Flags: approval-mozilla-aurora?
Attachment #8394252 - Flags: approval-mozilla-beta?
Attachment #8394252 - Flags: approval-mozilla-beta+
Attachment #8394252 - Flags: approval-mozilla-aurora?
Attachment #8394252 - Flags: approval-mozilla-aurora+
Whiteboard: [Australis:P3+] → [Australis:P3+][qa-]
You need to log in before you can comment on or make changes to this bug.