942144 - Australis - Legacy button not positioned correctly in Australis panel

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Mike Kaply [:mkaply]]

Attachment: Screenshot of offset icon

I have a legacy toolbar. I've changed by button orientation to vertical to match Australis.

When I put the button in the Australis panel, the button is offset by a few pixels (image attached).

The button is using standard toolbar CSS - class="chromeclass-toolbar-additional toolbarbutton-1"

The button should be a perfect aligned square just like the other buttons.

Comment 1

[:Gijs (he/him)]

(In reply to Mike Kaply (:mkaply) from comment #1)
> Created attachment 8336789 [details]
> Screenshot of offset icon

There's an existing bug about the placeholders being wrong. Can you (a) update to current nightly (the styling in the panel changed somewhat dramatically last night), and (b) take a screenshot with the button in the middle of the other icons? Thanks!

Comment 2

[Mike Kaply [:mkaply]]

Attachment: Requested screenshot

This is the screenshot you requested on the right, a redo of my original case in the middle, and doing my test with a built in button on the left.

Oddly, my case looks much better now (to me), but the built in button case looks worse.

I guess it depends on whether they are supposed to be centered or bottom aligned with each other.

The text in the legacy button is noticeably higher as well.

Comment 4

[Mike Kaply [:mkaply]]

Attachment: panelmenu.png

Buttons with menus (not menu buttons) also cause some weirdness.

The area for the button is slightly too large, so it causes the buttons in the toolbarpanel to oveflow, so you can't place anything to the far right.

Comment 5

[Mike Kaply [:mkaply]]

So my problem with my login button is because we have a toolbarbutton as a child of a toolbaritem and the CSS in panelUIOverlay.css only applies if the toolbarbutton is at the top level (because that's where cui-areatype="menu-panel gets set)

toolbarbutton[cui-areatype="menu-panel"] {
  margin-right: 0;
  margin-left: 0;
  margin-bottom: 0;
}

I imagine you're going to run into a lot of these edge cases...

Comment 6

[:Gijs (he/him)]

(In reply to Mike Kaply (:mkaply) from comment #5)
> So my problem with my login button is because we have a toolbarbutton as a
> child of a toolbaritem and the CSS in panelUIOverlay.css only applies if the
> toolbarbutton is at the top level (because that's where
> cui-areatype="menu-panel gets set)
> 
> toolbarbutton[cui-areatype="menu-panel"] {
>   margin-right: 0;
>   margin-left: 0;
>   margin-bottom: 0;
> }
> 
> I imagine you're going to run into a lot of these edge cases...

Why do you need a toolbaritem rather than just a toolbarbutton?

Comment 7

[:Gijs (he/him)]

Generally, I think the assumption is we'll do our best to make single toolbarbuttons look good by default, but toolbaritems are meant to take care of their own styling, as they could literally contain anything. See also e.g. the bookmarks toolbar items, the search bar, etc.

Comment 8

[Mike Kaply [:mkaply]]

> Why do you need a toolbaritem rather than just a toolbarbutton?

We have three toolbar buttons in a toolbaritem and switch them out as necessary.

This works because previously, the styling of a toolbarbutton was based on the CSS classes toolbarbutton-1 and chromeclass-toolbar-additional. This allowed toolbarbuttons to be styled correctly even if they were inside a toolbaritem (or anywhere else for that major).

You've made a major change with Australis. You've changed toolbarbuttons to have a certain look/feel in certain places regardless of the style. 

This code in panelUIOverlay.inc.css is not correct (or all of the similar code).

 #widget-overflow-list > toolbarbutton {

It should be:

#widget-overflow-list toolbarbutton[toolbarbutton-1]

See:

https://developer.mozilla.org/en-US/docs/XUL/Toolbars/Creating_toolbar_buttons

Comment 9

[Ben Bucksch (:BenB)]

I agree with Mike's comment 8.

We use <toolbaritem> to group a number of elements that need to stay together, e.g. a search field with a search button. We thus place a <toolbarbutton> plus other elements together in the <toolbaritem>

The styling of <toolbarbutton> needs to apply, no matter where (within <toolbar> or dropdown) it is. <toolbarbutton> is a standard element and should work everywhere in a toolbar, even if I need to add a container around it.

That's the idea of XUL, actually. Imagine a <textfield> that only works when it's immediately underneath <form>, but breaks (in styling) when a <div> is between them. That makes no sense.

Please just remove the ">"

Comment 10

[:Gijs (he/him)]

(In reply to Mike Kaply (:mkaply) from comment #8)
> This code in panelUIOverlay.inc.css is not correct (or all of the similar
> code).
> 
>  #widget-overflow-list > toolbarbutton {
> 
> It should be:
> 
> #widget-overflow-list toolbarbutton[toolbarbutton-1]
> (sic; I think you mean .toolbarbutton-1, but that's irrelevant)

This won't work well either, because the specificity of that rule is much higher. For instance, it will match both toolbar buttons involved in a toolbarbutton[type="menu-button"].

I'm very much not convinced that changing this selector will solve more problems than it would cause.

Comment 11

[Mike Kaply [:mkaply]]

> This won't work well either, because the specificity of that rule is much higher. For instance, it will match both toolbar buttons involved in a toolbarbutton[type="menu-button"].

That's why if you look in the source code, they have rules to handle the type="menu-button" cases, like here:

http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.css#385

The problem is you're only applying these styles to direct children of the list.

Whereas previously, by using the toolbarbutton-1 class, you could get correct toolbarbutton styling no matter what your parentage was.

You've broken that model.

I should be able to give a button the "toolbarbutton-1" class and anywhere I put it, it should look correct. Even if it is nested in a toolbaritem.

Comment 12

[:Gijs (he/him)]

(In reply to Mike Kaply (:mkaply) from comment #11)
> > This won't work well either, because the specificity of that rule is much higher. For instance, it will match both toolbar buttons involved in a toolbarbutton[type="menu-button"].
> 
> That's why if you look in the source code, they have rules to handle the
> type="menu-button" cases, like here:
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.
> css#385

The solution there isn't the :not([type="menu-button"]) - the menu-button is an example, hence "For instance" - the solution is using a lower-specificity rule (in particular, 'toolbar' is a tag selector rather than an id selector like #widget-overflow-list). That's not easy to do in these cases, and I'm not convinced it won't cause other problems. And, again, descendant selectors are evil from a performance perspective.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Gijs and I are going to investigate potential solutions to this tomorrow.

Comment 14

[Ben Bucksch (:BenB)]

Thank you!

Comment 15

[Mike Conley (:mconley) (:⚙️)]

I think I've found a pretty simple solution for standard type="button" buttons.

type="menu" and type="menu-button" is going to take a bit more work. Let me post a patch for the "button" case, anyway.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

er, type="button" isn't actually a thing. I guess I just mean buttons that are not type="menu" or type="menu-button".

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

I tried a number of different solutions, and I think this one is probably the simplest. I don't normally like to use !important, but the specificity of the rule that sets the margin on toolbarbutton-1's is really too high[1] for me to do anything else (except creating a rule that's more specific, which in this case is probably a worse approach).

[1]: It's too high because it's using toolbarbutton-1:not(-moz-any(list of built-in toolbarbutton ids)).

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8361657 [details] [diff] [review]
Patch v1

Thoughts on this, Jared?

Comment 19

[Ben Bucksch (:BenB)]

Thank you. However, most of our problems are with menu-buttons, indeed.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

I believe menu-buttons are going to be dealt with in bug 940307.

Depending on the solution to it, we might put the fix for type="menu" there or here. Unsure yet.

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8361657 [details] [diff] [review]
Patch v1

We'll likely fix "menu-button" and "menu" type buttons in bug 940307.

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8361657 [details] [diff] [review]
Patch v1

After talking with jaws, we're going to go with a descendant selector instead of the !important.

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v2

Ok, forget !imporant. Let's use a descendant selector.

Comment 24

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8362619 [details] [diff] [review]
Patch v2

How's this?

Comment 25

[Mike Conley (:mconley) (:⚙️)]

Thanks! Landed on fx-team as:

remote: https://hg.mozilla.org/integration/fx-team/rev/a6dd7dcee301

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a6dd7dcee301