1171410 - Fix sorter buttons on about:addons - text labels twitching when I switch type of sorting

Status: RESOLVED WONTFIX

(Toolkit :: Themes, defect)

Description

[arni2033]

STR:
1. Go to about:addons
2. Search for addons
3. Change type of sorting (by name/date/relevance , local/remote)


Result:

Text labels moving
Watch video if STR are unclear: https://dl.dropboxusercontent.com/s/6kx9nkspmdwkvtu/Text%20labels%20twitching%20when%20I%20switch%20type%20of%20sorting.webm?dl=0


Expectations:

Text labels stay still

See also userstyle I made for release version to see my suggestion on how sorter buttons should look like. They at least should use "smart padding" to not twitching, but also following already established in about:preferences style for sorter buttons [gray] would be way better than just "smart padding". (Don't mind other aspects of the style)
Userstyle: https://userstyles.org/styles/113418/restyle-about-addons-for-firefox

Comment 1

[:Gijs (he/him)]

Richard, would you care to look at this?

Comment 2

[Richard Marti (:Paenglab)]

Yes, I can.

The sort buttons need some love in this bug. The radio buttons don't move in my Nightly. I suppose because of bug 1165973. I'll check this this evening at home if they move also on older releases.

Comment 3

[Richard Marti (:Paenglab)]

Attachment: Bug1171410.patch

(In reply to Richard Marti (:Paenglab) from comment #2)
> The radio buttons don't move in my Nightly. I suppose because of
> bug 1165973.

Yes, this bug fixes the radio button moving. Asked for approval-aurora in it's bug.

This patch adds a padding-start on sorter to make the gap on both sides the same. The OS X arrow-dn.gif is 11px wide -> using now the arrow-dn.png which is also 5px as the Linux and Windows ones.

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 8615442 [details] [diff] [review]
Bug1171410.patch

This doesn't look right at all on Linux

Comment 5

[Dão Gottwald [:dao]]

Attachment: screenshot with patch

Comment 6

[Richard Marti (:Paenglab)]

Attachment: Bug1171410.patch v2

Sorry, not checked the focusring. Now the padding is on the .button-box.

Comment 7

[Richard Marti (:Paenglab)]

Attachment: Bug1171410.patch v3

Fixed now also the sorter arrows on Linux.

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 8615541 [details] [diff] [review]
Bug1171410.patch v3

I'm not quite sure what to make of this patch. I tested this on Linux and don't seem to notice any positive effect. The differences I see is that you dropped the native sorting icons in favor of arrow-dn.gif and that the horizontal button padding looks uneven on hover, neither of which seem like improvements to me.

Comment 9

[Richard Marti (:Paenglab)]

Attachment: Bug1171410.patch v4

(In reply to Dão Gottwald [:dao] from comment #8)
> Comment on attachment 8615541 [details] [diff] [review]
> Bug1171410.patch v3
> 
> I'm not quite sure what to make of this patch. I tested this on Linux and
> don't seem to notice any positive effect. The differences I see is that you
> dropped the native sorting icons in favor of arrow-dn.gif and that the
> horizontal button padding looks uneven on hover, neither of which seem like
> improvements to me.

Linux had -moz-margin-end: 2px; on button-icon -> removed this margin on sorter button-icon. Now the padding is equal. Without the patch it's more uneven (on Linux 8px).

A big improvement is when you have the last sorter selected and the select the first, look at the middle sorter how the text jumps without this patch.

Comment 10

[arni2033]

Hello, Richard.
1) Are you going to make inContent sorter buttons from this bug look like sorter buttons on (e.g.) about:config OR should I go through some mailing lists/open another bug exactly for that feature/suggestion OR that is clearly WONTFIX ??
2) See also bug 1177961

Comment 11

[Richard Marti (:Paenglab)]

1) About:config is a tree with different rows and their headers to sort them. The add-ons are in a list without this rows and it's not possible to have such sort headers. Because of this I would say this would be a WONTFIX.

