Last Comment Bug 888787 - FoxyProxy toolbar button is rendered as a giant sprite sheet (with many icons)
: FoxyProxy toolbar button is rendered as a giant sprite sheet (with many icons)
Status: VERIFIED FIXED
: addon-compat, regression
Product: Core
Classification: Components
Component: XBL (show other bugs)
: Trunk
: x86_64 All
: -- normal (vote)
: mozilla25
Assigned To: Blake Kaplan (:mrbkap)
:
Mentors:
Depends on:
Blocks: 653881 888967 889098 895129
  Show dependency treegraph
 
Reported: 2013-06-30 19:00 PDT by Shaddy Baddah
Modified: 2013-09-25 07:11 PDT (History)
30 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
verified


Attachments
Bug with 25.0a1 (2013-06-30) with icons on navigation toolbar (75.51 KB, image/png)
2013-06-30 19:00 PDT, Shaddy Baddah
no flags Details
Restore the old behavior (1.93 KB, patch)
2013-07-11 11:53 PDT, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
Reftest (3.41 KB, patch)
2013-07-11 12:22 PDT, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
Restore the old behavior v2 (3.17 KB, patch)
2013-07-11 13:28 PDT, Blake Kaplan (:mrbkap)
jonas: review+
Details | Diff | Splinter Review
Reftest v2 (4.44 KB, patch)
2013-07-11 13:28 PDT, Blake Kaplan (:mrbkap)
jonas: review+
Details | Diff | Splinter Review
Fix a typo (4.13 KB, patch)
2013-07-17 16:18 PDT, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review

Description Shaddy Baddah 2013-06-30 19:00:04 PDT
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):

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 2013-06-30 20:41:29 PDT
Confirmed that last known good was 25.0a1 (2013-06-28):

http://hg.mozilla.org/mozilla-central/rev/8e3a124c9c1a
Comment 2 Alice0775 White 2013-06-30 22:31:33 PDT
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 2013-06-30 23:12:06 PDT
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] 2013-06-30 23:24:58 PDT
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 2013-07-01 00:37:42 PDT
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 2013-07-01 04:08:29 PDT
Add FlashBlock as also showing Icon-sprites on the Customize Pallet.  Looks OK in the nav-bar.
Comment 7 :Gijs Kruitbosch (away 26-29 incl.) 2013-07-03 04:22:33 PDT
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 Kruitbosch (away 26-29 incl.) 2013-07-03 05:41:18 PDT
(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: gavin@gavinsharp.com] 2013-07-04 10:44:05 PDT
(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 Kruitbosch (away 26-29 incl.) 2013-07-04 10:49:24 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.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.
Comment 11 Blake Kaplan (:mrbkap) 2013-07-11 11:52:05 PDT
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) 2013-07-11 11:53:01 PDT
Created attachment 774151 [details] [diff] [review]
Restore the old behavior

I'm running this through try right now. Testcases to come.
Comment 13 Blake Kaplan (:mrbkap) 2013-07-11 12:22:05 PDT
Created attachment 774175 [details] [diff] [review]
Reftest
Comment 14 Blake Kaplan (:mrbkap) 2013-07-11 13:28:08 PDT
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.
Comment 15 Blake Kaplan (:mrbkap) 2013-07-11 13:28:36 PDT
Created attachment 774228 [details] [diff] [review]
Reftest v2
Comment 16 Blake Kaplan (:mrbkap) 2013-07-11 14:55:28 PDT
The rest of the try sever run was green.
Comment 19 Blake Kaplan (:mrbkap) 2013-07-17 16:18:58 PDT
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 20 Blake Kaplan (:mrbkap) 2013-07-17 17:51:46 PDT
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 2013-07-28 22:49:50 PDT
(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 Alexandra Lucinet, QA Mentor [:adalucinet] 2013-09-25 07:11:13 PDT
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).

Note You need to log in before you can comment on or make changes to this bug.