993208 - HTTPS Everywhere and Stylish icons push other icons off Australis panel

Status: RESOLVED FIXED

(WebExtensions :: General, defect)

Description

[u498184]

Attachment: australis menu bug.png

Steps to reproduce:

1) Install https-everywhere or Stylish on a clean profile
2) Add icon to panel


Actual results:

HTTPS Everywhere and Stylish will only allow one other icon on its row: They both knock the third to a new row.

Stylish's label displays to the right of the icon when placed in the menu, rather than below it as with other icons.


Expected results:

Each icon should be spaced evenly throughout the panel as with other icons

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Stylish is doing this because it sets orient=horizontal on the button.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

This bug seems like a duplicate of https://bugzilla.mozilla.org/show_bug.cgi?id=971246#c2.

I don't see much that we can do here without add-on author involvement.

Jorge, can you reach out to these add-on developers?

Comment 3

[Jorge Villalobos [:jorgev] (he/him)]

Adding developers for both add-ons to CC.

Comment 4

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)
> This bug seems like a duplicate of
> https://bugzilla.mozilla.org/show_bug.cgi?id=971246#c2.
> 
> I don't see much that we can do here without add-on author involvement.

Should this remain a P3 in this case? (I don't think so, but wanted to check with you before "demoting")

Comment 5

[Jason Barnabe (np)]

Attachment: stylish icon in firefox 28 without orient="horizontal"

Attached is how Firefox 28 displays Stylish's icon if I remove orient="horizontal".

Is there a recommendation for how to have an icon show correctly (drop down arrow to the right) in both pre and post Australis while still showing consistently with other icons in the customize toolbar panel? I suppose I can version detect and apply it by CSS...

https://github.com/JasonBarnabe/stylish/blob/master/content/overlay-fx4.xul#L72

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Gijs Kruitbosch from comment #4)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)
> > This bug seems like a duplicate of
> > https://bugzilla.mozilla.org/show_bug.cgi?id=971246#c2.
> > 
> > I don't see much that we can do here without add-on author involvement.
> 
> Should this remain a P3 in this case? (I don't think so, but wanted to check
> with you before "demoting")

I kept it as P3 based on the prominence of the add-ons involved. It is there to show that we view getting these fixed as pretty important.

(In reply to Jason Barnabe (np) from comment #5)
> Created attachment 8403714 [details]
> stylish icon in firefox 28 without orient="horizontal"
> 
> Attached is how Firefox 28 displays Stylish's icon if I remove
> orient="horizontal".
> 
> Is there a recommendation for how to have an icon show correctly (drop down
> arrow to the right) in both pre and post Australis while still showing
> consistently with other icons in the customize toolbar panel? I suppose I
> can version detect and apply it by CSS...
> 
> https://github.com/JasonBarnabe/stylish/blob/master/content/overlay-fx4.
> xul#L72

The issue is only present in the new menu panel. The simplest way to fix this would be to remove usage of the "orient" attribute, and then add the following (simplified) rule to your CSS:

toolbar #stylish-toolbar-button {
  -moz-box-orient: horizontal;
}

This will then only apply horizontal orientation when in the toolbar, and will pick up vertical orientation when it is displayed in the new #PanelUI-popup.

Comment 7

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6)
> (In reply to :Gijs Kruitbosch from comment #4)
> > (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)
> > > This bug seems like a duplicate of
> > > https://bugzilla.mozilla.org/show_bug.cgi?id=971246#c2.
> > > 
> > > I don't see much that we can do here without add-on author involvement.
> > 
> > Should this remain a P3 in this case? (I don't think so, but wanted to check
> > with you before "demoting")
> 
> I kept it as P3 based on the prominence of the add-ons involved. It is there
> to show that we view getting these fixed as pretty important.

It looks like HTTPS Everywhere has a github mirror: https://github.com/EFForg/https-everywhere . We can probably look at what needs to be done and do a pull request? I can look into it more later today.

Comment 8

[Jason Barnabe (np)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6)
> The issue is only present in the new menu panel. The simplest way to fix
> this would be to remove usage of the "orient" attribute, and then add the
> following (simplified) rule to your CSS:
> 
> toolbar #stylish-toolbar-button {
>   -moz-box-orient: horizontal;
> }

Actually, since none of the other buttons that open menus when clicked have the menu arrow any more, I'm going to remove type="menu" as well, which eliminates the need to mess with orient.