2) I'll look at this. I have already a solution.

Comment 12

[arni2033]

I don't understand your reason, I was talking about the gray background for buttons "Name", "Last Updated" and "Best match" (xul:button.sorter). Another example of that style is about:preferences#applications
Sorry for spamming here though; looks like I really should "go through some mailing lists"

Comment 13

[Richard Marti (:Paenglab)]

Dao, please could you look at this patch when you have some time?

Comment 14

[Richard Marti (:Paenglab)]

Attachment: Bug1171410.patch v5

Unbitrotted.

Dao, please could you look at this patch. The previous was already waiting 6 month for review.

Comment 15

[Richard Marti (:Paenglab)]

Comment on attachment 8691490 [details] [diff] [review]
Bug1171410.patch v5

It seems, Dão has no time to review this patch.

Marco, maybe you could look at it?

Comment 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8691490 [details] [diff] [review]
Bug1171410.patch v5

I'm not the best person to evaluate this change (even if technically I could), and my work queue isn't really short (it would still take too many days...)
Moreover it's possible Dao delayed this for a reason, that I don't know.

While I understand having a review blocked for months is frustrating, I think the best path forward is to have an open and kind discussion about it and try to understand what's blocking.
Try sending him an email and eventually setup a brief irc chat to figure out the blocking problems.

Comment 17

[Richard Marti (:Paenglab)]

Attachment: Bug1171410.patch

Unbitrotted patch.

Dão, please can you look at this patch soon? The previous patch was already waiting 10 month. It's a small patch with simple changes.

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 8794096 [details] [diff] [review]
Bug1171410.patch

Sorry, this still looks wrong to me, especially the hover state being asymmetrical.

Comment 19

[Richard Marti (:Paenglab)]

Attachment: sorter.png

