Closed
Bug 1417880
Opened 6 years ago
Closed 6 years ago
Allow theming arrow panels
Categories
(WebExtensions :: Themes, enhancement, P5)
WebExtensions
Themes
Tracking
(firefox60 fixed)
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: ntim, Assigned: masinico, Mentored)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete)
Attachments
(2 files)
Here are the properties that can be supported: colors.arrowpanel colors.arrowpanel_text colors.arrowpanel_border
Reporter | ||
Updated•6 years ago
|
Updated•6 years ago
|
Priority: -- → P5
Updated•6 years ago
|
Assignee: nobody → jaws
Updated•6 years ago
|
Depends on: dark-theme-darkening
Updated•6 years ago
|
Blocks: dark-theme-darkening
No longer depends on: dark-theme-darkening
Updated•6 years ago
|
Assignee: jaws → masinico
Mentor: mconley, jaws, ntim.bugs
Reporter | ||
Updated•6 years ago
|
Keywords: dev-doc-needed
Updated•6 years ago
|
Component: WebExtensions: Frontend → WebExtensions: Themes
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 4•6 years ago
|
||
mozreview-review |
Comment on attachment 8948605 [details] Bug 1417880: Allow theming arrow panels(popups). https://reviewboard.mozilla.org/r/218022/#review223838 ::: toolkit/components/extensions/ext-theme.js:160 (Diff revision 3) > + case "arrowpanel": > + this.lwtStyles.arrowpanel = cssColor; > + break; > + case "arrowpanel_text": > + this.lwtStyles.arrowpanel_text = cssColor; > + break; > + case "arrowpanel_border": > + this.lwtStyles.arrowpanel_border = cssColor; > + break; The 3 cases are already covered by the 3 other lines you added before. ::: toolkit/components/extensions/test/browser/browser.ini:21 (Diff revision 3) > [browser_ext_themes_static_onUpdated.js] > [browser_ext_themes_tab_text.js] > [browser_ext_themes_toolbar_fields.js] > [browser_ext_themes_toolbars.js] > [browser_ext_themes_toolbarbutton_icons.js] > +[browser_ext_themes_arrowpanels.js] You forgot to add the test file. (hg add path/to/file )
Attachment #8948605 -
Flags: review?(ntim.bugs)
Comment hidden (mozreview-request) |
Comment 6•6 years ago
|
||
Hi Connor, thanks for working on this! It'd be very cool if you attached a screenshot of your patch in action to this bug, so that people who are watching this can get a feel for what it looks like. Thanks!
Reporter | ||
Comment 7•6 years ago
|
||
mozreview-review |
Comment on attachment 8948605 [details] Bug 1417880: Allow theming arrow panels(popups). https://reviewboard.mozilla.org/r/218022/#review224140 ::: toolkit/components/extensions/test/browser/browser_ext_themes_arrowpanels.js:4 (Diff revision 4) > +"use strict"; > + > + > +ignoreAllUncaughtExceptions(); Why is this here ? ::: toolkit/components/extensions/test/browser/browser_ext_themes_arrowpanels.js:14 (Diff revision 4) > +add_task(async function test_arrowpanel_styling(browser, accDoc) { > + const ARROWPANEL_BACKGROUND_COLOR = "#FF0000"; > + const ARROWPANEL_TEXT_COLOR = "#008000"; > + const ARROWPANEL_BORDER_COLOR = "#0000FF"; > + > + //Create an extension with the added properties add a space after // here and in the rest of the test ::: toolkit/components/extensions/test/browser/browser_ext_themes_arrowpanels.js:66 (Diff revision 4) > + Assert.equal( > + window.getComputedStyle(arrowpanel).getPropertyValue("--arrowpanel-border-color"), > + ARROWPANEL_BORDER_COLOR, > + "Arrowpanel background color" > + ); I wrote this function to test border colors: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields.js#13-26 You can move it into head.js, then use it in your test.
Comment hidden (mozreview-request) |
Reporter | ||
Comment 9•6 years ago
|
||
mozreview-review |
Comment on attachment 8948605 [details] Bug 1417880: Allow theming arrow panels(popups). https://reviewboard.mozilla.org/r/218022/#review224454 ::: toolkit/components/extensions/test/browser/head.js:57 (Diff revision 5) > function imageBufferFromDataURI(encodedImageData) { > let decodedImageData = atob(encodedImageData); > return Uint8Array.from(decodedImageData, byte => byte.charCodeAt(0)).buffer; > } > + > +function testBorderColor(element, expected) { Now that it's in head.js, can you remove it from browser_ext_themes_toolbar_fields.js ? ::: toolkit/components/extensions/test/browser/head.js:60 (Diff revision 5) > + "Field left border color should be set."); > + Assert.equal(window.getComputedStyle(element).borderRightColor, > + hexToCSS(expected), > + "Field right border color should be set."); > + Assert.equal(window.getComputedStyle(element).borderTopColor, > + hexToCSS(expected), > + "Field top border color should be set."); > + Assert.equal(window.getComputedStyle(element).borderBottomColor, > + hexToCSS(expected), > + "Field bottom border color should be set."); Should be "Element" instead of "Field"
Comment 10•6 years ago
|
||
mozreview-review |
Comment on attachment 8948605 [details] Bug 1417880: Allow theming arrow panels(popups). https://reviewboard.mozilla.org/r/218022/#review224466 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: toolkit/components/extensions/test/browser/head.js:57 (Diff revision 5) > function imageBufferFromDataURI(encodedImageData) { > let decodedImageData = atob(encodedImageData); > return Uint8Array.from(decodedImageData, byte => byte.charCodeAt(0)).buffer; > } > + > +function testBorderColor(element, expected) { Error: 'testbordercolor' is defined but never used. allowed unused vars must match /^(cc|ci|cr|cu|exported_symbols)$/. [eslint: no-unused-vars]
Reporter | ||
Comment 11•6 years ago
|
||
(In reply to Code Review Bot [:reviewbot] from comment #10) > > ::: toolkit/components/extensions/test/browser/head.js:57 > (Diff revision 5) > > function imageBufferFromDataURI(encodedImageData) { > > let decodedImageData = atob(encodedImageData); > > return Uint8Array.from(decodedImageData, byte => byte.charCodeAt(0)).buffer; > > } > > + > > +function testBorderColor(element, expected) { > > Error: 'testbordercolor' is defined but never used. allowed unused vars must > match /^(cc|ci|cr|cu|exported_symbols)$/. [eslint: no-unused-vars] Ah yes, you need to add "testBorderColor" on top of the head.js file in the /* exported ... */ comment
Comment 12•6 years ago
|
||
mozreview-review |
Comment on attachment 8948605 [details] Bug 1417880: Allow theming arrow panels(popups). https://reviewboard.mozilla.org/r/218022/#review224514 Looks good, Connor! I have a few suggestions below. Thanks for the patch! ::: toolkit/components/extensions/test/browser/browser_ext_themes_arrowpanels.js:33 (Diff revision 5) > + await BrowserTestUtils.withNewTab({ gBrowser, url: "about:home" }, async function(browser) { > + await extension.startup(); > + > + // Load a webpage to access the information arrow panel > + let promise = BrowserTestUtils.browserLoaded(browser); > + browser.loadURI("https://example.com"); > + await promise; If you're sending the tab to example.com anyways, might as well just load that instead of about:home on line 33. Then you can get rid of lines 36-39. ::: toolkit/components/extensions/test/browser/browser_ext_themes_arrowpanels.js:42 (Diff revision 5) > + let identityBox = document.querySelector("#identity-box"); > + identityBox.setAttribute("open", "true"); > + identityBox.click(); Usually, you have to wait for the popupshown event to fire to be sure that the panel is actually open. There are also some variables stashed around that you can use instead of re-querying for ID's. Here's an example: https://searchfox.org/mozilla-central/rev/a539f8f7fe288d0b4d8d6a32510be20f52bb7a0f/browser/base/content/test/permissions/browser_permissions.js#9-19 I've also included a function that'll close the popup, which you'll need to do at the end up the test. ::: toolkit/components/extensions/test/browser/browser_ext_themes_arrowpanels.js:46 (Diff revision 5) > + // Open the information arrow panel by "clicking" the info icon > + let identityBox = document.querySelector("#identity-box"); > + identityBox.setAttribute("open", "true"); > + identityBox.click(); > + > + let arrowpanel = document.getElementsByClassName("panel-arrowcontent")[0]; Instead of querying like this and relying on DOM structure, I think you can use: ```js let arrowContent = document.getAnonymousElementByAttribute(gIdentityHandler._identityPopup, "class", "panel-arrowcontent"); ``` which should be less brittle. ::: toolkit/components/extensions/test/browser/browser_ext_themes_arrowpanels.js:50 (Diff revision 5) > + > + let arrowpanel = document.getElementsByClassName("panel-arrowcontent")[0]; > + > + // Ensure arrowpanel background color was set properly > + Assert.equal( > + window.getComputedStyle(arrowpanel).getPropertyValue("background-color"), You can probably do this getComputedStyle once before these assertions, and check the return value instead (see a similar suggestion I made for `testBorderColor()` if you're not sure what I mean) ::: toolkit/components/extensions/test/browser/browser_ext_themes_arrowpanels.js:52 (Diff revision 5) > + > + // Ensure arrowpanel background color was set properly > + Assert.equal( > + window.getComputedStyle(arrowpanel).getPropertyValue("background-color"), > + `rgb(${hexToRGB(ARROWPANEL_BACKGROUND_COLOR).join(", ")})`, > + "Arrowpanel background color" "Arrow panel background color should have been themed" ::: toolkit/components/extensions/test/browser/browser_ext_themes_arrowpanels.js:59 (Diff revision 5) > + > + // Ensure arrowpanel text color was set properly > + Assert.equal( > + window.getComputedStyle(arrowpanel).getPropertyValue("color"), > + `rgb(${hexToRGB(ARROWPANEL_TEXT_COLOR).join(", ")})`, > + "Arrowpanel text color" "Arrow panel text color should have been themed" ::: toolkit/components/extensions/test/browser/browser_ext_themes_arrowpanels.js:64 (Diff revision 5) > + "Arrowpanel text color" > + ); > + > + // Ensure arrowpanel border color was set properly > + testBorderColor(arrowpanel, ARROWPANEL_BORDER_COLOR) > + Here's where we need to close the panel, and wait for it to fire popuphidden. ::: toolkit/components/extensions/test/browser/head.js:58 (Diff revision 5) > + Assert.equal(window.getComputedStyle(element).borderLeftColor, > + hexToCSS(expected), > + "Field left border color should be set."); > + Assert.equal(window.getComputedStyle(element).borderRightColor, > + hexToCSS(expected), > + "Field right border color should be set."); > + Assert.equal(window.getComputedStyle(element).borderTopColor, > + hexToCSS(expected), > + "Field top border color should be set."); > + Assert.equal(window.getComputedStyle(element).borderBottomColor, While we're moving this, I think we can improve it: 1. We should be able to run getComputedStyle once and use the return value repeatedly 2. "Field X border color should be set" is kind of a misnomer now because we're using it for a panel too. Perhaps let's call it an element instead. What I'm suggesting is something like this: ```js let styles = window.getComputedStyles(element); Assert.equal(styles.borderLeftColor, hextToCSS(expected), "Left border color should be set on element"); Assert.equal(styles.borderRightColor, hextToCSS(expected), "Right border color should be set on element");); Assert.equal(styles.borderTopColor, hextToCSS(expected), "Top border color should be set on element"); Assert.equal(styles.borderBottomColor, hextToCSS(expected), "Bottom border color should be set on element"); ```
Attachment #8948605 -
Flags: review?(mconley) → review-
Comment 13•6 years ago
|
||
mozreview-review |
Comment on attachment 8948605 [details] Bug 1417880: Allow theming arrow panels(popups). https://reviewboard.mozilla.org/r/218022/#review224524 mconley's review here is sufficient.
Attachment #8948605 -
Flags: review?(jaws)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 16•6 years ago
|
||
Reporter | ||
Comment 17•6 years ago
|
||
mozreview-review |
Comment on attachment 8948605 [details] Bug 1417880: Allow theming arrow panels(popups). https://reviewboard.mozilla.org/r/218022/#review224726 Looks close from landing! ::: toolkit/components/extensions/test/browser/browser_ext_themes_arrowpanels.js:23 (Diff revision 7) > +add_task(async function test_arrowpanel_styling(browser, accDoc) { > + const ARROWPANEL_BACKGROUND_COLOR = "#FF0000"; > + const ARROWPANEL_TEXT_COLOR = "#008000"; > + const ARROWPANEL_BORDER_COLOR = "#0000FF"; > + > + // Create an extension with the added properties nit: you probably don't need this comment. ::: toolkit/components/extensions/test/browser/browser_ext_themes_arrowpanels.js:49 (Diff revision 7) > + > + await BrowserTestUtils.withNewTab({ gBrowser, url: "https://example.com" }, async function(browser) { > + await extension.startup(); > + > + // Open the information arrow panel > + openIdentityPopup(); Should be `await openIdentityPopup();` since you want to wait for the promise to resolve. ::: toolkit/components/extensions/test/browser/browser_ext_themes_arrowpanels.js:70 (Diff revision 7) > + ); > + > + // Ensure arrowpanel border color was set properly > + testBorderColor(arrowContent, ARROWPANEL_BORDER_COLOR) > + > + closeIdentityPopup(); Same here.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 20•6 years ago
|
||
I suspect "arrowpanel" is too specific. Themes will probably want to style popups consistently, so we should pick a more generic name such as "menu" or "popup" or "panel", even though we may support it only for arrow panels initially. Bug 1417883 would be the first candidate to use the same variables in another context.
Reporter | ||
Comment 21•6 years ago
|
||
(In reply to Dão Gottwald [::dao] from comment #20) > I suspect "arrowpanel" is too specific. Themes will probably want to style > popups consistently, so we should pick a more generic name such as "menu" or > "popup" or "panel", even though we may support it only for arrow panels > initially. Bug 1417883 would be the first candidate to use the same > variables in another context. I agree "arrowpanel" is probably a bad naming choice, since the fact that we use arrows is just an UX decision, and it might change in the future. However, I don't think we should use the same property for both the autocomplete dropdowns and the arrow panels. Those two differ UX-wise in Firefox, so themes might also want to make them differ too, we should give them the flexibility of doing so. Also, it doesn't add much maintenance cost to have separate properties for those two, since the variables already exist internally.
Comment 22•6 years ago
|
||
I agree with Tim's comment 21.
Comment 23•6 years ago
|
||
Arrow panels don't have a consistent UX to begin with. Some look like menus, others have entirely random content, so I don't see how the address bar and search bar popups wouldn't fit in there. Maintainability isn't the concern here. The concern is bad API design, i.e. providing APIs that are more confusing than useful. If we can make hacking together an internally consistent theme simpler at the cost of not supporting fringe use cases, that's a win in my book.
Reporter | ||
Comment 24•6 years ago
|
||
(In reply to Dão Gottwald [::dao] from comment #23) > Arrow panels don't have a consistent UX to begin with. Some look like menus, > others have entirely random content, so I don't see how the address bar and > search bar popups wouldn't fit in there. How you interact with autocomplete popups is entirely different, autocomplete popups are keyboard oriented, while arrow panels are mouse oriented. This also has consequences on how they look: autocomplete popups have a special selected state (blue), while arrow panels essentially don't. If anything, unifying those two conceptually different APIs seems confusing to me.
Comment 25•6 years ago
|
||
Keyboard access for arrow panels is planned as part of bug 1418973. The selected state can be supported as something like panel_highlight_background and panel_highlight_text, which doesn't imply that every panel must use these colors.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 30•6 years ago
|
||
mozreview-review |
Comment on attachment 8948605 [details] Bug 1417880: Allow theming arrow panels(popups). https://reviewboard.mozilla.org/r/218022/#review225056 r+ with the one change below ::: browser/themes/linux/customizableui/panelUI.css (Diff revision 13) > * Linux works around this issue. This bug is on file as 1394575. > */ > #pageActionFeedback > .panel-arrowcontainer > .panel-arrowcontent { > box-shadow: none; > } > - Please undo this change.
Attachment #8948605 -
Flags: review?(jaws) → review+
Comment hidden (mozreview-request) |
Updated•6 years ago
|
Attachment #8948605 -
Flags: review?(mconley)
Comment 32•6 years ago
|
||
Looks like jaws beat me to this one.
Comment 33•6 years ago
|
||
Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5e81474b90dc Allow theming arrow panels(popups). r=jaws
Comment 34•6 years ago
|
||
Backed out for ESlint failure on /browser_ext_themes_arrowpanels.js Push: https://hg.mozilla.org/integration/autoland/rev/5e81474b90dcb5bde84d309e2b630b3e1f388778 Backout: https://hg.mozilla.org/integration/autoland/rev/6bdf613e5305adcb36d294446c6d5e1cbfcad587 Log: https://treeherder.mozilla.org/logviewer.html#?job_id=161593400&repo=autoland&lineNumber=235
Flags: needinfo?(masinico)
Comment hidden (mozreview-request) |
Updated•6 years ago
|
Status: NEW → ASSIGNED
Reporter | ||
Comment 37•6 years ago
|
||
mozreview-review |
Comment on attachment 8948605 [details] Bug 1417880: Allow theming arrow panels(popups). https://reviewboard.mozilla.org/r/218022/#review225042 ::: browser/themes/shared/customizableui/panelUI.inc.css:959 (Diff revision 15) > min-height: unset; > + border: 1px solid #e1e1e1; > + border-radius: 10000px; > + padding: 1px 8px; > } > Thanks for combining the rules. Can you also change the border color to be var(--panel-separator-color) ?
Comment hidden (mozreview-request) |
Reporter | ||
Comment 39•6 years ago
|
||
mozreview-review |
Comment on attachment 8948605 [details] Bug 1417880: Allow theming arrow panels(popups). https://reviewboard.mozilla.org/r/218022/#review225076 ::: browser/themes/shared/customizableui/panelUI.inc.css:960 (Diff revisions 15 - 16) > border: 1px solid #e1e1e1; > border-radius: 10000px; > padding: 1px 8px; > + background-color: var(--arrowpanel-background); > +} > + > +#appMenu-zoomReset-button@buttonStateHover@ { > + background-color: var(--arrowpanel-dimmed); > +} > + > +#appMenu-zoomReset-button@buttonStateActive@ { > + background-color: var(--arrowpanel-dimmed-further); > } You probably don't want to change the styling too much so: border: 1px solid var(--panel-separator-color); background-color: var(--arrowpanel-dimmed); For hover: background-color: var(--arrowpanel-dimmed-further); For active: background-color: var(--arrowpanel-dimmed-even-further);
Updated•6 years ago
|
Assignee | ||
Comment 40•6 years ago
|
||
The
> border-radius: 10000px;
> padding: 1px 8px;
are needed to make the zoom reset button the rectangle with curved edges shape it currently is instead of a regular boxy rectangle so I left them in. I did change the colors for the various states though.
Flags: needinfo?(masinico)
Comment hidden (mozreview-request) |
Reporter | ||
Comment 42•6 years ago
|
||
mozreview-review |
Comment on attachment 8948605 [details] Bug 1417880: Allow theming arrow panels(popups). https://reviewboard.mozilla.org/r/218022/#review225482
Attachment #8948605 -
Flags: review+
Comment 43•6 years ago
|
||
Pushed by ntim.bugs@gmail.com: https://hg.mozilla.org/integration/autoland/rev/719333bb05af Allow theming arrow panels(popups). r=jaws
Comment 44•6 years ago
|
||
Backed out changeset 719333bb05af (bug 1417880) for failing in toolkit/components/extensions/test/browser/browser_ext_themes_arrowpanels.js on a CLOSED TREE Push on which the failures started: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=719333bb05af84c0f87065b33105f7a4b0da535c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-resultStatus=pending&filter-resultStatus=running&filter-searchStr=79d87fbd1fcd2f263d1ad926abd71464dfa6d2b5 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=161924905&repo=autoland&lineNumber=5194 Backout push: https://hg.mozilla.org/integration/autoland/rev/ef64ddb693f462bda7742bb7840f93d249ebe64f
Flags: needinfo?(masinico)
Reporter | ||
Comment 45•6 years ago
|
||
mozreview-review |
Comment on attachment 8948605 [details] Bug 1417880: Allow theming arrow panels(popups). https://reviewboard.mozilla.org/r/218022/#review225754 ::: toolkit/components/extensions/test/browser/browser_ext_themes_arrowpanels.js:67 (Diff revision 17) > + `rgb(${hexToRGB(POPUP_TEXT_COLOR).join(", ")})`, > + "Popup text color should have been themed" > + ); > + > + // Ensure popup border color was set properly > + testBorderColor(arrowContent, POPUP_BORDER_COLOR); On macOS, the border is applied as a box-shadow, so you'll probably want something like: if (AppConstants.platform == "macosx") { Assert.ok( arrowContentComputedStyle.getPropertyValue("box-shadow").includes(`rgb(${hexToRGB(POPUP_TEXT_COLOR).join(", ")})`), "Popup border color should be set" ); } else { testBorderColor(...); }
Comment 46•6 years ago
|
||
mozreview-review |
Comment on attachment 8948605 [details] Bug 1417880: Allow theming arrow panels(popups). https://reviewboard.mozilla.org/r/218022/#review226090 Clearing review request - this already landed (and bounced). ntim has good feedback here on how to get the test fixed.
Attachment #8948605 -
Flags: review?(mconley)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 49•6 years ago
|
||
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s b0e38fb1dc4807a742ccfdaf56774cd7bf44fbc6 -d 9688823d23fd: rebasing 447624:b0e38fb1dc48 "Bug 1417880: Allow theming arrow panels(popups). r=jaws,ntim" (tip) merging browser/themes/shared/customizableui/panelUI.inc.css merging toolkit/components/extensions/ext-theme.js merging toolkit/components/extensions/schemas/theme.json merging toolkit/components/extensions/test/browser/browser.ini merging toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields.js merging toolkit/components/extensions/test/browser/head.js merging toolkit/modules/LightweightThemeConsumer.jsm warning: conflicts while merging toolkit/modules/LightweightThemeConsumer.jsm! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request) |
Reporter | ||
Comment 51•6 years ago
|
||
mozreview-review |
Comment on attachment 8948605 [details] Bug 1417880: Allow theming arrow panels(popups). https://reviewboard.mozilla.org/r/218022/#review226688 ::: toolkit/modules/LightweightThemeConsumer.jsm:18 (Diff revision 20) > + ["--arrowpanel-background", "popup"], > + ["--arrowpanel-color", "popup_text"], > + ["--arrowpanel-border-color", "popup_border"], Remove this, and add this in ThemeVariableMap.jsm (it's in the browser/ directory).
Attachment #8948605 -
Flags: review+
Comment 52•6 years ago
|
||
(In reply to Tim Nguyen :ntim from comment #51) > Comment on attachment 8948605 [details] > Bug 1417880: Allow theming arrow panels(popups). > > https://reviewboard.mozilla.org/r/218022/#review226688 > > ::: toolkit/modules/LightweightThemeConsumer.jsm:18 > (Diff revision 20) > > + ["--arrowpanel-background", "popup"], > > + ["--arrowpanel-color", "popup_text"], > > + ["--arrowpanel-border-color", "popup_border"], > > Remove this, and add this in ThemeVariableMap.jsm (it's in the browser/ > directory). Why? --arrowpanel-* are toolkit variables.
Comment 53•6 years ago
|
||
The ThemeVariableMap.jsm are app specific now. Some variables use IDs which are FX specific and TB don't have. See bug 1436100.
Comment 54•6 years ago
|
||
Yet --arrowpanel-* are toolkit variables. Why not set them in toolkit/?
Reporter | ||
Comment 55•6 years ago
|
||
(In reply to Dão Gottwald [::dao] from comment #54) > Yet --arrowpanel-* are toolkit variables. Why not set them in toolkit/? ThemeVariableMap.jsm is just a map from existing CSS variables to the property names exposed in each product.
Reporter | ||
Comment 56•6 years ago
|
||
There's also no ThemeVariableMap.jsm inside toolkit atm.
Comment 57•6 years ago
|
||
(In reply to Tim Nguyen :ntim from comment #56) > There's also no ThemeVariableMap.jsm inside toolkit atm. Right. LightweightThemeConsumer.jsm is already in toolkit so it wouldn't need another module for toolkit variables.
Comment 58•6 years ago
|
||
Pushed by ntim.bugs@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/29569f02015d Allow theming arrow panels. r=jaws, ntim
Updated•6 years ago
|
Flags: needinfo?(masinico)
Comment 59•6 years ago
|
||
Why was this landed without the question where toolkit variables should go sorted out?
Flags: needinfo?(ntim.bugs)
Reporter | ||
Comment 60•6 years ago
|
||
(In reply to Dão Gottwald [::dao] from comment #59) > Why was this landed without the question where toolkit variables should go > sorted out? Bug 1436100 introduced the change, so I don't see why this bug specifically should fix its fallouts.
Flags: needinfo?(ntim.bugs)
Comment 61•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/29569f02015d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Comment 62•6 years ago
|
||
We seem to keep talking past each other. Bug 1436100 moved browser variable mappings to browser/, which makes a lot of sense. This bug doing the same with toolkit variables isn't bug 1436100's fallout. Now, I'm not saying that putting these variables in browser/ is a showstopper, but cutting the discussion short by just landing the patch after a review peer raised a concern isn't the way to go. Turns out there's also a pending review request for mconley. I've backed this out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 63•6 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e09d310a14b2
Comment 64•6 years ago
|
||
Can we have a ThemeVariableMap.jsm in toolkit, and one in browser, but the one in browser extends whatever is defined by the toolkit one?
Reporter | ||
Comment 65•6 years ago
|
||
(In reply to Dão Gottwald [::dao] from comment #62) > cutting the discussion short by just landing the patch after a review peer raised a concern isn't the way to go. That wasn't my intent. Since I didn't see any direct reply relating to comment 55 (which was my answer to the question), I assumed you were ok with it. Sorry for the misunderstanding. Feel free to needinfo? me next time if whatever answer I provide is not sufficient. To expand on comment 55, having toolkit CSS variables in the product-specific mapping doesn't really break the meaning of "product-specific mapping", since toolkit/ is technically part of the product. Also, I think introducing a new map for toolkit unnecessarily introduces complexity without actually giving us much benefit. The only benefit I possibly see is reducing the duplicated items between comm-central and mozilla-central, but I don't really see what it would bring otherwise...
Comment 66•6 years ago
|
||
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #64) > Can we have a ThemeVariableMap.jsm in toolkit, and one in browser, but the > one in browser extends whatever is defined by the toolkit one? As I tried to say in comment 57 already, why not just let LightweightThemeConsumer.jsm keep its own array besides the app-specific ThemeVariableMap.jsm?
Comment 67•6 years ago
|
||
Backout landed on central: https://hg.mozilla.org/integration/mozilla-inbound/rev/e09d310a14b269778f40c1cc563db28ed93c68f2
Comment 68•6 years ago
|
||
fwiw, I think a review from jaws and ntim should be fine, and shouldn't require yet another review from me in order to land. That's definitely not Connor's fault here - more of a process thing; I imagine whoever gets to the patch first can review it, and clear the other reviewers unless they don't feel confident being the sole reviewer. If that was the case here, I can still review - just let me know. In any event, Connor, sorry you're kinda caught here - a discussion about where variables need to live is happening, and that's what's blocking your patch. Not your fault at all. We'll get this sorted out soon. ni?ing ntim for dao's question in comment 66.
Flags: needinfo?(ntim.bugs)
Reporter | ||
Comment 69•6 years ago
|
||
(In reply to Dão Gottwald [::dao] from comment #66) > (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #64) > > Can we have a ThemeVariableMap.jsm in toolkit, and one in browser, but the > > one in browser extends whatever is defined by the toolkit one? > > As I tried to say in comment 57 already, why not just let > LightweightThemeConsumer.jsm keep its own array besides the app-specific > ThemeVariableMap.jsm? We could do something similar to `ThemeVariableMap = ThemeVariableMap.concat(<toolkit vars>)`. I honestly still don't understand where's the benefit though. The current mapping reads "CSS variable X maps to WebExtension property Y on Firefox", it honestly does not make a difference that X is from toolkit, this is just an implementation detail... What matters the most in the mapping is the WebExtension property, not the CSS variable. Scattering the mapping over two places seems counterproductive and makes the mapping harder to read IMHO. I'm not convinced this should be done unless there is some real benefit into doing this.
Flags: needinfo?(ntim.bugs) → needinfo?(dao+bmo)
Comment 70•6 years ago
|
||
(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #68) > fwiw, I think a review from jaws and ntim should be fine, and shouldn't > require yet another review from me in order to land. That's definitely not > Connor's fault here - more of a process thing; I imagine whoever gets to the > patch first can review it, and clear the other reviewers unless they don't > feel confident being the sole reviewer. If that was the case here, I can > still review - just let me know. > > In any event, Connor, sorry you're kinda caught here - a discussion about > where variables need to live is happening, and that's what's blocking your > patch. Not your fault at all. We'll get this sorted out soon. The patch changed after jaws r+'d it and Connor didn't even seem to be involved with that, so definitely not his fault. (In reply to Tim Nguyen :ntim from comment #69) > I honestly still don't understand where's the benefit though. The current > mapping reads "CSS variable X maps to WebExtension property Y on Firefox", > it honestly does not make a difference that X is from toolkit, this is just > an implementation detail... Sure, it is an implementation detail. We're discussing the implementation here. What's your point? > What matters the most in the mapping is the > WebExtension property, not the CSS variable. Scattering the mapping over two > places seems counterproductive and makes the mapping harder to read IMHO. Harder to read for whom? I hope we don't point people to ThemeVariableMap.jsm as some kind of webextension theme documentation? > I'm not convinced this should be done unless there is some real benefit into > doing this. The benefit would be that the toolkit theme stuff would be self-contained and shared with other users, who (1) won't have to maintain this part themselves and (2) may even contribute back to the shared stuff.
Flags: needinfo?(dao+bmo)
Reporter | ||
Comment 71•6 years ago
|
||
> Harder to read for whom? For future people looking at this code ? It doesn't allow you to have a concise overview of how properties are mapped for each product. > The benefit would be that the toolkit theme stuff would be self-contained and shared with other users, who (1) won't have to maintain this part themselves and (2) may even contribute back to the shared stuff. The main point bug 1436100 moved the mapping is because comm-central wanted more flexibility over the mapping... and because toolkit changes broke TB. It might be counterproductive for TB to have toolkit mappings forced on them: they might want to implement the "popup" property differently, simply because they don't have any arrow panels or urlbar autocomplete... There are probably more than one example on this, since TB and Firefox are quite different products. It's really about how each product chooses how to implement the WebExtension property, if they choose to use the shared toolkit CSS variable, they may do it by adding a single line in the mapping, if they don't choose to use the shared variable or choose to extend it for their product, they can choose to include a different variable... I don't understand why the CSS variable location matters so much here, this is really about how each product chooses to implement the WebExtension property, not about where each CSS variable is located.
Comment 72•6 years ago
|
||
(In reply to Tim Nguyen :ntim from comment #71) > > Harder to read for whom? > > For future people looking at this code ? It doesn't allow you to have a > concise overview of how properties are mapped for each product. I assume by people you mean engineers. The only reference to ThemeVariableMap.jsm is in LightweightThemeConsumer.jsm where also the array for toolkit variables would reside. Seems straightforward enough. > > The benefit would be that the toolkit theme stuff would be self-contained and shared with other users, who (1) won't have to maintain this part themselves and (2) may even contribute back to the shared stuff. > > The main point bug 1436100 moved the mapping is because comm-central wanted > more flexibility over the mapping... and because toolkit changes broke TB. > It might be counterproductive for TB to have toolkit mappings forced on > them: they might want to implement the "popup" property differently, simply > because they don't have any arrow panels or urlbar autocomplete... There are > probably more than one example on this, since TB and Firefox are quite > different products. Unless I'm missing something, ThemeVariableMap.jsm can map properties to app-specific variables despite LightweightThemeConsumer.jsm already mapping the same properties to toolkit theme variables.
Reporter | ||
Comment 73•6 years ago
|
||
> Unless I'm missing something, ThemeVariableMap.jsm can map properties to app-specific variables despite LightweightThemeConsumer.jsm already mapping the same properties to toolkit theme variables.
In the current setup, that would cause both the item from ThemeVariableMap.jsm and from LightweightThemeConsumer.jsm to be used. One wouldn't override the other.
Comment 74•6 years ago
|
||
Yes, that's how I'd expect it to work. We want to be able to map theme properties to multiple variables.
Reporter | ||
Comment 75•6 years ago
|
||
(In reply to Dão Gottwald [::dao] from comment #74) > Yes, that's how I'd expect it to work. We want to be able to map theme > properties to multiple variables. What if TB doesn't want the mapping from toolkit ?
Comment 76•6 years ago
|
||
(In reply to Tim Nguyen :ntim from comment #75) > (In reply to Dão Gottwald [::dao] from comment #74) > > Yes, that's how I'd expect it to work. We want to be able to map theme > > properties to multiple variables. > > What if TB doesn't want the mapping from toolkit ? You might as well ask "what if Thunderbird doesn't want rule X from toolkit stylesheet Y?" Thunderbird may override it, and is also free to not using the toolkit theme at all. But when it does, it seems reasonable to me that this is by default part of the package.
Comment 77•6 years ago
|
||
mozreview-review |
Comment on attachment 8948605 [details] Bug 1417880: Allow theming arrow panels(popups). https://reviewboard.mozilla.org/r/218022/#review228620 ::: toolkit/modules/LightweightThemeConsumer.jsm:20 (Diff revision 20) > ChromeUtils.defineModuleGetter(this, "LightweightThemeImageOptimizer", > "resource://gre/modules/addons/LightweightThemeImageOptimizer.jsm"); > > + ["--arrowpanel-background", "popup"], > + ["--arrowpanel-color", "popup_text"], > + ["--arrowpanel-border-color", "popup_border"], Please put these in an array before the ThemeVariableMap.jsm import. ::: toolkit/modules/LightweightThemeConsumer.jsm:201 (Diff revision 20) > elem.style.removeProperty(variableName); > } > } > > function _setProperties(root, active, vars) { > for (let [cssVarName, varsKey, optionalElementID] of ThemeVariableMap) { ... and walk both arrays here like so: for (let map of [toolkitVariableMap, ThemeVariableMap]) { for (let [cssVarName, varsKey, optionalElementID] of map) { Thanks and sorry for the hassle!
Updated•6 years ago
|
Attachment #8948605 -
Flags: review?(mconley)
Comment hidden (mozreview-request) |
Comment 79•6 years ago
|
||
mozreview-review |
Comment on attachment 8948605 [details] Bug 1417880: Allow theming arrow panels(popups). https://reviewboard.mozilla.org/r/218022/#review228700 Clearing review - there are some open-issues that need to be addressed before this can land.
Attachment #8948605 -
Flags: review?(mconley)
Comment 80•6 years ago
|
||
Oh! I apologize - that latest patch addresses the issues. Connor, would you mind please marking them "Fixed"?
Flags: needinfo?(masinico)
Assignee | ||
Comment 81•6 years ago
|
||
(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #80) > Oh! I apologize - that latest patch addresses the issues. Connor, would you > mind please marking them "Fixed"? All issues have been dealt with. Sorry I forgot about that!
Flags: needinfo?(masinico)
Comment 82•6 years ago
|
||
mozreview-review |
Comment on attachment 8948605 [details] Bug 1417880: Allow theming arrow panels(popups). https://reviewboard.mozilla.org/r/218022/#review228704 Just two cosmetic issues, but otherwise, this looks great! Thanks Connor! ::: toolkit/components/extensions/test/browser/browser_ext_themes_arrowpanels.js:42 (Diff revision 21) > + }, > + files: { > + "image1.png": BACKGROUND, > + }, > + }); > + Nit - please remove this extra newline. ::: toolkit/modules/LightweightThemeConsumer.jsm:14 (Diff revision 21) > +["--arrowpanel-background", "popup"], > +["--arrowpanel-color", "popup_text"], > +["--arrowpanel-border-color", "popup_border"], Please indent these 2 spaces.
Attachment #8948605 -
Flags: review+
Comment hidden (mozreview-request) |
Comment 84•6 years ago
|
||
Thanks, Connor! Pushed to Try for you. If it comes back green, we can go ahead and autoland this.
Comment hidden (mozreview-request) |
Comment 86•6 years ago
|
||
Pushed by ntim.bugs@gmail.com: https://hg.mozilla.org/integration/autoland/rev/fc6f924977b4 Allow theming arrow panels(popups). r=jaws,mconley,ntim
Comment 87•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fc6f924977b4
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Resolution: --- → FIXED
Comment 88•6 years ago
|
||
:ntim, I think you have updated the docs page and data for this, so if you are happy with the screenshots we can mark it dev-doc-complete.
Flags: needinfo?(ntim.bugs)
Reporter | ||
Updated•6 years ago
|
Flags: needinfo?(ntim.bugs)
Updated•6 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
•