629438 - Add-ons manager add-on icon should be positioned correctly in list and not cause sizing differences

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Matthew Printz [:hipokrit]]

User-Agent:       Mozilla/5.0 (X11; Linux i686; rv:2.0b10) Gecko/20100101 Firefox/4.0b10
Build Identifier: 

Currently, icons are centered using a preset offset in pixels in the CSS. This works for icons that are 32px tall, but causes inconsistencies for differently sized icons.

For larger icons, this causes the addon listitem to have a different height than other addons, making for an inconsistent interface.

The icons should be centered for the default view, but should not change position when viewing release notes as addressed in bug 603057.

Reproducible: Always

Comment 1

[Matthew Printz [:hipokrit]]

Attachment: Screenshot showing sizing issue

Comment 2

[Matthew Printz [:hipokrit]]

Attachment: Patch

This patch should resolve all icon sizing/spacing issues.

Comment 3

[Matthew Printz [:hipokrit]]

Attachment: Screenshot showing icon placement in list view

As seen here, all addon items are sized with the same height regardless of the size of the icon, but the icon center position is consistent for all items.

Comment 4

[Matthew Printz [:hipokrit]]

Attachment: Screenshot showing icon placement in Release Note view

As seen in this image, the icon does not change position when the release notes are expanded.

Comment 5

[Dave Townsend [:mossop]]

Comment on attachment 507531 [details] [diff] [review]
Patch

percentages generally are the devil in XUL but I don't think this patch should need it. Does it work without them?

Comment 6

[Matthew Printz [:hipokrit]]

Attachment: Updated patch

Dave's correct, the height isn't needed. Tested and confirmed still working identically, but without using percentage.

Comment 7

[Matthew Printz [:hipokrit]]

Comment on attachment 507576 [details] [diff] [review]
Updated patch

After speaking with Boriss, I found that the current margins are causing the addon entry to be too tall.

Removing review status pending new patch with agreed upon margins.

Comment 8

[Matthew Printz [:hipokrit]]

Attachment: Patch with revised margins

Contains margin change to ensure proper height for the addon richlistitem entries.

Comment 9

[Matthew Printz [:hipokrit]]

Attachment: Screenshot showing new heights for items with changed margins.

Comment 10

[Dão Gottwald [::dao]]

I don't understand why this patch is adding an inner container. Why can't this be fixed by changing pack="start" to pack="center" and adjusting the CSS?

Comment 11

[Matthew Printz [:hipokrit]]

