892628 - Find a way of using a fallback icon for toolbarbutton-1's to avoid showing the entire spritesheet.

Status: RESOLVED WONTFIX

(Firefox :: Theme, defect)

Description

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

There have been a few cases (both on UX and m-c)[1] where toolbar buttons for one reason or another, show the *entire toolbar button spritesheet*. This is pretty disruptive to the UI, and with our new huge toolbar button spritesheets, has the potential to push critical bits of the UI offscreen.

Plus, it just looks plain awful. I mean, look at this: http://i.imgur.com/ZAukmwT.png . That's terrible.

The toolbar button spritesheet contains a "no-icon icon" to be used as a fallback for these sorts of situations. We should find a way of using that icon instead of the big sheet for items that inherit toolbarbutton-1 but don't set a region.

[1] Like bug 892509 and bug 888787

Comment 1

[Dão Gottwald [:dao]]

If something is broken, what's the downside of seeing /immediately/ that it's broken? This isn't something end users should encounter anyway.

Comment 2

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

(In reply to Dão Gottwald [:dao] from comment #1)
> If something is broken, what's the downside of seeing /immediately/ that
> it's broken? This isn't something end users should encounter anyway.

"Should" and "do" are two separate things. We already have end-users who are encountering this through poorly written add-ons.

We can just pull that out in to a separate image (but leave it in the Toolbar.png image). Toolbar buttons that want something out of the sprite sheet should opt-in to it through @primaryToolbarButtons@.

Comment 3

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

The other part of this brokenness, is that when we show the full toolbar sprite we end up pushing many of the toolbar items in to overflow mode, so not only is it extremely visible, but it can also make the browser quite unusable for people until the issue is fixed. Moving this out to a separate image will allow the browser to be usable until the add-on is fixed.

Comment 4

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] from comment #2)
> (In reply to Dão Gottwald [:dao] from comment #1)
> > If something is broken, what's the downside of seeing /immediately/ that
> > it's broken? This isn't something end users should encounter anyway.
> 
> "Should" and "do" are two separate things. We already have end-users who are
> encountering this through poorly written add-ons.

Poorly written in what way? Given <http://hg.mozilla.org/projects/ux/file/5a294331b6b5/browser/themes/shared/toolbarbuttons.inc.css#l1>, using toolbarbutton-1 isn't enough to get the image anymore, is it?

Comment 5

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

(In reply to Dão Gottwald [:dao] from comment #4)
> (In reply to Jared Wein [:jaws] from comment #2)
> > (In reply to Dão Gottwald [:dao] from comment #1)
> > > If something is broken, what's the downside of seeing /immediately/ that
> > > it's broken? This isn't something end users should encounter anyway.
> > 
> > "Should" and "do" are two separate things. We already have end-users who are
> > encountering this through poorly written add-ons.
> 
> Poorly written in what way? Given
> <http://hg.mozilla.org/projects/ux/file/5a294331b6b5/browser/themes/shared/
> toolbarbuttons.inc.css#l1>, using toolbarbutton-1 isn't enough to get the
> image anymore, is it?

That's a good point - and I should have known that, since I wrote the patch.

So how did bug 892509 happen then? I wonder if there's a hole in our rules for one of our primary buttons that this user fell into...

Comment 6

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

Ok, I have a hypothesis:

The moz-image-region rules are for the most part only applied to items with customizableui-areatype="toolbar". That attribute it set when building the toolbar area.

If that buildArea *fails*, and errors out before it can set those attributes, that'd explain the big spritesheet.

Comment 7

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

So should the spritesheet only be applied to @primaryToolbarButtons@[customizable-areatype] ?

Comment 8

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

Yeah, and then we need to special-case #back-button and #forward-button, since they don't get that attribute (since they're held within the unified toolbarbuttonitem).

Comment 9

[Dão Gottwald [:dao]]

Um, can we prevent buildArea from failing? Otherwise you'd get no icon at all in the case the customizable-areatype attribute isn't set and Toolbar.png was depending on it.

In other words, we should fix whatever is broken here rather than trying to wallpapering over it.

Comment 10

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

Yeah, I agree that we should fix the root cause, but we should also make the icon here atomic, so that if a reflow is forced between these two users won't see a flash of Toolbar.png.

Comment 11

[Dão Gottwald [:dao]]

If there's a point in time where toolbar buttons get rendered without their icons (be it with no icon at all or all of Toolbar.png instead), then that's a bug in our code, right? I don't think we want the flash you seem to be concerned about to be less obtrusive; it shouldn't exist at all.

Comment 12

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

(In reply to Dão Gottwald [:dao] from comment #9)
> Um, can we prevent buildArea from failing? Otherwise you'd get no icon at
> all in the case the customizable-areatype attribute isn't set and
> Toolbar.png was depending on it.
> 
> In other words, we should fix whatever is broken here rather than trying to
> wallpapering over it.

Of course. If my hypothesis is correct, I plan on finding the cause of the buildArea failure in bug 892509.

Comment 13

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

Attachment: Patch v1

I opted to just add customizableui-areatype="toolbar" to the items that are not directly run through buildArea (back, forward, menu button, overflow chevron).

Comment 14

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

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

What do you think Jared? Too hacky?

Comment 15

[Dão Gottwald [:dao]]

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

I don't see why this should be necessary, per previous comments.

By the way, I think you might be missing zoom controls here, for instance, unless I have a wrong idea of what kind of nodes get the customizableui-areatype attribute. Anyway, I don't see the point of this in the first place.

Comment 16

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

(In reply to Dão Gottwald [:dao] from comment #15)
> Comment on attachment 774235 [details] [diff] [review]
> Patch v1
> 
> Anyway, I don't see the point of this in the first place.

Ok, I'll try to sum it up, because I still think this patch is worth pursuing. Feel free to jump in where I go wrong:

1) As it currently stands, all primaryToolbarButtons have the massive spritesheet set on them via list-style-image. This is true regardless of where they're positioned.
2) buildArea is a function that's called when a toolbar or the menu panel starts to construct itself. Part of that functions job is to give each child an attribute indicating the type of area they're in. This gives us a number of advantages, which I won't list here. We also use that attribute to know that we're supposed to crop the list-style-image to that toolbarbuttons appropriate coordinates.
3) The reason why we set the list-style-image on primaryToolbarButtons, and not on primaryToolbarButtons[customizableui-areatype="toolbar"] is because there are some items that do not get attributes set on them via buildArea. The menu button, for example, is outside of the toolbar customization target. The back and forward buttons are special because they get icons, and their *parent* gets the customizableui-areatype attribute set.

So, I suspect that bug 892509 is caused when buildArea fails out for some reason (yet to be determined), and so the customizableui-areatype attribute is not getting set. This means that one or more items are just displaying the entire spritesheet.

This is unfortunate for a number of reasons:

1) It looks bloody awful, and confusing
2) It can push important UI elements out of view, or into overflow.

I would even go so far as to say that it threatens to make the browser un-usable. Or, at the very least, very uncomfortable to use.

Clearly we want to determine what (if anything) is happening in buildArea to cause this. But at the same time, I don't think we want the UI to completely explode like this when something goes wrong in buildArea. An error message should definitely go to the console. But I'm not convinced that causing one or more full sheets to be displayed is desirable at all, ever. So while one might call this "wallpapering", I think this is just being more resilient against such failures. And who doesn't want that?

My solution is to only set the sprite if customizableui-areatype is also set on the button. This means I've had to special-case the back, forward, and menu button. The chevron is also outside the customization target, but it already does its own override in toolbarbuttons.inc.css, so I shouldn't have to do anything there (will update the patch to reflect this).

Comment 17

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

Attachment: Patch v1.1

Removing customizableui-areatype from the chevron, because it doesn't need it. Also, I forgot about Retina.

Comment 18

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

Comment on attachment 774265 [details] [diff] [review]
Patch v1.1

What do you think, Jared?

Comment 19

[Dão Gottwald [:dao]]

Getting the failure reported in the console is missing the point. If buildArea fails, that's a lost cause one way or another (e.g. invisible buttons assuming your patch got landed). As the central piece of code that it appears to be, buildArea should never fail. If we have buggy code in there, we need to fix it and move on. If buildArea somehow provides hooks for external code that we might not control, then it should protect itself against failures in that code by catching exceptions or something.

Comment 20

[Dão Gottwald [:dao]]

Comment on attachment 774265 [details] [diff] [review]
Patch v1.1

Also, this still seems to be missing zoom and edit controls.

Comment 21

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

Comment on attachment 774265 [details] [diff] [review]
Patch v1.1

Review of attachment 774265 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, I agree that we need to catch exceptions within buildArea.

Comment 22

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

Cool.