Implement browser_style for the sidebar

NEW
Assigned to

Status

()

Toolkit
WebExtensions: Frontend
P2
normal
4 months ago
4 days ago

People

(Reporter: andym, Assigned: mattw)

Tracking

(Blocks: 1 bug)

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

4 months ago
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.
(Reporter)

Updated

4 months ago
Assignee: nobody → mixedpuppy
webextensions: --- → +
Priority: -- → P2
Whiteboard: triaged
Depends on: 1208596
(Reporter)

Updated

4 months ago
Assignee: mixedpuppy → mwein
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 4

6 days ago
mozreview-review
Comment on attachment 8868374 [details]
Bug 1330369 - 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 5

6 days ago
mozreview-review
Comment on attachment 8868375 [details]
Bug 1330369 - 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.
(Assignee)

Comment 7

4 days ago
(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.
(Assignee)

Comment 8

4 days ago
mozreview-review-reply
Comment on attachment 8868374 [details]
Bug 1330369 - 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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
You need to log in before you can comment on or make changes to this bug.