Closed
Bug 1330369
Opened 8 years ago
Closed 7 years ago
Implement browser_style for the sidebar
Categories
(WebExtensions :: Frontend, defect, P2)
WebExtensions
Frontend
Tracking
(firefox55 fixed)
People
(Reporter: andy+bugzilla, Assigned: mattw)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete, Whiteboard: triaged)
Attachments
(3 files)
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•8 years 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•8 years ago
|
Assignee: nobody → mixedpuppy
webextensions: --- → +
Priority: -- → P2
Whiteboard: triaged
Reporter | ||
Updated•8 years ago
|
Assignee: mixedpuppy → mwein
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 4•8 years 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•8 years 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+
Comment 6•8 years ago
|
||
(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•8 years 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•8 years 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•8 years 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•8 years 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•7 years 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+
Updated•7 years ago
|
Attachment #8868374 -
Flags: review?(mixedpuppy)
Comment 18•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•7 years ago
|
Attachment #8868374 -
Flags: review?(mixedpuppy)
Reporter | ||
Updated•7 years ago
|
Keywords: dev-doc-needed
Comment 26•7 years ago
|
||
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)
Updated•7 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•