Closed
Bug 888787
Opened 11 years ago
Closed 11 years ago
FoxyProxy toolbar button is rendered as a giant sprite sheet (with many icons)
Categories
(Core :: XBL, defect)
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)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
sicking
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
sicking
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•11 years ago
|
||
Confirmed that last known good was 25.0a1 (2013-06-28):
http://hg.mozilla.org/mozilla-central/rev/8e3a124c9c1a
Comment 2•11 years ago
|
||
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
status-firefox25:
--- → affected
tracking-firefox25:
--- → ?
Ever confirmed: true
Keywords: regression
Updated•11 years ago
|
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).
Comment 4•11 years ago
|
||
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.
Updated•11 years ago
|
Summary: Navigation toolbar displaying extra icons. Possibly to do with foxyproxy → FoxyProxy toolbar button is rendered as a giant sprite sheet (with many icons)
Comment 5•11 years ago
|
||
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•11 years ago
|
||
Add FlashBlock as also showing Icon-sprites on the Customize Pallet. Looks OK in the nav-bar.
Updated•11 years ago
|
Component: Toolbars and Customization → XBL
Product: Firefox → Core
Comment 7•11 years ago
|
||
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)
Comment 8•11 years ago
|
||
(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).
Updated•11 years ago
|
Comment 9•11 years ago
|
||
(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•11 years ago
|
||
(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.
Assignee | ||
Comment 11•11 years ago
|
||
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)
Assignee | ||
Comment 12•11 years ago
|
||
I'm running this through try right now. Testcases to come.
Attachment #774151 -
Flags: review?(jonas)
Assignee | ||
Comment 13•11 years ago
|
||
Attachment #774175 -
Flags: review?(jonas)
Assignee | ||
Comment 14•11 years ago
|
||
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)
Assignee | ||
Comment 15•11 years ago
|
||
Attachment #774228 -
Flags: review?(jonas)
Assignee | ||
Comment 16•11 years ago
|
||
The rest of the try sever run was green.
Attachment #774228 -
Flags: review?(jonas) → review+
Attachment #774226 -
Flags: review?(jonas) → review+
Assignee | ||
Comment 17•11 years ago
|
||
Comment 18•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/aed4063ae237
https://hg.mozilla.org/mozilla-central/rev/b6687eeb804f
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Assignee | ||
Comment 19•11 years ago
|
||
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+
Assignee | ||
Comment 20•11 years ago
|
||
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+
Reporter | ||
Comment 21•11 years ago
|
||
(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•11 years ago
|
||
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).
You need to log in
before you can comment on or make changes to this bug.
Description
•