(In reply to comment #10)
> I don't understand why this patch is adding an inner container. Why can't this
> be fixed by changing pack="start" to pack="center" and adjusting the CSS?

This is to satisfy the icon moving when the item is expanded as described in bug 603057. Compare the screenshot on that bug to attachment 507536 [details] above.

By using two containers, we can ensure that the center of the icon is always the same distance from top of the list item, regardless of size.

Comment 12

[Dão Gottwald [::dao]]

Attachment: alternative patch

How about this?

Comment 13

[Matthew Printz [:hipokrit]]

Attachment: Screenshot without two containers

(In reply to comment #12)
> Created attachment 508136 [details] [diff] [review]
> alternative patch
> 
> How about this?

This is how it looks for with this patch. While the list looks great, this does not resolve the issue with the icon changing position on resize. That is why I went with the two containers in my patch as the inner container will not inherit the flex.

I believe that the flex is not inherited because flexing is removed if an element has an align attribute.

Adding align="start" to the parent box fixes the vertical alignment issues on the .icon-container, but causes the buttons to shift to the left and no longer be right justified.

Comment 14

[Matthew Printz [:hipokrit]]

Attachment: Patch with proper margins

Working with Dao's patch made me realize I had the margins on .icon-outer-container incorrect. I redid the math and found that the margins should be 3px instead of 1px. Moving down the margin better centers the icon for the collapsed view.

Comment 15

[Dão Gottwald [::dao]]

(In reply to comment #13)
> Created attachment 508403 [details]
> Screenshot without two containers
> 
> (In reply to comment #12)
> > Created attachment 508136 [details] [diff] [review]
> > alternative patch
> > 
> > How about this?
> 
> This is how it looks for with this patch.

This is not what I see. Screenshot coming...

Comment 16

[Dão Gottwald [::dao]]

Attachment: screenshot using my patch

Comment 17

[Matthew Printz [:hipokrit]]

> > (In reply to comment #12)
> > > Created attachment 508136 [details] [diff] [review]
> > > alternative patch
> > > 
> > > How about this?
> > 
> > This is how it looks for with this patch.
> 
> This is not what I see. Screenshot coming...

Wierd. I don't know what I did to mess up the patch, but I reapplied and the single container is looking good now. However, we still want the top/bottom margins on .icon-container to be 3px instead of 7px.

Comment 18

[Matthew Printz [:hipokrit]]

Attachment: Patch combining Dao's single container patch with proper margins

Updated .icon-container margins to 3px to keep the entries to be the correct height.

Comment 19

[Dão Gottwald [::dao]]

As far as I remember and considering attachment 508403 [details], the margin used in my patch looks just about right.

Comment 20

[Matthew Printz [:hipokrit]]

(In reply to comment #19)
> As far as I remember and considering attachment 508403 [details], the margin used in my
> patch looks just about right.

I think attachment 508403 [details] should be disregarded as obviously I had a bad merge or something and isn't really what any of the patches actually generated.

The issue is that the image is dictating the minimum entry size:


.icon-container: 48px height; margin-top:7px; margin-bottom: 7px
.addon: padding-top: 5px; padding-bottom: 5px

This means that the minimum size of an entry can be is 72 px (48 + 7 + 7 + 5 + 5), can is usually a pixel or two larger due to the border, etc...


Per Boriss, the target size should be closer to 68px. This is why the margins has been reduced.

I am including below a log from an IRC conversation with Boriss regarding this:


<hipokrit> Boriss, are we looking to make the Addon items in the list as compact as possible? Do we have a target height?
<Boriss> target's 68px high per engry
<Boriss> entry*
* not_gavin ([e-mail]) has joined #ux
* ChanServ gives channel operator status to not_gavin
<hipokrit> I'm working on bug 629438, and noticed that the size of the entry is being determined by the margin around the icon container
<firebot> Bug https://bugzilla.mozilla.org/show_bug.cgi?id=629438 nor, --, ---, hipokrit, ASSI, Add-ons manager add-on icon should be positioned correctly in list and not cause sizing differences
<hipokrit> Just wanted to make sure that any changes I made wouldn't cause unwanted sizing change
<hipokrit> Boriss, with the code I have in my patch, it looks like we are looking at a size of 73px, with 7px margin on top and bottom
<hipokrit> do you think I should change the margins to 1px top/bottom to conform?
<hipokrit> Screen of current appearance with patch: http://hipokrit.com/Screen1.png
<Boriss> hipokrit: nice, that's really close!  think you can shave off 5px?  1px margina are all that's needed.  here's the intended size on osx http://grab.by/grabs/8c174b10b3a32e85f33628616a5a75e1.png and windows http://grab.by/grabs/e821218e4d0750a876341cd86ef067e1.png
<Boriss> that's already looking so much better, hipokrit 
<Boriss> nice work :)
<dolske> so many buttons
<hipokrit> Boriss, here is screen with 1px, size looks to be 66px tall: http://www.hipokrit.com/Screen2.png
<Boriss> hipokrit: that looks great!
<hipokrit> Boriss, Thanks. I'll get this patch in.


I updated the margin from 1px to 3px to better center the icon on the collapsed view, but this does't change the size beyond the 66px mentioned in IRC.

If you still have concerns, we should probably as Boriss to weigh in.

Comment 21

[Matthew Printz [:hipokrit]]

Attachment: Screenshot showig placement and sizing from latest patch

This screen demonstrates the sizing and placement of the items and icons with the patch in attachment 508440 [details] [diff] [review].

Comment 22

[Dão Gottwald [::dao]]

Ok, I understand what you mean with regards to the margin affecting the item height.

Comment 23

[Dão Gottwald [::dao]]

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

Comment 24

[Dave Townsend [:mossop]]

(In reply to comment #23)
> http://hg.mozilla.org/mozilla-central/rev/2f353922a56c

That appears to be for a different bug, this doesn't appeared to have landed, which is good because it hasn't yet got approval to do so.

Comment 25

[Dão Gottwald [::dao]]

well...

http://hg.mozilla.org/mozilla-central/rev/a12d11c8912d

Comment 26

[Dave Townsend [:mossop]]

Or mxr was lying to me

Comment 27

[Pardal Freudenthal (:ShareBird)]

I'm sorry for spamming this bug, but I can't think on another place to express my complains about the way the add-ons manager is going to be developed...

Besides the lack of classes and Ids as mentioned in Bug 569519, which require themes to use sibling selectors, for example, I do not understand why not use CSS for "align" and "pack", respectively -moz-box-align and -moz-box-pack, instead of defining them in-line, making it impossible for themes to override them. Is there a good reason to do so?

I've worked very hard to give to the add-ons manager a different appearance: https: / / static.addons.mozilla.net/img/uploads/previews/full/51/51429.png? modified = 1291234709
Things like:

  - <xul:hbox>
  - <xul:vbox align="center" pack="start" class="icon-container">
  + <xul:hbox align="start">
  + <xul:vbox align="center" pack="center" class="icon-container">

breaks me horribly!

I've already needed to use a custom binding just for being able to change those things because of Bug 593535.
Setting these attributes through CSS would make possible for themes to skin the Add-ons Manager in a free way without using many hacks ...

Any chance to change this, setting "align" and "pack" through CSS? Please?

Comment 28

[Dão Gottwald [::dao]]

(In reply to comment #27)
> Any chance to change this, setting "align" and "pack" through CSS? Please?

Only if you file a bug on it. Bonus points for attaching a patch.

Comment 29

[Pardal Freudenthal (:ShareBird)]

(In reply to comment #28)
> (In reply to comment #27)
> > Any chance to change this, setting "align" and "pack" through CSS? Please?
> 
> Only if you file a bug on it. Bonus points for attaching a patch.

Dão, filed Bug 631827 - Use CSS to set the "align" and "pack" attributes on Add-ons Manager to make easier for themes to handle items. Patch is coming soon.
Thank you very much!

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110209 Firefox/4.0b12pre

So what about tests for this particular issue? Doable via an automated test or do we need a manual one?

Comment 31

[Matthew Printz [:hipokrit]]

I cannot think of a way to do an automated test to ensure the positioning is correct. A manual test is likely required unless someone else knows how to do this.

Comment 32

[Dave Townsend [:mossop]]

Unless we start doing reftest like UI tests I don't think we can automatically test it, don't think it is worth a manual test either.