Closed Bug 1330369 Opened 7 years ago Closed 7 years ago

Implement browser_style for the sidebar

Categories

(WebExtensions :: Frontend, defect, P2)

Product:
WebExtensions
Component:
Frontend
Type:
defect

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed
Blocking Flags:
webextensions +

People

(Reporter: andy+bugzilla, Assigned: mattw)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: triaged)

Attachments

(3 files)

Bug 1330369 - Part 2 - Add browser_style support to ext-sidebar.js
59 bytes, text/x-review-board-request
kmag
: review+
Details
Bug 1330369 - Part 3 - Add unit tests for sidebar.browser_style
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
Bug 1330369 - Part 1 - Share the extension stylesheets between sidebar, popups, and options
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details 
Like browser_action and options_ui we should have a browser_style option for the sidebar that implements some CSS to pull in standard styles. I'm not actually sure what that means or what those look like so I'll just point over to someone like bwinton who is way more knowledgeable on this sort of thing.
Since the sidebar is a new API and we don't have to worry about breaking existing add-ons, we should probably make browser_style default to true for sidebars.
Assignee: nobody → mixedpuppy
webextensions: --- → +
Priority: -- → P2
Whiteboard: triaged
Depends on: 1208596
Assignee: mixedpuppy → mwein
Comment on attachment 8868374 [details]
Bug 1330369 - Part 2 - Add browser_style support to ext-sidebar.js

https://reviewboard.mozilla.org/r/139956/#review143568

