Closed
Bug 1423757
Opened 6 years ago
Closed 5 years ago
Allow theming toolbar fields focused state
Categories
(WebExtensions :: Themes, enhancement, P5)
WebExtensions
Themes
Tracking
(firefox61 fixed)
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: ntim, Assigned: lianzhen)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
It would be nice to be able to theme the hover/focused state of the toolbar fields: notably the background/border/text/box-shadow
Updated•6 years ago
|
Severity: normal → enhancement
Priority: -- → P5
Updated•5 years ago
|
Depends on: dark-theme-darkening
Updated•5 years ago
|
Blocks: dark-theme-darkening
No longer depends on: dark-theme-darkening
Updated•5 years ago
|
Component: WebExtensions: Frontend → WebExtensions: Themes
Updated•5 years ago
|
Assignee: nobody → lianzhen
Comment hidden (mozreview-request) |
Comment 2•5 years ago
|
||
mozreview-review |
Comment on attachment 8956736 [details] Bug 1423757 - Allow theming toolbar fields focused state https://reviewboard.mozilla.org/r/225696/#review231758 ::: browser/modules/ThemeVariableMap.jsm:29 (Diff revision 1) > ["--lwt-toolbarbutton-icon-fill-attention", "icon_attention_color"], > ["--lwt-toolbarbutton-hover-background", "button_background_hover"], > ["--lwt-toolbarbutton-active-background", "button_background_active"], > ["--lwt-selected-tab-background-color", "tab_selected"], > + ["--lwt-toolbar-field-hover", "toolbar_field_hover_background"], > + ["--lwt-toolbar-field-hover-text", "toolbar_field_hover_text_color"], Please use --lwt-toolbar-field-hover-color instead of *-text since it is clearer that we expect a `color` value here. ::: toolkit/components/extensions/schemas/theme.json:188 (Diff revision 1) > + }, > + "toolbar_field_hover_border": { > + "$ref": "ThemeColor", > + "optional": true > } > + Please remove this blank line. ::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:6 (Diff revision 1) > + const TOOLBAR_BACKGROUND = "#FFFFFF"; > + const TOOLBAR_TEXT = "#000000"; These can be removed, we don't need to test what the initial colors are. ::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:10 (Diff revision 1) > +add_task(async function test_toolbar_field_hover() { > + const TOOLBAR_BACKGROUND = "#FFFFFF"; > + const TOOLBAR_TEXT = "#000000"; > + const TOOLBAR_HOVER_BACKGROUND = "#FF0000"; > + const TOOLBAR_HOVER_TEXT = "#9400FF"; > + const TOOLBAR_HOVER_BORDER = "#FFFFFF"; You should have a test for TOOLBAR_HOVER_BORDER ::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:15 (Diff revision 1) > + "images": { > + "headerURL": "image1.png", > + }, Images are not required for themes anymore (bug 1404688) so this can be removed as well as the `files` section below. ::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:19 (Diff revision 1) > + "theme": { > + "images": { > + "headerURL": "image1.png", > + }, > + "colors": { > + "accentcolor": "#000000", Your test was failing becuase you didn't specify a `textcolor` here. Therefore the theme wasn't valid and wasn't being applied. ::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:20 (Diff revision 1) > + "toolbar": TOOLBAR_BACKGROUND, > + "toolbar_text": TOOLBAR_TEXT, These two should be removed. ::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:34 (Diff revision 1) > + // Remove the `remotecontrol` attribute since it interferes with the urlbar styling. > + document.documentElement.removeAttribute("remotecontrol"); I see that this part was copied from another test, so it's not your fault, but we should be using registerCleanupFunction after we remove this attribute to guarantee that it gets re-added back in case an exception gets thrown before line 63 is reached. See https://searchfox.org/mozilla-central/rev/efce4ceddfbe5fe4e2d74f1aed93894bada9b273/browser/components/extensions/test/browser/browser_ext_tabs_create.js#14 for example usage. ::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:39 (Diff revision 1) > + let urlBarQuery = document.querySelector("#urlbar"); > + let urlBar = document.getElementById("urlbar"); These two variables will be referencing the same element. ::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:41 (Diff revision 1) > + > + info("Checking toolbar field's hover color"); > + > + let urlBarQuery = document.querySelector("#urlbar"); > + let urlBar = document.getElementById("urlbar"); > + Assert.equal(window.getComputedStyle(urlBar, ":hover").backgroundColor, The second argument to getComputedStyle is for pseudo-elements, such as ::before and ::after. Using :hover here has no effect, and can be removed. ::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:41 (Diff revision 1) > + Assert.equal(window.getComputedStyle(urlBar, ":hover").backgroundColor, > + `rgb(${hexToRGB(TOOLBAR_BACKGROUND).join(", ")})`, > + "Initial background color is not changed"); > + Assert.equal(window.getComputedStyle(urlBar).color, You can keep these assertions, but change them to Assert.notEqual and compare against their respective hover colors. ::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:48 (Diff revision 1) > + "Initial background color is not changed"); > + Assert.equal(window.getComputedStyle(urlBar).color, > + `rgb(${hexToRGB(TOOLBAR_TEXT).join(", ")})`, > + "Initial text color is not changed"); > + > + urlBarQuery.setAttribute("focused", "true"); You don't need to set the focused="true" attribute. The background color is set when either focused="true" attribute is present or the :hover pseudo-class is applied. ::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:59 (Diff revision 1) > + "Hover Background Color is changed"); > + Assert.equal(window.getComputedStyle(urlBar).color, > + `rgb(${hexToRGB(TOOLBAR_HOVER_TEXT).join(", ")})`, > + "Hover Text Color is changed"); > + > + nit, this extra line with trailing whitespace should be deleted.
Attachment #8956736 -
Flags: review?(jaws) → review+
Comment 3•5 years ago
|
||
mozreview-review |
Comment on attachment 8956736 [details] Bug 1423757 - Allow theming toolbar fields focused state https://reviewboard.mozilla.org/r/225696/#review231768 Sorry, meant to mark r-
Attachment #8956736 -
Flags: review+ → review-
Comment hidden (mozreview-request) |
Reporter | ||
Comment 5•5 years ago
|
||
Can we just allow theming the focus state for now ? It is the use-case theme developers are requesting, and is more straightforward to implement than the hover state. colors.toolbar_field_focus colors.toolbar_field_text_focus colors.toolbar_field_border_focus and potentially: properties.toolbar_field_shadow_focus
Comment hidden (mozreview-request) |
Comment 7•5 years ago
|
||
mozreview-review |
Comment on attachment 8956736 [details] Bug 1423757 - Allow theming toolbar fields focused state https://reviewboard.mozilla.org/r/225696/#review232414 ::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:37 (Diff revision 3) > + Assert.notEqual(window.getComputedStyle(urlBar).backgroundColor, > + `rgb(${hexToRGB(TOOLBAR_FIELD_BACKGROUND).join(", ")})`, > + "Hover Background Color is not equal to initial"); Sorry, I may not have been clear enough on my previous review. Now that you've got these changes made, it doesn't make much sense to have an assertion for "not equal" followed by an assertion for "equal". Since the two are mutually exclusive, the "equal" case will already cover the "not equal" case. You should move "not equal" assertions to above the call to addPseudoClassLock, and change them from "not equal" to "equal" since the urlBar *should* have those styles before :hover is applied. ::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:52 (Diff revision 3) > + // Restore the `remotecontrol` attribute at the end of the test. > + registerCleanupFunction(() => { > + SpecialPowers.clearUserPref("remotecontrol"); > + }); This should be moved up to where the attribute is removed, and secondly, this is now incorrect in calling clearUserPref. Your previous code added the attribute back, this is now clearing some preference that doesn't exist.
Attachment #8956736 -
Flags: review?(jaws) → review-
Updated•5 years ago
|
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment 9•5 years ago
|
||
mozreview-review |
Comment on attachment 8956736 [details] Bug 1423757 - Allow theming toolbar fields focused state https://reviewboard.mozilla.org/r/225696/#review233740 Looks good, basically done after you make the following changes. r+ from me with the below changes made. ::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:48 (Diff revision 4) > + Assert.notEqual(window.getComputedStyle(urlBar).backgroundColor, > + `rgb(${hexToRGB(TOOLBAR_HOVER_BACKGROUND).join(", ")})`, This should be checking Assert.equal() with TOOLBAR_FIELD_BACKGROUND ::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:50 (Diff revision 4) > + > + urlBar.removeAttribute("focused"); > + > + Assert.notEqual(window.getComputedStyle(urlBar).backgroundColor, > + `rgb(${hexToRGB(TOOLBAR_HOVER_BACKGROUND).join(", ")})`, > + "Hover Background Color is set back to initial"); remove the "Hover" word here ::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:51 (Diff revision 4) > + Assert.notEqual(window.getComputedStyle(urlBar).color, > + `rgb(${hexToRGB(TOOLBAR_HOVER_TEXT).join(", ")})`, This shoudl be checking Assert.equal() with TOOLBAR_FIELD_COLOR ::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:53 (Diff revision 4) > + Assert.notEqual(window.getComputedStyle(urlBar).backgroundColor, > + `rgb(${hexToRGB(TOOLBAR_HOVER_BACKGROUND).join(", ")})`, > + "Hover Background Color is set back to initial"); > + Assert.notEqual(window.getComputedStyle(urlBar).color, > + `rgb(${hexToRGB(TOOLBAR_HOVER_TEXT).join(", ")})`, > + "Hover Text Color is set back to initial"); remove the "Hover" here ::: toolkit/modules/LightweightThemeConsumer.jsm:85 (Diff revision 4) > break; > } > }, > > _update(aData) { > + console.log(aData); remove the console.log() here and below ::: toolkit/modules/LightweightThemeConsumer.jsm:186 (Diff revision 4) > > function _setProperties(root, active, vars) { > + console.log(vars); > for (let map of [toolkitVariableMap, ThemeVariableMap]) { > for (let [cssVarName, varsKey, optionalElementID] of map) { > + // console.log(cssVarName, varsKey, optionalElementID, vars[varsKey]); Remove this comment
Attachment #8956736 -
Flags: review?(jaws) → review+
Reporter | ||
Comment 10•5 years ago
|
||
mozreview-review |
Comment on attachment 8956736 [details] Bug 1423757 - Allow theming toolbar fields focused state https://reviewboard.mozilla.org/r/225696/#review233346 In addition to :jaws comments, could you rename all `HOVER` vars in your test to use `FOCUS` ? ::: browser/themes/shared/urlbar-searchbar.inc.css:68 (Diff revision 4) > - background-color: var(--url-and-searchbar-background-color, white); > + background-color: var(--lwt-toolbar-field-focus, white); > + color: var(--lwt-toolbar-field-focus-color, black); > + border-color: var(--lwt-toolbar-field-focus-border-color, white); Those 3 properties should first fallback to the non-focus properties if the focus properties are no specified, then fallback to the default values. Here's how it would be done for background-color: background-color: var(--lwt-toolbar-field-focus, var(--url-and-searchbar-background-color, white)); This basically says if the focus variable is not specified, use the non-focus variable, and if that's not specified, use white.
Attachment #8956736 -
Flags: review?(ntim.bugs)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 13•5 years ago
|
||
mozreview-review |
Comment on attachment 8956736 [details] Bug 1423757 - Allow theming toolbar fields focused state https://reviewboard.mozilla.org/r/225696/#review234336 ::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:30 (Diff revision 6) > + // Remove the `remotecontrol` attribute since it interferes with the urlbar styling. > + document.documentElement.removeAttribute("remotecontrol"); I think what jaws meant here was: ``` document.documentElement.removeAttribute("remotecontrol"); registerCleanupFunction(() => { document.documentElement.setAttribute("remotecontrol", "true"); }); ``` ::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:33 (Diff revision 6) > + await extension.startup(); > + > + // Remove the `remotecontrol` attribute since it interferes with the urlbar styling. > + document.documentElement.removeAttribute("remotecontrol"); > + > + info("Checking toolbar field's hover color"); s/hover/focus ::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_hover.js:55 (Diff revision 6) > + // Restore the `remotecontrol` attribute at the end of the test. > + document.documentElement.setAttribute("remotecontrol", true); > + registerCleanupFunction(() => { > + SpecialPowers.clearUserPref("remotecontrol"); > + }); If you do what I wrote above, you should be able to remove this.
Attachment #8956736 -
Flags: review?(ntim.bugs) → review+
Reporter | ||
Comment 14•5 years ago
|
||
mozreview-review |
Comment on attachment 8956736 [details] Bug 1423757 - Allow theming toolbar fields focused state https://reviewboard.mozilla.org/r/225696/#review234342 ::: toolkit/components/extensions/test/browser/browser.ini:20 (Diff revision 6) > [browser_ext_themes_separators.js] > [browser_ext_themes_static_onUpdated.js] > [browser_ext_themes_tab_line.js] > [browser_ext_themes_tab_loading.js] > [browser_ext_themes_tab_text.js] > +[browser_ext_themes_toolbar_fields_hover.js] s/[browser_ext_themes_toolbar_fields_hover.js]/[browser_ext_themes_toolbar_fields_focus.js]
Comment hidden (mozreview-request) |
Comment 16•5 years ago
|
||
mozreview-review |
Comment on attachment 8956736 [details] Bug 1423757 - Allow theming toolbar fields focused state https://reviewboard.mozilla.org/r/225696/#review234472 Code analysis found 5 defects in this patch: - 5 defects 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/browser_ext_themes_toolbar_fields_focus.js:51 (Diff revision 7) > + testBorderColor(urlBar, TOOLBAR_FOCUS_BORDER); > + > + urlBar.removeAttribute("focused"); > + > + Assert.equal(window.getComputedStyle(urlBar).backgroundColor, > + `rgb(${hexToRGB(TOOLBAR_FIELD_BACKGROUND).join(", ")})`, Error: Expected indentation of 15 spaces but found 18. [eslint: indent] ::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_focus.js:52 (Diff revision 7) > + > + urlBar.removeAttribute("focused"); > + > + Assert.equal(window.getComputedStyle(urlBar).backgroundColor, > + `rgb(${hexToRGB(TOOLBAR_FIELD_BACKGROUND).join(", ")})`, > + "Background Color is set back to initial"); Error: Expected indentation of 15 spaces but found 18. [eslint: indent] ::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_focus.js:54 (Diff revision 7) > + > + Assert.equal(window.getComputedStyle(urlBar).backgroundColor, > + `rgb(${hexToRGB(TOOLBAR_FIELD_BACKGROUND).join(", ")})`, > + "Background Color is set back to initial"); > + Assert.equal(window.getComputedStyle(urlBar).color, > + `rgb(${hexToRGB(TOOLBAR_FIELD_COLOR).join(", ")})`, Error: Expected indentation of 15 spaces but found 18. [eslint: indent] ::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_focus.js:55 (Diff revision 7) > + Assert.equal(window.getComputedStyle(urlBar).backgroundColor, > + `rgb(${hexToRGB(TOOLBAR_FIELD_BACKGROUND).join(", ")})`, > + "Background Color is set back to initial"); > + Assert.equal(window.getComputedStyle(urlBar).color, > + `rgb(${hexToRGB(TOOLBAR_FIELD_COLOR).join(", ")})`, > + "Text Color is set back to initial"); Error: Expected indentation of 15 spaces but found 18. [eslint: indent] ::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_focus.js:56 (Diff revision 7) > + `rgb(${hexToRGB(TOOLBAR_FIELD_BACKGROUND).join(", ")})`, > + "Background Color is set back to initial"); > + Assert.equal(window.getComputedStyle(urlBar).color, > + `rgb(${hexToRGB(TOOLBAR_FIELD_COLOR).join(", ")})`, > + "Text Color is set back to initial"); > + Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]
Comment hidden (mozreview-request) |
Reporter | ||
Updated•5 years ago
|
Summary: Allow theming toolbar fields hover & focused state → Allow theming toolbar fields focused state
Comment 18•5 years ago
|
||
Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6fbbb00841e4 Allow theming toolbar fields hover & focused state r=jaws,ntim
Comment 19•5 years ago
|
||
Backed out changeset 6fbbb00841e4 (bug 1423757) for failing browser_ext_themes_toolbar_fields_focus.js Backout link: https://hg.mozilla.org/integration/autoland/rev/03a0851d631012ed637e28092385642871fd05fd Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=6fbbb00841e489a639cbe766ad5f02c798c21ac7 Failure log link: https://treeherder.mozilla.org/logviewer.html#?job_id=169988257&repo=autoland&lineNumber=1983 Log snippet: [task 2018-03-23T22:43:04.996Z] 22:43:04 INFO - TEST-START | toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_focus.js [task 2018-03-23T22:43:04.997Z] 22:43:04 INFO - TEST-INFO | started process screentopng [task 2018-03-23T22:43:06.029Z] 22:43:06 INFO - TEST-INFO | screentopng: exit 0 [task 2018-03-23T22:43:06.030Z] 22:43:06 INFO - TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_focus.js | Exception thrown - [Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIXPCComponents_Utils.importGlobalProperties]" nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)" location: "JS frame :: chrome://mochitests/content/browser/toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_focus.js :: <TOP_LEVEL> :: line 3" data: no] [task 2018-03-23T22:43:06.033Z] 22:43:06 INFO - GECKO(1054) | MEMORY STAT vsizeMaxContiguous not supported in this build configuration. [task 2018-03-23T22:43:06.033Z] 22:43:06 INFO - GECKO(1054) | MEMORY STAT | vsize 2008MB | residentFast 253MB | heapAllocated 99MB [task 2018-03-23T22:43:06.033Z] 22:43:06 INFO - TEST-OK | toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_focus.js | took 49ms [task 2018-03-23T22:43:06.034Z] 22:43:06 INFO - checking window state [task 2018-03-23T22:43:06.035Z] 22:43:06 INFO - Console message: [JavaScript Warning: "Use of nsIFile in content process is deprecated." {file: "resource://gre/modules/FileUtils.jsm" line: 170}] [task 2018-03-23T22:43:06.679Z] 22:43:06 INFO - Console message: [JavaScript Warning: "Use of nsIFile in content process is deprecated." {file: "resource://gre/modules/FileUtils.jsm" line: 170}] [task 2018-03-23T22:43:07.482Z] 22:43:07 INFO - GECKO(1054) | Completed ShutdownLeaks collections in process 1167 [task 2018-03-23T22:43:07.490Z] 22:43:07 INFO - GECKO(1054) | Completed ShutdownLeaks collections in process 1191 [task 2018-03-23T22:43:07.608Z] 22:43:07 INFO - GECKO(1054) | Completed ShutdownLeaks collections in process 1109 [task 2018-03-23T22:43:07.838Z] 22:43:07 INFO - GECKO(1054) | Completed ShutdownLeaks collections in process 1054
Flags: needinfo?(lianzhen)
Reporter | ||
Comment 20•5 years ago
|
||
mozreview-review |
Comment on attachment 8956736 [details] Bug 1423757 - Allow theming toolbar fields focused state https://reviewboard.mozilla.org/r/225696/#review236324 ::: commit-message-bccdc:1 (Diff revision 8) > +Bug 1423757 - Allow theming toolbar fields hover & focused state r?ntim,jaws nit: remove the "hover" part from the commit message ::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields_focus.js:3 (Diff revision 8) > +"use strict"; > + > +Cu.importGlobalProperties(["InspectorUtils"]); Looks like you don't need this, and it seems to be causing the problem in your test too.
Comment hidden (mozreview-request) |
Comment 22•5 years ago
|
||
Pushed by ntim.bugs@gmail.com: https://hg.mozilla.org/integration/autoland/rev/e811edda21f3 Allow theming toolbar fields focused state r=jaws,ntim
Updated•5 years ago
|
Flags: needinfo?(lianzhen)
Comment 23•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e811edda21f3
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 24•5 years ago
|
||
Is it planned to uplift this bug to beta? Then ESR will have the same theme feature set.
Flags: needinfo?(jaws)
Comment 25•5 years ago
|
||
No, we're not planning on uplifting theme features to beta for ESR support. It may be nice to not have the difference between release and ESR, but there are a number of new API features for theming that are landing in 61 and uplifting all of them would make fixing any bugs that are found during QA much riskier. We'll only uplift theme API changes if it is to fix a regression or compatibility problem, generally.
Flags: needinfo?(jaws)
Updated•5 years ago
|
Product: Toolkit → WebExtensions
Comment 26•5 years ago
|
||
Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.
Flags: needinfo?(lianzhen)
Reporter | ||
Updated•5 years ago
|
Flags: needinfo?(lianzhen) → qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•