(In reply to Dão Gottwald [:dao] from comment #18)
> Comment on attachment 8794096 [details] [diff] [review]
> Bug1171410.patch
> 
> Sorry, this still looks wrong to me, especially the hover state being
> asymmetrical.

I see no asymmetry on hover (see screenshot on Linux). On both sides there is 23px space.

Please, can you explain, what you mean?

Comment 20

[Dão Gottwald [:dao]]

Attachment: screenshot with patch

This is what I'm seeing.

Comment 21

[Richard Marti (:Paenglab)]

(In reply to Dão Gottwald [:dao] from comment #20)
> Created attachment 8795268 [details]
> screenshot with patch
> 
> This is what I'm seeing.

This is strange. I tied it now on a different Linux and it look symmetrical here.

Please, can you check what width the .button-icon of the hovered/not active button has? It should be 5px with an margin-left of 6px. Do you have something that overrides it?

I see now I have still -moz properties instead of the inline ones. I'll fix this with the next patch after your reply.

Comment 22

[Richard Marti (:Paenglab)]

Dão, I created try builds with this patch, https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-501059328169784e434a00f5f1a8cbb476547b8d/ .

This builds show me symmetrical buttons on all platforms. Please can you try this builds to check if you still see the issue?

Comment 23

[Dão Gottwald [:dao]]

Comment on attachment 8794096 [details] [diff] [review]
Bug1171410.patch

Okay, this works for me now when applying the patch on current mozilla-central. No idea what went wrong the last two times I tried this patch.

>--- a/toolkit/themes/osx/mozapps/extensions/extensions.css
>+++ b/toolkit/themes/osx/mozapps/extensions/extensions.css

>-.sorter[checkState="1"] {
>-  list-style-image: url("chrome://global/skin/arrow/arrow-dn.gif");
>+.sorter[checkState="1"],
>+.sorter[checkState="2"] {
>+  list-style-image: url("chrome://global/skin/arrow/arrow-dn.png");
> }
> 
>-.sorter[checkState="2"] {
>-  list-style-image: url("chrome://global/skin/arrow/arrow-up.gif");
>+.sorter[checkState="2"] > .button-box > .button-icon {
>+  transform: scaleY(-1);
> }

Why these changes?

> .sorter .button-box {
>+  /* compensate the sort arrow width and margin to center the text */
>+  -moz-padding-start: 21px !important;
>   padding-top: 0;
>   padding-bottom: 0;
> }
> 
> .sorter[checkState="1"],
> .sorter[checkState="2"] {
>   background-color: #ebebeb;
>   box-shadow: 0 -4px 0 0 #ff9500 inset;
> }
> 
> .sorter .button-icon {
>   margin-inline-start: 6px;
>+  -moz-margin-end: 0;
>+  width: 5px;
> }

Use padding-inline-start, margin-inline-end

Comment 24

[Richard Marti (:Paenglab)]

Attachment: Bug1171410.patch

(In reply to Dão Gottwald [:dao] from comment #23)
> Comment on attachment 8794096 [details] [diff] [review]
> Bug1171410.patch
> >--- a/toolkit/themes/osx/mozapps/extensions/extensions.css
> >+++ b/toolkit/themes/osx/mozapps/extensions/extensions.css
> 
> >-.sorter[checkState="1"] {
> >-  list-style-image: url("chrome://global/skin/arrow/arrow-dn.gif");
> >+.sorter[checkState="1"],
> >+.sorter[checkState="2"] {
> >+  list-style-image: url("chrome://global/skin/arrow/arrow-dn.png");
> > }
> > 
> >-.sorter[checkState="2"] {
> >-  list-style-image: url("chrome://global/skin/arrow/arrow-up.gif");
> >+.sorter[checkState="2"] > .button-box > .button-icon {
> >+  transform: scaleY(-1);
> > }
> 
> Why these changes?

Because the arrow-dn.gif and arrow-up.gif are bigger than the Linux and Windows icons. arrow-dn.png has the Linux/Windows icons size. This makes it possible to use the shared CSS file for the button padding.

> > .sorter .button-box {
> >+  /* compensate the sort arrow width and margin to center the text */
> >+  -moz-padding-start: 21px !important;
> >   padding-top: 0;
> >   padding-bottom: 0;
> > }
> > 
> > .sorter[checkState="1"],
> > .sorter[checkState="2"] {
> >   background-color: #ebebeb;
> >   box-shadow: 0 -4px 0 0 #ff9500 inset;
> > }
> > 
> > .sorter .button-icon {
> >   margin-inline-start: 6px;
> >+  -moz-margin-end: 0;
> >+  width: 5px;
> > }
> 
> Use padding-inline-start, margin-inline-end

Done. I wrote in comment 21 I'll fix this with the next patch after we cleared the symmetry issue.

Comment 25

[Dão Gottwald [:dao]]

(In reply to Richard Marti (:Paenglab) from comment #24)
> Because the arrow-dn.gif and arrow-up.gif are bigger than the Linux and
> Windows icons. arrow-dn.png has the Linux/Windows icons size.

It probably looks worse, though?

Comment 26

[Richard Marti (:Paenglab)]

Attachment: OSX-sorter.png

(In reply to Dão Gottwald [:dao] from comment #25)
> (In reply to Richard Marti (:Paenglab) from comment #24)
> > Because the arrow-dn.gif and arrow-up.gif are bigger than the Linux and
> > Windows icons. arrow-dn.png has the Linux/Windows icons size.
> 
> It probably looks worse, though?

This is how it looks under OS X.

Comment 27

[Dão Gottwald [:dao]]

Comment on attachment 8801141 [details] [diff] [review]
Bug1171410.patch

The search pane is gone (bug 1263313) but sorters are still used in the Updates pane. Not sure how to test this but I think we can probably just get rid of the sorters there.

Comment 28

[Tim Nguyen :ntim]

The sorters got removed in bug 1455392.