::: browser/base/content/webext-panels.js:20
(Diff revision 1)
>    promiseEvent,
>  } = ExtensionUtils;
>  
>  const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
>  
> +XPCOMUtils.defineLazyGetter(this, "browserStyleCSS", () => {

Lets export this from ExtensionPopups.jsm and import it here.

Side question, what is the difference between popupStylesheets and standaloneStylesheets in ExtensionPopups.jsm, which is appropriate for a non-popup?

::: browser/base/content/webext-panels.js:73
(Diff revision 1)
> +      browser.messageManager.loadFrameScript(
> +        "chrome://extensions/content/ext-browser-content.js", false);
> +
> +      browser.messageManager.sendAsyncMessage("Extension:InitBrowser", {
> +        stylesheets: browserStyleCSS,
> +        isSidebar: true,

I think we should call this something else, or possibly it is not necessary (see my notes later).  It's conceivable that we have more UI in the future.  So far we've added isInline, now isSidebar, I'd rather have flags for functionality a panel needs than naming a specific panel.

::: browser/components/extensions/ext-sidebarAction.js:129
(Diff revision 1)
>        }
>      }
>    }
>  
>    sidebarUrl(panel) {
> +    let url = `${sidebarURL}?panel=${encodeURIComponent(panel)}`;

File a follup bug to change how we handle the url here, I'd like to avoid adding more args in the future.

::: toolkit/components/extensions/ext-browser-content.js:62
(Diff revision 1)
>      }
>  
>      addEventListener("DOMWindowCreated", this, true);
> +
> +    // Sidebars only need to listen to DOMWindowCreated.
> +    if (this.isSidebar) {

back to isSidebar, I think it's unecessary.

!!blockParser should add the event listener for that (DOMDocElementInserted).  

If any combination of fixedWidth, maxWidth, maxHeight are provided we can assume the panel needs resizing.  

needsResize = fixedWidth || maxWidth || maxHeight;

DOMContentLoaded and MozScrolledAreaChanged are only for resizing, we can break out of load early (prior to handleDOMChange) if resizing is not necessary.

We want to get the Extension:BrowserContentLoaded message, so we need to keep load and DOMContentLoaded, but I think we should make sure we're not doing double work if we receive both.
Attachment #8868374 - Flags: review?(mixedpuppy)
Comment on attachment 8868375 [details]
Bug 1330369 - Part 3 - Add unit tests for sidebar.browser_style

https://reviewboard.mozilla.org/r/139958/#review143572
Attachment #8868375 - Flags: review?(mixedpuppy) → review+
(In reply to Shane Caraveo (:mixedpuppy) from comment #4)

> If any combination of fixedWidth, maxWidth, maxHeight are provided we can
> assume the panel needs resizing.  
> 
> needsResize = fixedWidth || maxWidth || maxHeight;

We'll need to verify that...resize may be needed if none of these are provided.  In that case, I might suggest a resize object that can include those values (or not).  If the resize object itself is undefined, no resizing logic is started.
(In reply to Shane Caraveo (:mixedpuppy) from comment #6)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #4)
> 
> > If any combination of fixedWidth, maxWidth, maxHeight are provided we can
> > assume the panel needs resizing.  
> > 
> > needsResize = fixedWidth || maxWidth || maxHeight;
> 
> We'll need to verify that...resize may be needed if none of these are
> provided.  In that case, I might suggest a resize object that can include
> those values (or not).  If the resize object itself is undefined, no
> resizing logic is started.

I think this is fine. Looking at _handleDOMChange, the first half only runs if fixedWidth is set to true, and the other half relies on maxWidth and maxHeight for contentViewer.getContentSizeConstrained.
Comment on attachment 8868374 [details]
Bug 1330369 - Part 2 - Add browser_style support to ext-sidebar.js

https://reviewboard.mozilla.org/r/139956/#review143568

> Lets export this from ExtensionPopups.jsm and import it here.
> 
> Side question, what is the difference between popupStylesheets and standaloneStylesheets in ExtensionPopups.jsm, which is appropriate for a non-popup?

Good idea. These styles are also used for the options_ui style - http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.js#102 - so I think it would be a good idea to go one step further and have it exported there as well. Since that code doesn't import ExtensionPopups.jsm, but it already imports ExtensionParent.jsm, I think it might be good to export it there.

All that standaloneStylesheets does is add a border-radius to the popup's body element, which I don't think we want for the sidebar.

> I think we should call this something else, or possibly it is not necessary (see my notes later).  It's conceivable that we have more UI in the future.  So far we've added isInline, now isSidebar, I'd rather have flags for functionality a panel needs than naming a specific panel.

Yeah, I think that would be much better.

> File a follup bug to change how we handle the url here, I'd like to avoid adding more args in the future.

Will do.
Comment on attachment 8869617 [details]
Bug 1330369 - Part 1 - Share the extension stylesheets between sidebar, popups, and options

https://reviewboard.mozilla.org/r/141184/#review146140

::: toolkit/components/extensions/ExtensionParent.jsm:1098
(Diff revision 1)
>      }
>  
>      gBaseManifestProperties = Object.getOwnPropertyNames(manifest.properties);
>      return gBaseManifestProperties;
>    },
> +  get extensionStyleSheets() {

I know we talked about putting this here before, but how about we put this into ExensionUtils next to the existing stylesheetMap.
Comment on attachment 8868374 [details]
Bug 1330369 - Part 2 - Add browser_style support to ext-sidebar.js

https://reviewboard.mozilla.org/r/139956/#review146148

::: toolkit/components/extensions/ext-browser-content.js:79
(Diff revision 2)
> +    this.blockParser = blockParser;
> +    this.needsResize = fixedWidth || maxHeight || maxWidth;
> +
>      this.oldBackground = null;
>  
> +    this.browserContentLoaded = false;

I'm not certain we need this, and I'm wondering if it would be problematic.  e.g. a page reloads itself or changes to another page (location = "./someOtherPage.html").  Is there a problem you would anticipate if it were removed?
Attachment #8868374 - Flags: review?(mixedpuppy)
Comment on attachment 8868374 [details]
Bug 1330369 - Part 2 - Add browser_style support to ext-sidebar.js

https://reviewboard.mozilla.org/r/139956/#review147166

::: browser/base/content/webext-panels.js:75
(Diff revision 3)
>  function loadWebPanel() {
>    let sidebarURI = new URL(location);
>    let sidebar = {
>      uri: sidebarURI.searchParams.get("panel"),
>      remote: sidebarURI.searchParams.get("remote"),
> +    style: sidebarURI.searchParams.get("style"),

Please call these `browserStyle` and `browser-style`
Attachment #8868374 - Flags: review?(kmaglione+bmo) → review+
Attachment #8868374 - Flags: review?(mixedpuppy)
Comment on attachment 8869617 [details]
Bug 1330369 - Part 1 - Share the extension stylesheets between sidebar, popups, and options

https://reviewboard.mozilla.org/r/141184/#review147194
Attachment #8869617 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8868374 [details]
Bug 1330369 - Part 2 - Add browser_style support to ext-sidebar.js

https://reviewboard.mozilla.org/r/139956/#review147166

Thanks!

> Please call these `browserStyle` and `browser-style`

Done.
Comment on attachment 8868374 [details]
Bug 1330369 - Part 2 - Add browser_style support to ext-sidebar.js

https://reviewboard.mozilla.org/r/139956/#review143568

> Will do.

Filed bug 1368220.
Pushed by mwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/163ef155b600
Part 1 - Share the extension stylesheets between sidebar, popups, and options r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/a766850786e7
Part 2 - Add browser_style support to ext-sidebar.js r=kmag
https://hg.mozilla.org/integration/autoland/rev/8092c26b85d4
Part 3 - Add unit tests for sidebar.browser_style r=mixedpuppy
Attachment #8868374 - Flags: review?(mixedpuppy)
Keywords: dev-doc-needed
I've updated the docs for this:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/sidebar_action

Please let me know if it looks OK.
Flags: needinfo?(mwein)
This looks great, thanks.
Flags: needinfo?(mwein)
Depends on: 1383910
Product: Toolkit → WebExtensions
Depends on: 1488055
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: