Closed Bug 959640 Opened 6 years ago Closed 6 years ago

Have a default icon for add-on SDK widgets in customization view

Categories

(Firefox :: Theme, defect)

28 Branch
x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed

People

(Reporter: phlsa, Assigned: zer0)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P3+])

Attachments

(5 files, 3 obsolete files)

Whiteboard: [Australis:P3]
Attached file Icon sets (obsolete) —
Here's a bunch of PNGs with the new icon inside (last one on the right).
I think the icons for Linux are the same as for Windows at the moment.
Assignee: nobody → gijskruitbosch+bugs
(In reply to Philipp Sackl [:phlsa] from comment #1)
> Created attachment 8360980 [details]
> Icon sets
> 
> Here's a bunch of PNGs with the new icon inside (last one on the right).
> I think the icons for Linux are the same as for Windows at the moment.

There are several different sprite sets for Windows, this zipfile only has one. Can you provide the other ones as well?
Assignee: gijskruitbosch+bugs → philipp
Could you point me to the assets needed?
I can't find them in the source of mozilla-central;
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Philipp Sackl [:phlsa] from comment #3)
> Could you point me to the assets needed?
> I can't find them in the source of mozilla-central;

In m-c, they are:

browser/themes/windows/Toolbar.png
browser/themes/windows/Toolbar-lunaSilver.png
browser/themes/windows/Toolbar-aero.png
browser/themes/windows/Toolbar-inverted.png (for dark LWTs)

There are also separate files for Windows 8, but they've not been checked in yet. See bug 960517.
Flags: needinfo?(gijskruitbosch+bugs)
As discussed in IRC, the icon is not needed for toolbars. We'll keep using just text there to avoid having identical icons in the toolbar or wasting even more space.

I double checked and the icons included in the attachment are all that are needed for the menu panel (Linux is identical to Windows).
Assignee: philipp → gijskruitbosch+bugs
(In reply to Philipp Sackl [:phlsa] from comment #5)
> As discussed in IRC, the icon is not needed for toolbars. We'll keep using
> just text there to avoid having identical icons in the toolbar or wasting
> even more space.
> 
> I double checked and the icons included in the attachment are all that are
> needed for the menu panel (Linux is identical to Windows).

Ehm, actually, there's two different menuPanel sprites - one 'normal' and one for aero. The difference is the white highlight. See http://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/menuPanel-aero.png vs. http://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/menuPanel.png . Your zipfile only has one. :-(
Assignee: gijskruitbosch+bugs → philipp
(In reply to :Gijs Kruitbosch from comment #6)
> (In reply to Philipp Sackl [:phlsa] from comment #5)
> > As discussed in IRC, the icon is not needed for toolbars. We'll keep using
> > just text there to avoid having identical icons in the toolbar or wasting
> > even more space.
> > 
> > I double checked and the icons included in the attachment are all that are
> > needed for the menu panel (Linux is identical to Windows).
> 
> Ehm, actually, there's two different menuPanel sprites - one 'normal' and
> one for aero. The difference is the white highlight. See
> http://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/
> menuPanel-aero.png vs.
> http://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/
> menuPanel.png . Your zipfile only has one. :-(

(so you totally can't see this on mxr because transparency and white show up the same - if you use the devtools inspector to make the image background grey, it's pretty obvious)
Attached file menuPanel icon sets.zip (obsolete) —
You are (again) right, sorry about that!
The attached icon set should now include all platforms (including some icons for Windows 8).
Attachment #8360980 - Attachment is obsolete: true
Assignee: philipp → gijskruitbosch+bugs
And then I realized we don't have a real icon for the SDK items because they're toolbaritems. Sigh.
So I poked more at this last night, and I could get the icon to show up using just a ::before element, but it pushed down the button as a whole and just generally didn't work well. :-(

The best way to fix this would be to have a real replacement toolbarbutton in all these widgets that gets created by the SDK. The toolbaritem the SDK uses also shouldn't have the margins it does when it's in the panel or palette. Matteo, do you have time to help with this?
Flags: needinfo?(zer0)
What's the motivation for this bug? Comment 0 has zero background information.
(In reply to Dão Gottwald [:dao] from comment #11)
> What's the motivation for this bug? Comment 0 has zero background
> information.

SDK items are currently represented only by their text. The iframe provided by the add-on is hidden. You can drag the empty space where the icon would normally be, but from a UX perspective, it's a pretty bad experience.
We're talking about SDK Widget, right?
Gijs, we can try to sort it out together, I'm not sure the best way to fix it and didn't break the customization thing; without a massive change on widget code (that is a deprecated API): Could we just add a xul:image hidden in the toolbaritems and show it when we are in customization mode? Or a whole toolbarbutton inside a toolbaritem?
Flags: needinfo?(zer0)
Blocks: 962457
The icons here are now out of date because the 'email link' icon got added at the end. :-(
Here's a new icon set that includes both the new email link icon as well as the default add-on icon.
Attachment #8363185 - Attachment is obsolete: true
(In reply to Philipp Sackl [:phlsa] from comment #15)
> Here's a new icon set that includes both the new email link icon as well as
> the default add-on icon.

Looks great, I love the default add-on icon.
The problem is this takes both SDK and Fx fixes and it's not straightforward. :-(
(In reply to :Gijs Kruitbosch from comment #17)
> The problem is this takes both SDK and Fx fixes and it's not
> straightforward. :-(

Wait, in what scenario would the icon be shown? For example, the icon property for the SDK's button apis are a required property - the code will throw an exception without it.
(In reply to Jeff Griffiths (:canuckistani) from comment #18)
> (In reply to :Gijs Kruitbosch from comment #17)
> > The problem is this takes both SDK and Fx fixes and it's not
> > straightforward. :-(
> 
> Wait, in what scenario would the icon be shown? For example, the icon
> property for the SDK's button apis are a required property - the code will
> throw an exception without it.

This is about widgets. Not about the new button code.

Widgets don't have any icons at all. They currently get a label inserted but we want to add an icon.
Summary: Have a default icon for SDK addons in customization view → Have a default icon for add-on SDK widgets in customization view
Per discussion on IRC, Matteo is working on this.
Assignee: gijskruitbosch+bugs → zer0
Whiteboard: [Australis:P3] → [Australis:P3+]
Matteo, how is this going?
Flags: needinfo?(zer0)
So we talked about this yesterday and agreed we should at least land the icons already, seeing as that won't affect current consumers in any way, but makes the rest of this easier, and avoids later conflicts with other icons (e.g. for the sidebar widget)
Comment on attachment 8385817 [details] [diff] [review]
add icons to enable SDK add-ons to show default icon in Australis, rs=jaws,various-real-life-review+s

Review of attachment 8385817 [details] [diff] [review]:
-----------------------------------------------------------------

remote:   https://hg.mozilla.org/integration/fx-team/rev/619010dcdffc
Attachment #8385817 - Flags: checkin+
Keywords: leave-open
This is the SDK part. I worked on a separate CSS to test the rule for Firefox, now I have to use the proper m-c file that will comes in another patch.
Attachment #8388527 - Flags: review?(rFobic)
Flags: needinfo?(zer0)
Attachment #8388527 - Flags: review?(rFobic) → review+
Comment on attachment 8388527 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1426

Irakli, I modified the review, please take another look. I removed the label element, using just the toolbarbutton, plus if the widget's `contentURL` option is an image, it would be use that one during customization instead of the default one.

If you're not convinced by the last improvement, I can rollback and use always the default icon, and makes this change in another release.

Let me know a.s.a.p. so that we can merge and uplift.
Attachment #8388527 - Flags: review+ → review?(rFobic)
I decided to rollback the icon thingy: there are some cases we should take in consideration. Plus, widget is going to be deprecated anyway.
Attachment #8388527 - Flags: review?(rFobic) → review+
Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/aebf7533188539abd71b8bc4f9f2d1d33cdfe1e3
Bug 959640 - Have a default icon for add-on SDK widgets in customization view

- Added nested toolbarbutton to use during customization

https://github.com/mozilla/addon-sdk/commit/58db0a174cd214ab7f28bd6f891b8b85185642dc
Merge pull request #1426 from ZER0/widget-default-icon/959640

Bug 959640 - Have a default icon for add-on SDK widgets in customization view r=@gozala
Gijs, could you please double check the retina rules? I wasn't able to test them properly. Hope I split the rules in the right files, but let me know about anything. There are a tons of improvements I would love to do about styling widgets in australis, 'cause currently they seems not working well, but we're out of time and definitely those improvements are not in this bug's scope.
Attachment #8389456 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8389456 [details] [diff] [review]
CSS to style the widget during and after customization

Review of attachment 8389456 [details] [diff] [review]:
-----------------------------------------------------------------

Getting there, but I have a bunch of comments and I'd like to see the resulting patch (and be able to test it). For the latter, can you suggest an add-on I can use to actually test this? I just spent 30 minutes trying to get the SDK set up to do this (hello, bug 556562) and using lightbeam, only to find that they apparently switched to the new ui-button stuff already...

::: browser/themes/osx/browser.css
@@ +1035,4 @@
>    /* Menu panel and palette styles */
>  
>    :-moz-any(@primaryToolbarButtons@)[cui-areatype="menu-panel"],
> +  toolbarpaletteitem[place="palette"] > :-moz-any(@primaryToolbarButtons@),

Add the extra rules above the item so you don't touch blame for the existing line

@@ +1035,5 @@
>    /* Menu panel and palette styles */
>  
>    :-moz-any(@primaryToolbarButtons@)[cui-areatype="menu-panel"],
> +  toolbarpaletteitem[place="palette"] > :-moz-any(@primaryToolbarButtons@),
> +  toolbaritem[sdkstylewidget="true"][cui-areatype="menu-panel"] > toolbarbutton,

We only show the button (and consequently the icon) in the panel and palette, so you can suffice with adding just 1 selector:

toolbaritem[sdkstylewidget="true"] > toolbarbutton

@@ +1162,4 @@
>      -moz-image-region: rect(0px, 1600px, 64px, 1536px);
>    }
>  
> +  toolbaritem[sdkstylewidget="true"][cui-areatype="menu-panel"] > toolbarbutton,

ditto

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +207,5 @@
>    width: 32px;
>  }
>  
> +toolbarpaletteitem > toolbaritem[sdkstylewidget="true"][cui-areatype="menu-panel"] > toolbarbutton,
> +toolbarpaletteitem[place="palette"] > toolbaritem[sdkstylewidget="true"] > toolbarbutton {

Use one selector:

toolbarpaletteitem:-moz-any([place="palette"], [place="panel"]) > toolbaritem[sdkstylewidget="true"] > toolbarbutton

::: browser/themes/shared/menupanel.inc.css
@@ +2,5 @@
>  
>  :-moz-any(@primaryToolbarButtons@)[cui-areatype="menu-panel"],
> +toolbarpaletteitem[place="palette"] > :-moz-any(@primaryToolbarButtons@),
> +toolbaritem[sdkstylewidget="true"][cui-areatype="menu-panel"] > toolbarbutton,
> +toolbarpaletteitem[place="palette"] > toolbaritem[sdkstylewidget="true"] > toolbarbutton {

Same as for OS X, please add only one selector and add it above the existing items.

@@ +128,5 @@
>    -moz-image-region: rect(0, 800px, 32px, 768px);
>  }
>  
> +toolbaritem[sdkstylewidget="true"][cui-areatype="menu-panel"] > toolbarbutton,
> +toolbarpaletteitem[place="palette"] > toolbaritem[sdkstylewidget="true"] > toolbarbutton {

Same selector here.
Attachment #8389456 - Flags: review?(gijskruitbosch+bugs) → feedback+
Gijs, I will address your review's comment; in the meanwhile I attached a simple add-on that you can try. It basically creates a Widget with an icon 16x16 and that's all. Let me know if you need anything else!
I addressed the review's comment. I didn't use the `toolbaritem[sdkstylewidget="true"] > toolbarbutton` on first place because I was thinking the scenario where, if the widget as an icon and not a HTML document, we could show that one in the toolbar. However at the end it wasn't worthy, especially because we didn't have time to implement that properly.
Attachment #8389456 - Attachment is obsolete: true
Attachment #8389847 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8389847 [details] [diff] [review]
CSS to style the widget during and after customization

Review of attachment 8389847 [details] [diff] [review]:
-----------------------------------------------------------------

This still needs width/height: 32px rules for OS X. I've just gone and added those, fixed the nit below, and then landed this for you. Also, the icon is vertically offset in the panel by a few pixels, but I can't figure out how to fix that (obvious things like adding a few pixels margins don't seem to work), and I think we should just land this as-is:

remote:   https://hg.mozilla.org/integration/fx-team/rev/81a2f36e2fa7

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +207,4 @@
>    width: 32px;
>  }
>  
> +toolbarpaletteitem:-moz-any([place="palette"], [place="panel"]) > toolbaritem[sdkstylewidget="true"] > toolbarbutton {

The other rule about this behaviour is in browser/base, so this one should be, too.
Attachment #8389847 - Flags: review?(gijskruitbosch+bugs) → review+
Keywords: leave-open
Whiteboard: [Australis:P3+] → [Australis:P3+][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/81a2f36e2fa7
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3+][fixed-in-fx-team] → [Australis:P3+]
Target Milestone: --- → Firefox 30
Comment on attachment 8385817 [details] [diff] [review]
add icons to enable SDK add-ons to show default icon in Australis, rs=jaws,various-real-life-review+s

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: no icons for SDK widgets
Testing completed (on m-c, etc.): on m-c, locally
Risk to taking this patch (and alternatives if risky): low, SDK-specific, can't get much worse than it is
String or IDL/UUID changes made by this patch: none
Attachment #8385817 - Flags: approval-mozilla-aurora?
Comment on attachment 8389847 [details] [diff] [review]
CSS to style the widget during and after customization

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis + SDK widgets
User impact if declined: SDK widgets don't have icons in customize mode's panel/palette
Testing completed (on m-c, etc.): on m-c, locally
Risk to taking this patch (and alternatives if risky): low, SDK-specific, CSS only changes
String or IDL/UUID changes made by this patch: none
Attachment #8389847 - Flags: approval-mozilla-aurora?
Attachment #8385817 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8389847 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
QA Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.