Closed Bug 1315255 Opened 8 years ago Closed 8 years ago

Add ability for themes to change toolbar button icons

Categories

(WebExtensions :: General, enhancement, P3)

Product:
WebExtensions
Component:
General
Type:
enhancement

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: jaws, Assigned: jaws)

References

Details

(Whiteboard: triaged)

Attachments

(1 file)

Bug 1315255 - Add ability for themes to change toolbar icons.
58 bytes, text/x-review-board-request
mikedeboer
: review+
Details 
Webextension themes defined using the API covered by bug 1306671 should be able to change the icons of various toolbarbuttons through the manifest.

"theme": {
  "icons": {
    "back_button": <local-uri>,
    "forward_button": <local-uri>,
    "reload_button": <local-uri>,
    "stop_button": <local-uri>,
    "bookmark_star_button": <local-uri>,
    "bookmark_menu_button": <local-uri>,
    "downloads_button": <local-uri>,
    "home_button": <local-uri>,
    "app_menu_button": <local-uri>,
    "cut_button": <local-uri>,
    "copy_button": <local-uri>,
    "paste_button": <local-uri>,
    "new_window_button": <local-uri>,
    "new_private_window_button": <local-uri>,
    "save_page_button": <local-uri>,
    "print_button": <local-uri>,
    "history_button": <local-uri>,
    "full_screen_button": <local-uri>,
    "find_button": <local-uri>,
    "options_button": <local-uri>,
    "addons_button": <local-uri>,
    "developer_button": <local-uri>,
    "synced_tabs_button": <local-uri>,
    "open_file_button": <local-uri>,
    "sidebars_button": <local-uri>,
    "share_page_button": <local-uri>,
    "subscribe_button": <local-uri>,
    "text_encoding_button": <local-uri>,
    "email_link_button": <local-uri>,
    "forget_button": <local-uri>,
    "pocket_button": <local-uri>,
  }
}
Checkpointing work here.

Current status:
All of the toolbar icons except the bookmarks-menu-dropdown are working
Will need to figure out a solution for different DPI, right now this assumes HiDPI (see -moz-image-region styling)
Should we allow custom icons in the menu panel? If so, we'll need to let devs specify different sizes, or require SVG
The test should probably use custom icons instead of referencing the sprite sheet that is already shipping with the browser.
Priority: -- → P3
Whiteboard: triaged
Attachment #8807710 - Flags: review?(mdeboer)
Comment on attachment 8807710 [details]
Bug 1315255 - Add ability for themes to change toolbar icons.

I need to make some changes here first.
Attachment #8807710 - Flags: review?(mdeboer)
Comment on attachment 8807710 [details]
Bug 1315255 - Add ability for themes to change toolbar icons.

https://reviewboard.mozilla.org/r/90784/#review93946

I like it, but please take a look at my comments - I don't think we'll want to break Chrome compat, even though these are 'just' demos.

::: browser/components/extensions/ext-theme.js:726
(Diff revision 3)
>      }
>    }
>  
>    render({asUpdate} = {asUpdate: false}) {
>      let browserStyles = [`
> -      @-moz-document url("about:home"),
> +      @-moz-document url("about:home"),\n

I don't think \n is necessary in `-denoted literal strings.

::: browser/components/extensions/schemas/theme.json:191
(Diff revision 3)
>                  "items": {
>                    "type": "number"
>                  },
>                  "optional": true
>                },
> -              "toolbar_button_stroke": {
> +              "toolbar_stroke": {

I used this name for Chrome compat.

::: browser/components/extensions/schemas/theme.json:198
(Diff revision 3)
>                  "items": {
>                    "type": "number"
>                  },
>                  "optional": true
>                },
> -              "toolbar_button_stroke_inactive": {
> +              "toolbar_stroke_inactive": {

I used this name for Chrome compat.

::: browser/components/extensions/schemas/theme.json:239
(Diff revision 3)
>            },
>            "gradients": {
>              "type": "object",
>              "optional": true,
>              "properties": {
> -              "toolbar_button": {
> +              "toolbar": {

I used this name for Chrome compat.

::: browser/components/extensions/schemas/theme.json:243
(Diff revision 3)
>              "properties": {
> -              "toolbar_button": {
> +              "toolbar": {
>                  "type": "string",
>                  "optional": true
>                },
> -              "toolbar_button_pressed": {
> +              "toolbar_pressed": {

I used this name for Chrome compat.

::: browser/components/extensions/test/browser/browser.ini:105
(Diff revision 3)
>  [browser_ext_theme_lwtsupport.js]
> +[browser_ext_theme_icons.js]
> +support-files =
> +  burrito.svg
> +  hamburger.svg
> +  pile-of-poop.svg

*chuckle* (I'm a kid, still)
Attachment #8807710 - Flags: review?(mdeboer) → review+
Hm, I think most of those changes are just from rebasing and mozreview not handling it well.
Comment on attachment 8807710 [details]
Bug 1315255 - Add ability for themes to change toolbar icons.

https://reviewboard.mozilla.org/r/90784/#review94170

Please see my previous review comments and please remove the separate .svg files.
(In reply to Mike de Boer [:mikedeboer] from comment #6)
> > +      @-moz-document url("about:home"),\n
> 
> I don't think \n is necessary in `-denoted literal strings.

I needed to add these to get the console to include the line breaks. We could remove them but it didn't hurt to have them here.
 
> > -              "toolbar_button_stroke": {
> > +              "toolbar_stroke": {
> 
> I used this name for Chrome compat.

I must have screwed up rebasing. This is fixed now.

> (Diff revision 3)
> >  [browser_ext_theme_lwtsupport.js]
> > +[browser_ext_theme_icons.js]
> > +support-files =
> > +  burrito.svg
> > +  hamburger.svg
> > +  pile-of-poop.svg
> 
> *chuckle* (I'm a kid, still)

Oh good! Me too!

https://hg.mozilla.org/projects/cedar/rev/4434ae7f99e197043e4192cf6612503e41da8b39
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Depends on: 1319841
Depends on: 1319870
Depends on: 1320077
Product: Toolkit → WebExtensions
Depends on: 1548769
Type: defect → enhancement
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: