Closed Bug 888787 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: XBL, defect)

x86_64
All
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla25
Tracking Status
firefox24 --- unaffected
firefox25 + verified

People

(Reporter: beryllium-bugs, Assigned: mrbkap)

References

Details

(Keywords: addon-compat, regression)

Attachments

(3 files, 3 obsolete files)

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.
Confirmed that last known good was 25.0a1 (2013-06-28):

http://hg.mozilla.org/mozilla-central/rev/8e3a124c9c1a
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
Blocks: 653881
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Keywords: addon-compat
OS: Windows 7 → All
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:
      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.
Summary: Navigation toolbar displaying extra icons. Possibly to do with foxyproxy → FoxyProxy toolbar button is rendered as a giant sprite sheet (with many icons)
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.
Component: Toolbars and Customization → XBL
Product: Firefox → Core
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?
Flags: needinfo?(mrbkap)
(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).
Assignee: nobody → mrbkap
(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 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.
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.
Flags: needinfo?(mrbkap)
Attached patch Restore the old behavior (obsolete) — Splinter Review
I'm running this through try right now. Testcases to come.
Attachment #774151 - Flags: review?(jonas)
Attached patch Reftest (obsolete) — Splinter Review
Attachment #774175 - Flags: review?(jonas)
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.
Attachment #774151 - Attachment is obsolete: true
Attachment #774175 - Attachment is obsolete: true
Attachment #774151 - Flags: review?(jonas)
Attachment #774175 - Flags: review?(jonas)
Attachment #774226 - Flags: review?(jonas)
Attached patch Reftest v2Splinter Review
Attachment #774228 - Flags: review?(jonas)
The rest of the try sever run was green.
https://hg.mozilla.org/mozilla-central/rev/aed4063ae237
https://hg.mozilla.org/mozilla-central/rev/b6687eeb804f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Blocks: 895129
Attached patch Fix a typo (obsolete) — Splinter Review
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).
Attachment #777455 - Flags: review+
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.
Attachment #777455 - Attachment is obsolete: true
Attachment #777455 - Flags: review+
(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.
Keywords: verifyme
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).
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.