Created attachment 769546 [details]
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):
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.
Confirmed that last known good was 25.0a1 (2013-06-28):
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20130628 Firefox/25.0 ID:20130628182742
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20130629 Firefox/25.0 ID:20130629065543
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20130628 Firefox/25.0 ID:20130628184843
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20130628 Firefox/25.0 ID:20130628185242
Regressed by : Bug 653881
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).
Just for clarity, here are STR:
1. Install FoxyProxy Basic, from here:
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.
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.
Add FlashBlock as also showing Icon-sprites on the Customize Pallet. Looks OK in the nav-bar.
So, AFAICT quickly, this is because the toolbarbutton binding includes these children by default:
( http://hg.mozilla.org/mozilla-central/annotate/b48e06621dc9/toolkit/content/widgets/toolbarbutton.xml#l19 )
and FoxyProxy overlays an <svg> element into its toolbarbutton:
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?
(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"/>
> widgets/toolbarbutton.xml#l19 )
> and FoxyProxy overlays an <svg> element into its toolbarbutton:
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).
(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.
(In reply to :Gavin Sharp (use email@example.com 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.
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.
Created attachment 774151 [details] [diff] [review]
Restore the old behavior
I'm running this through try right now. Testcases to come.
Created attachment 774175 [details] [diff] [review]
Created attachment 774226 [details] [diff] [review]
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.
Created attachment 774228 [details] [diff] [review]
The rest of the try sever run was green.
Created attachment 777455 [details] [diff] [review]
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 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.
(In reply to Shaddy Baddah from comment #1)
> Confirmed that last known good was 25.0a1 (2013-06-28):
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:
with about:buildconfig showing:
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.
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).