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

RESOLVED WONTFIX

Status

()

Firefox
Theme
RESOLVED WONTFIX
5 years ago
5 years ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

(Blocks: 1 bug)

Trunk
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:M?][Australis:P3])

Attachments

(1 attachment, 1 obsolete attachment)

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
If something is broken, what's the downside of seeing /immediately/ that it's broken? This isn't something end users should encounter anyway.
(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@.
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.
(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?
(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...
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.
So should the spritesheet only be applied to @primaryToolbarButtons@[customizable-areatype] ?
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).
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.
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.
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.
(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.
Created attachment 774235 [details] [diff] [review]
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).
Assignee: nobody → mconley
Status: NEW → ASSIGNED
Comment on attachment 774235 [details] [diff] [review]
Patch v1

What do you think Jared? Too hacky?
Attachment #774235 - Flags: review?(jaws)
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.
(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).
Created attachment 774265 [details] [diff] [review]
Patch v1.1

Removing customizableui-areatype from the chevron, because it doesn't need it. Also, I forgot about Retina.
Attachment #774235 - Attachment is obsolete: true
Attachment #774235 - Flags: review?(jaws)
Comment on attachment 774265 [details] [diff] [review]
Patch v1.1

What do you think, Jared?
Attachment #774265 - Flags: review?(jaws)
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 on attachment 774265 [details] [diff] [review]
Patch v1.1

Also, this still seems to be missing zoom and edit controls.
Attachment #774265 - Flags: review-
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.
Attachment #774265 - Flags: review?(jaws)
Cool.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.