I do have another issue, though. In previous versions, the toolbar icon sizes are 24px (when iconsize="large") and 16px (when iconsize="small"). With Australis, the toolbar icon is always 16px, but iconsize="large" is still set, so Stylish is placing the 24px icon. Is there a reason this attribute is still around, and if so, how do I work around this?

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Jason Barnabe (np) from comment #8)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6)
> > The issue is only present in the new menu panel. The simplest way to fix
> > this would be to remove usage of the "orient" attribute, and then add the
> > following (simplified) rule to your CSS:
> > 
> > toolbar #stylish-toolbar-button {
> >   -moz-box-orient: horizontal;
> > }
> 
> Actually, since none of the other buttons that open menus when clicked have
> the menu arrow any more, I'm going to remove type="menu" as well, which
> eliminates the need to mess with orient.
> 
> I do have another issue, though. In previous versions, the toolbar icon
> sizes are 24px (when iconsize="large") and 16px (when iconsize="small").
> With Australis, the toolbar icon is always 16px, but iconsize="large" is
> still set, so Stylish is placing the 24px icon. Is there a reason this
> attribute is still around, and if so, how do I work around this?

In Australis and on pre-Australis (Firefox 28 on Windows8.1), the toolbar button icons are actually 18px. I think maybe only Linux had 24px toolbar icons since the GTK icons were a bit larger?

Comment 10

[Jason Barnabe (np)]

Pre-Australis on Linux, when you toggle "Use small icons", you're switching between 16px and 24px icons. Since I use Linux, I assumed this was what was still happening on other platforms, but now that I test it, I see that that toggle just messes with the margins and padding on Windows and Mac. I believe long ago versions of Firefox on Windows also actually switched icon size.

Stylish worked with pre-Australis Windows and Mac because even when you toggle "Use small icons", the toolbar's iconsize attribute was always "small". On Linux, the toolbar's iconsize changes between "small" and "large" based on the user's choice.

The problem is that now with Australis (at least on Linux) the toolbar's iconsize is always "large", so Stylish is using its 24px icon, which is very out of place.

If the toolbar iconsize was instead always "small" in Australis, there would be no issue. If it's "large" for a reason, I'm looking for a way to always show my small icon on Australis, but toggle as it does now in previous versions.

Comment 11

[:Gijs (he/him)]

(In reply to Jason Barnabe (np) from comment #10)
> If the toolbar iconsize was instead always "small" in Australis, there would
> be no issue. If it's "large" for a reason, I'm looking for a way to always
> show my small icon on Australis, but toggle as it does now in previous
> versions.

What build are you testing with? IIRC we reset the mode and iconsize properties in a migration step (nsBrowserGlue.js - if your profile has been used both on and off australis, and you've toggled these bits, that might mean it's no longer being reset), and if you hit 'restore defaults'...

Comment 12

[Jason Barnabe (np)]

Using the same profile:

-In Firefox 28, customize toolbars to not use small icons
-In Firefox 29.0b6, no issue
-In Firefox 28 again, customize toolbars to not use small icons
-In Firefox 29.0b6, issue occurs.

"Restore defaults" does not fix the issue.

Comment 13

[:Gijs (he/him)]

Yeah, so I was confused - we did this for the "mode" attribute (text+icons / icons / text), but forgot the iconsize attribute. We should probably do those too. :-\

Comment 14

[Jason Barnabe (np)]

The issue with Stylish has been fixed in https://github.com/JasonBarnabe/stylish/issues/178 and is available in 1.4.3b1 at https://addons.mozilla.org/en-US/firefox/addon/stylish/versions/ .

The iconsize issue is still present as I'm expecting a fix in Firefox for that.

Comment 15

[:Gijs (he/him)]

(In reply to Jason Barnabe (np) from comment #14)
> The issue with Stylish has been fixed in
> https://github.com/JasonBarnabe/stylish/issues/178 and is available in
> 1.4.3b1 at https://addons.mozilla.org/en-US/firefox/addon/stylish/versions/ .
> 
> The iconsize issue is still present as I'm expecting a fix in Firefox for
> that.

Yes! I hope to finish that patch today. See bug 989289. Thanks for your help!

Comment 16

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #15)
> (In reply to Jason Barnabe (np) from comment #14)
> > The issue with Stylish has been fixed in
> > https://github.com/JasonBarnabe/stylish/issues/178 and is available in
> > 1.4.3b1 at https://addons.mozilla.org/en-US/firefox/addon/stylish/versions/ .
> > 
> > The iconsize issue is still present as I'm expecting a fix in Firefox for
> > that.
> 
> Yes! I hope to finish that patch today. See bug 989289. Thanks for your help!

This should be in the upcoming beta builds, as it landed on beta earlier today. I'm still hoping to be able to write a patch for https everywhere tomorrow.

Comment 17

[:Gijs (he/him)]

Yan, I've created a pull request to fix the https everywhere issue here: https://github.com/EFForg/https-everywhere/pull/216

Comment 18

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #17)
> Yan, I've created a pull request to fix the https everywhere issue here:
> https://github.com/EFForg/https-everywhere/pull/216

This was merged yesterday night. Not sure when this is going to be released, but I think at least as far as this bug is concerned we're done here. :-)

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Great news! Thank you to everyone involved here!