888787 - FoxyProxy toolbar button is rendered as a giant sprite sheet (with many icons)

Status: VERIFIED FIXED

(Core :: XBL, defect)

Description

[Shaddy Baddah]

Attachment: Bug with 25.0a1 (2013-06-30) with icons on navigation toolbar

I haven't done a bisect, but I am fairly sure this has appeared the first time for Nightly build for 25.0a1 (2013-06-30):

http://hg.mozilla.org/mozilla-central/rev/cbb24a4a96af

If not, it'll be no more than two builds earlier, as I have been allowing the update to take no more than a day or two from when prompted.

When I usually have a button on the Navigation Toolbar for foxyproxy, I am seeing a wider button with what seems to be the full list of standard icons available for display on the toolbar. See the image attached.

The button is clickable, and I get the foxyproxy pop-up as usual.

Comment 1

[Shaddy Baddah]

Confirmed that last known good was 25.0a1 (2013-06-28):

http://hg.mozilla.org/mozilla-central/rev/8e3a124c9c1a

Comment 2

[Alice0775 White]

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/c5ce065936fa
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20130628 Firefox/25.0 ID:20130628182742
Bad:
http://hg.mozilla.org/mozilla-central/rev/cbb24a4a96af
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20130629 Firefox/25.0 ID:20130629065543
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c5ce065936fa&tochange=cbb24a4a96af

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/4df4f2767a69
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20130628 Firefox/25.0 ID:20130628184843
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/1b8207252e9c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20130628 Firefox/25.0 ID:20130628185242
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4df4f2767a69&tochange=1b8207252e9c

Regressed by : Bug 653881

Comment 3

[mynockx]

It's not just FoxyProxy.  I'm getting the same thing with Download Statusbar Mini Mode.  I've also heard that it's happening with other icons as well (Advanced Cookie Manager, and Adblock Plus Dev build).

Comment 4

[Daniel Holbert [:dholbert]]

Just for clarity, here are STR:
 1. Install FoxyProxy Basic, from here:
      https://addons.mozilla.org/en-US/firefox/addon/foxyproxy-basic/
 2. Restart Nightly when prompted, to complete the addon install.

EXPECTED RESULTS: FoxyProxy icon to right of URL bar
ACTUAL RESUTLS: Mega multi-icon button to right of URL bar (as shown in attachment 769546 [details])

If you switch back to a normal version of Firefox, you get EXPECTED RESULTS. But once you switch back to a recent nightly, you get ACTUAL RESULTS.

Comment 5

[Yuan Pengfei]

One more symptom:
The browser proxy configuration does not take effect if FoxyProxy Basic is installed.
It comes back normal after FoxyProxy Basic is removed.

Comment 6

[Jim Jeffery not reading bug-mail 1/2/11]

Add FlashBlock as also showing Icon-sprites on the Customize Pallet.  Looks OK in the nav-bar.

Comment 7

[:Gijs (out for now; he/him)]

So, AFAICT quickly, this is because the toolbarbutton binding includes these children by default:

      <children includes="observes|template|menupopup|panel|tooltip"/>

