Closed Bug 629438 Opened 9 years ago Closed 9 years ago

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

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla2.0b12
Tracking Status
blocking2.0 --- -

People

(Reporter: hipokrit, Assigned: hipokrit)

References

Details

(Keywords: polish)

Attachments

(9 files, 4 obsolete files)

170.34 KB, image/png
Details
115.74 KB, image/png
Details
82.23 KB, image/png
Details
104.06 KB, image/png
Details
3.00 KB, patch
Details | Diff | Splinter Review
3.41 KB, patch
Details | Diff | Splinter Review
53.06 KB, image/png
Details
3.01 KB, patch
mossop
: review+
mossop
: approval2.0+
Details | Diff | Splinter Review
142.72 KB, image/png
Details
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
Attached patch Patch (obsolete) — Splinter Review
This patch should resolve all icon sizing/spacing issues.
Attachment #507531 - Flags: review?(dtownsend)
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.
As seen in this image, the icon does not change position when the release notes are expanded.
blocking2.0: --- → ?
Keywords: polish
See Also: → 623211, 603057
Version: unspecified → Trunk
blocking2.0: ? → -
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?
Attached patch Updated patch (obsolete) — Splinter Review
Dave's correct, the height isn't needed. Tested and confirmed still working identically, but without using percentage.
Attachment #507531 - Attachment is obsolete: true
Attachment #507576 - Flags: review?(dtownsend)
Attachment #507531 - Flags: review?(dtownsend)
Attachment #507576 - Flags: review?(dtownsend)
Attachment #507576 - Flags: review+
Attachment #507576 - Flags: approval2.0+
QA Contact: add-ons.manager → hipokrit
Assignee: nobody → hipokrit
Keywords: checkin-needed
Whiteboard: [softblocker][has patch][can land]
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Status: ASSIGNED → NEW
Keywords: checkin-needed
Whiteboard: [softblocker][has patch][can land] → [softblocker]
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.
Attachment #507576 - Flags: review+ → review-
Attached patch Patch with revised margins (obsolete) — Splinter Review
Contains margin change to ensure proper height for the addon richlistitem entries.
Attachment #507576 - Attachment is obsolete: true
Attachment #507705 - Flags: review?(dtownsend)
QA Contact: hipokrit → add-ons.manager
Attachment #507705 - Flags: review?(dtownsend)
Attachment #507705 - Flags: review+
Attachment #507705 - Flags: approval2.0+
Status: NEW → ASSIGNED
Keywords: checkin-needed
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?
Keywords: checkin-needed
Whiteboard: [softblocker]
(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.
How about this?
Attached image Screenshot without two containers (obsolete) —
(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.
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.
Attachment #507705 - Attachment is obsolete: true
Attachment #508418 - Flags: review?(dtownsend)
(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...
> > (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.
Attachment #508418 - Flags: review?(dtownsend)
Updated .icon-container margins to 3px to keep the entries to be the correct height.
Attachment #508440 - Flags: review?(dtownsend)
As far as I remember and considering attachment 508403 [details], the margin used in my patch looks just about right.
(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 (gavin@moz-9175EA73.cpe.net.cable.rogers.com) 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.
Attachment #508403 - Attachment is obsolete: true
This screen demonstrates the sizing and placement of the items and icons with the patch in attachment 508440 [details] [diff] [review].
Ok, I understand what you mean with regards to the margin affecting the item height.
Attachment #508440 - Flags: review?(dtownsend) → review+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/2f353922a56c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b12
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #508440 - Flags: approval2.0+
well...

http://hg.mozilla.org/mozilla-central/rev/a12d11c8912d
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Or mxr was lying to me
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?
(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.
(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!
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?
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Flags: in-litmus?
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.
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.
Flags: in-testsuite?
Flags: in-testsuite-
Flags: in-litmus?
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.