Implement browser_style for the sidebar

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
WebExtensions: Frontend
P2
normal
RESOLVED FIXED
5 months ago
11 days ago

People

(Reporter: andym, Assigned: mattw)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

unspecified
mozilla55
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: triaged)

MozReview Requests

()

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

Attachments

(3 attachments)

(Reporter)

Description

5 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.

Comment 1

5 months ago
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

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

Updated

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

Comment 4

a month ago
mozreview-review
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 5

a month ago
mozreview-review
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.
(Assignee)

Comment 7

a month 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

a month ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 12

a month ago
mozreview-review
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 13

a month ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 17

28 days ago
mozreview-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

::: 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 18

28 days ago
mozreview-review
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+
(Assignee)

Comment 19

28 days ago
mozreview-review-reply
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.
(Assignee)

Comment 20

28 days ago
mozreview-review-reply
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 24

28 days ago
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

Comment 25

27 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/163ef155b600
https://hg.mozilla.org/mozilla-central/rev/a766850786e7
https://hg.mozilla.org/mozilla-central/rev/8092c26b85d4
Status: NEW → RESOLVED
Last Resolved: 27 days ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Attachment #8868374 - Flags: review?(mixedpuppy)
(Reporter)

Updated

11 days ago
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.