( http://hg.mozilla.org/mozilla-central/annotate/b48e06621dc9/toolkit/content/widgets/toolbarbutton.xml#l19 )

and FoxyProxy overlays an <svg> element into its toolbarbutton:

https://mxr.mozilla.org/addons/source/15023/chrome/content/firefoxOverlay.xul#102

https://mxr.mozilla.org/addons/source/15023/chrome/content/firefoxOverlay-svg.xul#41

Looks like the old XBL implementation was cool with still adding non-matching children somewhere (I'd guess last, or possibly first?), the new one is not. If I'm right a trivial fix would be adding svg (and probably image for some of the other add-ons mentioned) to the list of includes. Not entirely sure that's what we want in this case - it's probably less trivial to auto-add a children element afterwards that includes the remaining items? mrbkap, does this explanation sound plausible to you? What's the right approach here?

Comment 8

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch from comment #7)
> So, AFAICT quickly, this is because the toolbarbutton binding includes these
> children by default:
> 
>       <children includes="observes|template|menupopup|panel|tooltip"/>
> 
> (
> http://hg.mozilla.org/mozilla-central/annotate/b48e06621dc9/toolkit/content/
> widgets/toolbarbutton.xml#l19 )
> 
> and FoxyProxy overlays an <svg> element into its toolbarbutton:
> 
> https://mxr.mozilla.org/addons/source/15023/chrome/content/firefoxOverlay.
> xul#102
> 
> https://mxr.mozilla.org/addons/source/15023/chrome/content/firefoxOverlay-
> svg.xul#41
> 

This doesn't seem to be the end of it, because just adding "svg" or "svg:svg" (?) or replacing it with <children /> doesn't seem to help. Neither does adjusting the overlay so the nodes match (right now it's hbox vs. toolbarbutton; honestly not even sure that matters).

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to :Gijs Kruitbosch from comment #7)
> If I'm right a trivial fix would be adding svg (and probably
> image for some of the other add-ons mentioned) to the list of includes.

I doubt that we can get away with one-off wallpapers for specific cases, I think we need to make the general case work the way it used to.

Comment 10

[:Gijs (out for now; he/him)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #9)
> (In reply to :Gijs Kruitbosch from comment #7)
> > If I'm right a trivial fix would be adding svg (and probably
> > image for some of the other add-ons mentioned) to the list of includes.
> 
> I doubt that we can get away with one-off wallpapers for specific cases, I
> think we need to make the general case work the way it used to.

I see your point wrt regressions, but considering what the code is doing here I'm actually somewhat surprised this ever worked at all, and do think that there is a point in not having some of the children be included in particular places. I don't know if XBL has an automatic fallback to put not-already-placed children somewhere else by default (hence also the needinfo request), in which case my point is moot and restricting what children would be allowed as part of an element was never possible in the first place.

Regardless, I tried this fix and it didn't help (see comment 9), so there's something else going on that I don't understand, and unfortunately I don't have time to look into it right now.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Hey Gijs, thanks for looking into this! To answer some of your questions, the old behavior that you were trying to figure out (and IMO doesn't make sense, which is why we tried to do away with it) was that if there was a child of the bound element (that wasn't <xul:template> or <xul:observer>) that didn't match any of the <children> in the binding's <content>, we would drop the <content> from the binding and continue using the bound element's children to render it.

The change in behavior from bug 653881 was to keep the <content> and any children that didn't match a <children> were simply not displayed – incidentally, this is what web components specifies. My hope was that nobody was using this purposefully and that uses of it were few and far between, but this bug (as well as bug 889098 and a couple of other bugs fixed during the run-up for bug 653881) have convinced me otherwise.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Restore the old behavior

I'm running this through try right now. Testcases to come.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Reftest

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Restore the old behavior v2

Try server pointed out that this now re-hides bug 758695 so we don't need to mark it as asserting. Also, debugging bug 888967 showed that my previous patch missed the no <children> at all case.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Reftest v2

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

The rest of the try sever run was green.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/aed4063ae237
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b6687eeb804f

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/aed4063ae237
https://hg.mozilla.org/mozilla-central/rev/b6687eeb804f

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix a typo

This has r=sicking in person. I made a dumb typo in the first patch: we should be ignoring <xul:observes> not <xul:observer> (which doesn't exist).

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 777455 [details] [diff] [review]
Fix a typo

I didn't realize that Neil had already filed bug 895129 when I attached this here. I'll move this patch to the other bug.

Comment 21

[Shaddy Baddah]

(In reply to Shaddy Baddah from comment #1)
> Confirmed that last known good was 25.0a1 (2013-06-28):
> 
> http://hg.mozilla.org/mozilla-central/rev/8e3a124c9c1a

I just want to make a correction on this. I've realised, in bisecting for another bug 889232, that I i caught the wrong last known good. The last known good was actually:

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-06-29-03-11-16-mozilla-central/

with about:buildconfig showing:

http://hg.mozilla.org/mozilla-central/rev/c5ce065936fa

I thought to log it here for two reasons:
* just in case the inaccuracy should cause any future confusion.
* the closeness in builds (only one apart) to bug 889232 may be significant to that bug. If there is any relationship at all.

Comment 22

[Ada [:adalucinet]]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:25.0) Gecko/20100101 Firefox/25.0
Mozilla/5.0 (X11; Linux x86_64; rv:25.0) Gecko/20100101 Firefox/25.0

Reproduced this issue with Nightly 25.0a1 (2013-07-13).
Verified as fixed with Firefox 25 beta 2 (Build ID: 20130923194050).