Closed Bug 941045 Opened 11 years ago Closed 10 years ago

Toolbar buttons in dropdown menu should not use default orientation of toolbar button

Categories

(Firefox :: Toolbars and Customization, defect)

Product:
Firefox
Component:
Toolbars and Customization
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED WONTFIX

People

(Reporter: mkaply, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P5])

Attachments

(1 file)

Testcase
2.96 KB, application/x-xpinstall
Details 
Because the dropdown menu is displaying a grid view, it should always use a vertical orientation (even if it has to fake it, like the customizae palette).

A lot of toolbars set a horizontal orientation because it looks better in the toolbar (for instance, icons and text).

This ends up looking strange when those items are placed in the dropdown menu.

See the addon in bug 940443 for an example.
(In reply to Mike Kaply (:mkaply) from comment #1)
> Because the dropdown menu is displaying a grid view, it should always use a
> vertical orientation (even if it has to fake it, like the customizae
> palette).
> 
> A lot of toolbars set a horizontal orientation because it looks better in
> the toolbar (for instance, icons and text).
> 
> This ends up looking strange when those items are placed in the dropdown
> menu.
> 
> See the addon in bug 940443 for an example.

It would be super nice if we had a reduced testcase here, too, because there is actually already a lot of custom styling going into those buttons, so I don't know why this would still go wrong, and am not entirely sure I get what you say when you say you "set" an orientation, and how that would override our specific styling for labels and icons.
Flags: needinfo?(mozilla)
Whiteboard: [Australis:P5]
Attached file Testcase — 
Testcase attached.

The button on the toolbar has "orient=horizontal" with icon and text.

When it is dragged into the australis panel, it looks strange because the panel is designed for vertical buttons.

I realize you guys are getting rid of the icon/text concept for Firefox, but it's quite prevalent on other toolbars.

Orientation can be set via CSS, but then it would be up to all of the add-on authors to modify their buttons to have different CSS for the toolbar and for the panel.

What should happen is that when any toolbar button is placed into the panel, it should get the vertical orientation (box-orient CSS).
Flags: needinfo?(mozilla)
Sadly, the documentation says this:

For XUL elements, if the orientation is set using the element's orient attribute, then the style is ignored.

https://developer.mozilla.org/en-US/docs/Web/CSS/box-orient (it's actually -moz-box-orient).

So it doesn't look like this can be overridden via CSS.

Hopefully you guys can figure a way to work around this, otherwise everyone that uses orient="horizontal" will need to switch to CSS.
> So it doesn't look like this can be overridden via CSS.
> Hopefully you guys can figure a way to work around this, otherwise everyone
> that uses orient="horizontal" will need to switch to CSS.

Given that the "problematic code" is what was recommended before, this affects a great many of extensions, and I think this should be fixed in Firefox, not every extension manually.

Suggestion: On startup, remove the XUL orient="" attribute and programmatically add a CSS rule with the same value.

This CSS rule can then be overridden by other CSS, e.g. for the menu. I.e. we have fixed the problem in comment 2.

Alternative would be to fix/remove/change the (XBL?) code that reads the XUL orient="" attribute.
(In reply to Ben Bucksch (:BenB) from comment #4)
> > So it doesn't look like this can be overridden via CSS.
> > Hopefully you guys can figure a way to work around this, otherwise everyone
> > that uses orient="horizontal" will need to switch to CSS.
> 
> Given that the "problematic code" is what was recommended before, this
> affects a great many of extensions, and I think this should be fixed in
> Firefox, not every extension manually.

I'll be happy to review a patch, but this is unlikely to be fixed otherwise. This could be done in the same place where we set the wrap attribute, if and only if the button sets an orient attribute of its own. We should store the original value in another attribute and restore it when the button leaves the menu.
Whiteboard: [Australis:P5] → [Australis:P5][mentor=Gijs][lang=js]
(In reply to :Gijs Kruitbosch from comment #5)
> (In reply to Ben Bucksch (:BenB) from comment #4)
> > > So it doesn't look like this can be overridden via CSS.
> > > Hopefully you guys can figure a way to work around this, otherwise everyone
> > > that uses orient="horizontal" will need to switch to CSS.
> > 
> > Given that the "problematic code" is what was recommended before, this
> > affects a great many of extensions, and I think this should be fixed in
> > Firefox, not every extension manually.
> 
> I'll be happy to review a patch, but this is unlikely to be fixed otherwise.

Sorry, this was meant to say "to be fixed in time for a 29 (or even a 30) release".
Gijs:

How much testing have you guys done with toolbars and Australis?

I grabbed a couple popular toolbars (Facebook toolbar and Web Developers toolbar) and neither interacted properly with the Australis popup (web developers stayed horizontal in the menu, I can't even describe the weirdness Facebook).

I think we should be doing more to let toolbar developers know that they need to do some work to interact properly with Australis...
(In reply to Mike Kaply (:mkaply) from comment #7)
> Gijs:
> 
> How much testing have you guys done with toolbars and Australis?
> 
> I grabbed a couple popular toolbars (Facebook toolbar and Web Developers
> toolbar) and neither interacted properly with the Australis popup (web
> developers stayed horizontal in the menu, I can't even describe the
> weirdness Facebook)

I'm confused. Do you mean the items from these customizable toolbars?
(In reply to :Gijs Kruitbosch from comment #8)
> 
> I'm confused. Do you mean the items from these customizable toolbars?

Yes. buttons dragged from these toolbars (the only two I tried), don't display properly at all in the Australis popup.
(In reply to Mike Kaply (:mkaply) from comment #7)
> How much testing have you guys done with toolbars and Australis?

Mike, please try to keep your comments constructive...
(In reply to Mike de Boer [:mikedeboer] from comment #10)
> (In reply to Mike Kaply (:mkaply) from comment #7)
> > How much testing have you guys done with toolbars and Australis?
> 
> Mike, please try to keep your comments constructive...

My apologies, I didn't mean that to sound disrespectful at all.

I know there are a lot of extensions out there, and it's difficult to test them all. I'm genuinely asking if you guys are aware of the various problems with popular add-on toolbars, or should I open bugs for each of them.
(In reply to Mike Kaply (:mkaply) from comment #11)
> (In reply to Mike de Boer [:mikedeboer] from comment #10)
> > (In reply to Mike Kaply (:mkaply) from comment #7)
> > > How much testing have you guys done with toolbars and Australis?
> > 
> > Mike, please try to keep your comments constructive...
> 
> My apologies, I didn't mean that to sound disrespectful at all.
> 
> I know there are a lot of extensions out there, and it's difficult to test
> them all. I'm genuinely asking if you guys are aware of the various problems
> with popular add-on toolbars, or should I open bugs for each of them.

No to the first question, and probably no to the second question, too, unless you have good reason to believe it's something that Firefox rather than the add-on in question should fix.
Blocks: 971246
> No to the first question, and probably no to the second question, too, unless you have good reason to believe it's something that Firefox rather than the add-on in question should fix.

Well my belief is that we should fix as much in Firefox as is humanly possible. I would hope that is yours as well. We shouldn't break add-ons if we can help it.
(In reply to Mike Kaply (:mkaply) from comment #13)
> > No to the first question, and probably no to the second question, too, unless you have good reason to believe it's something that Firefox rather than the add-on in question should fix.
> 
> Well my belief is that we should fix as much in Firefox as is humanly
> possible. I would hope that is yours as well. We shouldn't break add-ons if
> we can help it.

If it's a choice between 1 add-on that has 5 users doing a 1-line fix and Firefox adding a 1000-line workaround, I think the latter isn't the right choice. So no, I disagree. We need to be smart about where we spend our time, and just blanket saying "don't break add-ons if it's possible not to" isn't helpful.
Gijs, sure. But given that the way that's breaking now was before recommended on the wiki. Presumably many addons are using it. Me alone, I am maintaining 4 addons that do this. I think Mike has some of his own. And we're not the only ones.
And I don't think the fix in Firefox will be 1000 lines, more like 10-30 lines.

What makes it more difficult to fix on the addon's side is that the desired look depends on the place, and there are at least 3 different places: Toolbar, Overflow panel, and Firefox menu panel. They all look different. An addon arguably shouldn't have to care about this at all.
(In reply to Ben Bucksch (:BenB) from comment #15)
> Gijs, sure. But given that the way that's breaking now was before
> recommended on the wiki. Presumably many addons are using it. Me alone, I am
> maintaining 4 addons that do this. I think Mike has some of his own. And
> we're not the only ones.
> And I don't think the fix in Firefox will be 1000 lines, more like 10-30
> lines.
> 
> What makes it more difficult to fix on the addon's side is that the desired
> look depends on the place, and there are at least 3 different places:
> Toolbar, Overflow panel, and Firefox menu panel. They all look different. An
> addon arguably shouldn't have to care about this at all.

Please don't be so disingenuous. You're arguing about this bug, not about the general case which was what the discussion was about. I didn't claim this particular bug would be 1000 lines to fix, I was making the point that not every bug that could be fixed in Firefox, should be fixed in Firefox.

As it is, this bug is pretty low-prio because we have bigger fish to fry for 29. If you think it's so easy to fix, write a patch.

> But given that the way that's breaking now was before recommended on the wiki. 

[citation needed]

I don't know what wiki page you're on about so I have no idea why somebody recommended this.

The fact is, we have boatloads of outdated docs, I'm too busy to fix all of them, and I'm certainly not going to be held personally responsible for doing so.

Setting orient on your button is a Bad Idea. If you Did Not Do That, you wouldn't be having this problem. If you need to force the orientation in a toolbar, use a CSS rule specific to toolbars, and you'll be fine.
> You're arguing about this bug

Of course I am!

> Setting orient on your button is a Bad Idea

Then remove the XUL attribute.
> remove the XUL attribute.

FWIW, it seems that would actually fix this bug. See comment 3 and 4.
(In reply to Ben Bucksch (:BenB) from comment #4)
> Given that the "problematic code" is what was recommended before, this
> affects a great many of extensions,

Do you have data to support this assertion?

Add-on bugs should be fixed in add-on code, so I'm tempted to wontfix this.
Closing. There doesn't seem to be any evidence that this is an outstanding case where putting a hack in browser code would be justified. This seems like an average compatibility issue where add-ons just need to be updated because they made assumptions that aren't true anymore.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → WONTFIX
Dao, sorry, but you had asked for facts, and I needed to gather them first, and also I was away today and didn't have time to respond. FWIW, you gave me less than 24h hours.

Facts:
* Toolbar buttons with labels had their label below the icon and dramatically increased height of the whole toolbar
* The fix was to put an orient="" XUL attribute on the toolbar button. (CSS might have also worked, or not at that time. Even so, the XUL attribute was a legit fix.)
* Therefore, the addon code is not a "bug", but was in fact necessary.

This is why I think that many extensions that used toolbar buttons will have this problem. I cannot provide hard numbers, because I don't have access to the extension code repository.

Now, that code breaks
* due to changes in Firefox
* only in one place in Firefox

FWIW, this is fixed in my addons. I merely try to help the addons.
(In reply to Ben Bucksch (:BenB) from comment #21)
> Dao, sorry, but you had asked for facts, and I needed to gather them first,
> and also I was away today and didn't have time to respond. FWIW, you gave me
> less than 24h hours.
> 
> Facts:
> * Toolbar buttons with labels had their label below the icon and
> dramatically increased height of the whole toolbar

Where/how? All the buttons in Firefox's navbar have labels and they don't even show up by default, and never have (unless you went and changed the toolbar's mode in the customization dialog).

I'm still hoping someone can point to the wiki page which recommended orient="horizontal" so we can fix it.
(In reply to :Gijs Kruitbosch from comment #22)

> I'm still hoping someone can point to the wiki page which recommended
> orient="horizontal" so we can fix it.

The devmo toolbarbutton articles lists orient as an attribute to use and the CSS as an afterthought. I'm looking for other instances.

As I was thinking about this, though, I think it should be pretty straightforward to fix.

Couldn't we just take away the xbl:inherits on the toolbarbutton XBL and add a CSS selector rule for the toolbarbutton that changes -moz-box-orient based on the orient attribute?
(In reply to Mike Kaply (:mkaply) from comment #23)
> (In reply to :Gijs Kruitbosch from comment #22)
> 
> > I'm still hoping someone can point to the wiki page which recommended
> > orient="horizontal" so we can fix it.
> 
> The devmo toolbarbutton articles lists orient as an attribute to use and the
> CSS as an afterthought. I'm looking for other instances.

Do you mean https://developer.mozilla.org/en-US/docs/XUL/toolbarbutton ? That's hardly a "recommendation"...

> Couldn't we just take away the xbl:inherits on the toolbarbutton XBL and add
> a CSS selector rule for the toolbarbutton that changes -moz-box-orient based
> on the orient attribute?

That would be unlikely to help, because it's the container's (ie toplevel toolbarbutton's) orient'ing that's causing the issue in the first place, and that's where people are setting it. How it inherits into the children isn't important.


I think Dão's call here is correct.
Whiteboard: [Australis:P5][mentor=Gijs][lang=js] → [Australis:P5]
> Where/how? All the buttons in Firefox's navbar have labels and they don't even show up by default, and never have (unless you went and changed the toolbar's mode in the customization dialog).

A) We want to work even if the user does choose to enable labels. Obviously we don't want to be broken in non-default settings.
B) We make a separate toolbar, which does have labels enabled by default (if enough space). Nonetheless, we support Toolbar Customization, so we need to work in all places, including overflow menu and firefox menu.
As said, *we* don't have an actual problem anymore. I just want to avoid that other authors have the same pain.
(In reply to Ben Bucksch (:BenB) from comment #25)
> > Where/how? All the buttons in Firefox's navbar have labels and they don't even show up by default, and never have (unless you went and changed the toolbar's mode in the customization dialog).
> 
> A) We want to work even if the user does choose to enable labels. Obviously
> we don't want to be broken in non-default settings.

In this case, most of the default buttons on the navbar use a vertical orientation, so that's clearly not a reason to use the orient attribute to make your add-on button the odd one out. (I only see a horizontal one for the bookmarks menu button, which is new, so that probably wasn't an issue when these add-ons were written)

> B) We make a separate toolbar, which does have labels enabled by default (if
> enough space). Nonetheless, we support Toolbar Customization, so we need to
> work in all places, including overflow menu and firefox menu.

Of course, which is my CSS is the perfect thing to use, and the hardcoded attribute is just weird. Note how, for example, the bookmarks toolbar uses horizontal orientation, even though the navbar's is vertical. (The Home button gains a label in the bookmarks toolbar, for instance, and it's shown horizontally.)

I still haven't seen any evidence of a previous 'recommendations' to use this attribute, nor of some sensible reason as to why it was necessary.
(In reply to :Gijs Kruitbosch from comment #27)

> I still haven't seen any evidence of a previous 'recommendations' to use
> this attribute, nor of some sensible reason as to why it was necessary.

We should have AMO scan the add-on source code.

I know when I picked two toolbars at random (facebook and web developer), neither worked properly in the australis panel.
(In reply to Mike Kaply (:mkaply) from comment #28)
> (In reply to :Gijs Kruitbosch from comment #27)
> 
> > I still haven't seen any evidence of a previous 'recommendations' to use
> > this attribute, nor of some sensible reason as to why it was necessary.
> 
> We should have AMO scan the add-on source code.
> 
> I know when I picked two toolbars at random (facebook and web developer),
> neither worked properly in the australis panel.

I have access to the MXR instance (but not to complicated manual queries) at https://mxr.mozilla.org/addons/ and already tried this. Unfortunately it's not easily possible because the attributes are used on other types of nodes (so a single-line-matching grep pattern isn't likely to catch stuff) and/or sometimes assigned through JS. Conversely, some of the toolbar buttons are in sidebars or otherwise not customizable, and so their orientation is moot.

In any case, a query for:

toolbarbutton.*orient="ho

https://mxr.mozilla.org/addons/search?string=toolbarbutton.*orient%3D%22ho&regexp=1&find=\.xul%24&findi=\.xul%24&filter=^[^\0]*%24&hitlimit=&tree=addons

(there's a limit on the length of the query, and there's no other sensible values)

turns up results from 55 add-ons. I spot-checked some user numbers (most were under 100, some only 1 or 14 or something). There was only 1 significantly used add-on, which was Stylish, which I'm sure will upgrade as well if they haven't already (the MXR instance is sometimes a little behind) and so I don't think this is worth pursuing further. We'll do what we can in bug 971246, but I don't think we should add dedicated code just for this.
Status: RESOLVED → VERIFIED
Gijs, thanks for checking.
Yes, thanks. I think we should update devmo and have something in the Australis add-on docs that says "don't use orient for your toolbarbutton or it won't display properly in the panel"
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: