Closed
Bug 1418605
Opened 6 years ago
Closed 6 years ago
The toolbar, toolbar_text, toolbar_field, toolbar_field_text, toolbar_field_border properties should apply to the findbar
Categories
(WebExtensions :: Themes, enhancement, P2)
WebExtensions
Themes
Tracking
(firefox61 fixed)
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: ntim, Assigned: bogdan, Mentored)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete)
Attachments
(3 files, 2 obsolete files)
No description provided.
Updated•6 years ago
|
Priority: -- → P5
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: nobody → masinico
Updated•6 years ago
|
Mentor: mconley, jaws, ntim.bugs
Comment 1•6 years ago
|
||
Actually, I think bug 1417880 is higher priority to have Connor work on. Sorry for the churn!
Assignee: masinico → nobody
Updated•6 years ago
|
Assignee: nobody → bogdan
Status: NEW → ASSIGNED
Reporter | ||
Updated•6 years ago
|
Summary: The toolbar, toolbar_text, toolbar_field, toolbar_field_text properties should apply to the findbar → The toolbar, toolbar_text, toolbar_field, toolbar_field_text, toolbar_field_border properties should apply to the findbar
Updated•6 years ago
|
Component: WebExtensions: Frontend → WebExtensions: Themes
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 4•6 years ago
|
||
mozreview-review |
Comment on attachment 8950018 [details] Bug 1418605 - The toolbar, toolbar_text, toolbar_field, toolbar_field_text, toolbar_field_border properties should apply to the findbar https://reviewboard.mozilla.org/r/219302/#review225028 ::: browser/themes/osx/browser.css:228 (Diff revision 2) > /* Override OSX-specific toolkit findbar button styles */ > .findbar-button { > background: none; > box-shadow: none; > border: none; > + color: var(--toolbar-color); color: inherit; is probably a better choice, so it works for the default theme. Also, is this needed with the rule in themes/shared ::: browser/themes/shared/toolbarbuttons.inc.css:97 (Diff revision 2) > } > > .findbar-button { > -moz-appearance: none; > padding: 0; > + color: var(--toolbar-color); color: inherit; ::: toolkit/themes/linux/global/findBar.css:49 (Diff revision 2) > + border: 1px; > + border-style: solid; border: 1px solid;
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 8•6 years ago
|
||
mozreview-review |
Comment on attachment 8950018 [details] Bug 1418605 - The toolbar, toolbar_text, toolbar_field, toolbar_field_text, toolbar_field_border properties should apply to the findbar https://reviewboard.mozilla.org/r/219302/#review225046 ::: browser/themes/linux/browser.css:456 (Diff revision 5) > border: 1px ThreeDShadow; > border-style: none solid; > } > > .browserContainer > findbar { > background-color: -moz-dialog; background-color: var(--toolbar-bgcolor, -moz-dialog); ::: browser/themes/linux/browser.css:461 (Diff revision 5) > +.browserContainer > findbar:-moz-lwtheme { > + background: var(--toolbar-bgcolor); > +} Move this up to the .browserContainer > findbar block. ::: browser/themes/osx/browser.css:669 (Diff revision 5) > } > > /* ----- CONTENT ----- */ > > .browserContainer > findbar { > - background: @scopeBarBackground@; > + background-color: @scopeBarBackground@; scopeBarBackground is defined as a background expecting the shorthand syntax, so this is currently broken. You should expand the scopeBarBackground like you did for the roundButtonBorder and then you can set the background-color here. ::: toolkit/themes/linux/global/findBar.css:48 (Diff revision 5) > > /* Search field */ > > .findbar-textbox { > -moz-appearance: none; > - border: 1px solid ThreeDShadow; > + background-color: var(--url-and-searchbar-background-color, -moz-Field); We should probably rename this variable since it is being used elsewhere now and the name doesn't imply such. Please rename this to --toolbar-field-background-color. ::: toolkit/themes/linux/global/findBar.css:52 (Diff revision 5) > -moz-appearance: none; > - border: 1px solid ThreeDShadow; > + background-color: var(--url-and-searchbar-background-color, -moz-Field); > + border: 1px solid; > + border-color: var(--lwt-toolbar-field-border-color, ThreeDShadow); > box-shadow: 0 0 1px 0 ThreeDShadow inset; > + color: var(--url-and-searchbar-color); This should be renamed to --toolbar-field-color. ::: toolkit/themes/osx/global/findBar.css:203 (Diff revision 5) > + border-top-color: var(--lwt-toolbar-field-border-color); > + border-bottom-color: var(--lwt-toolbar-field-border-color); These should just set border-color ::: toolkit/themes/osx/global/shared.inc:6 (Diff revision 5) > %filter substitution > > %define loweredShadow 0 1px rgba(255, 255, 255, .4) > %define focusRingShadow 0 0 0 1px -moz-mac-focusring inset, 0 0 0 1px -moz-mac-focusring > > %define roundButtonBorder 1px solid rgba(0,0,0,.35) Is roundButtonBorder still used? It should be removed now that you've broken it apart, and any place that is still referencing it should use the long hand notation. ::: toolkit/themes/windows/global/findBar.css:113 (Diff revision 5) > + border-top-color: var(--lwt-toolbar-field-border-color); > + border-bottom-color: var(--lwt-toolbar-field-border-color); These should just be border-color, and then you can combine this block with the one for .findbar-find-next:-moz-lwtheme (in this file and the other files).
Attachment #8950018 -
Flags: review?(jaws) → review-
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 11•6 years ago
|
||
mozreview-review |
Comment on attachment 8950082 [details] Bug 1418605 - Rename --url-and-searchbar-background-color and --url-and-searchbar-color for clarity as they are used elsewhere https://reviewboard.mozilla.org/r/219352/#review225068 Looks good to me!
Attachment #8950082 -
Flags: review?(ntim.bugs) → review+
Updated•6 years ago
|
Attachment #8950018 -
Flags: review?(jaws) → review?(dao+bmo)
Updated•6 years ago
|
Attachment #8950018 -
Flags: review-
Updated•6 years ago
|
Attachment #8950018 -
Flags: review- → review?(jaws)
Comment 12•6 years ago
|
||
mozreview-review |
Comment on attachment 8950018 [details] Bug 1418605 - The toolbar, toolbar_text, toolbar_field, toolbar_field_text, toolbar_field_border properties should apply to the findbar https://reviewboard.mozilla.org/r/219302/#review225070 ::: browser/themes/linux/browser.css:455 (Diff revision 6) > .browserContainer > findbar { > - background-color: -moz-dialog; > - color: -moz-DialogText; > + background-color: var(--toolbar-bgcolor, -moz-dialog); > + color: var(--toolbar-color, -moz-DialogText); > text-shadow: none; > } Sorry for the churn here. Please move these to a .browserContainer > findbar:-moz-lwtheme section since --toolbar-bgcolor and --toolbar-color will always be defined. ::: browser/themes/osx/browser.css:669 (Diff revision 6) > - background: @scopeBarBackground@; > + background-color: var(--toolbar-bgcolor); > + background-image: @scopeBarBackgroundImage@; > + background-repeat: @scopeBarBackgroundRepeat@; > border-top: @scopeBarSeparatorBorder@; > - color: -moz-DialogText; > + color: var(--toolbar-color, -moz-DialogText); Same comment here as for the linux/browser.css file. ::: browser/themes/shared/toolbarbuttons.inc.css:97 (Diff revision 6) > } > > .findbar-button { > -moz-appearance: none; > padding: 0; > + color: inherit; What is the color set to without this rule? color should inherit by default already. ::: browser/themes/windows/browser.css:683 (Diff revision 6) > .browserContainer > findbar { > - background-color: -moz-dialog; > - color: -moz-DialogText; > + background-color: var(--toolbar-bgcolor, -moz-dialog); > + color: var(--toolbar-color, -moz-DialogText); Ditto. ::: toolkit/themes/linux/global/findBar.css:29 (Diff revision 6) > +findbar:-moz-lwtheme { > + background: var(--toolbar-bgcolor); > + color: var(--toolbar-color); This is trying to the same as what your /browser CSS changes already have. You can probably remove this.
Attachment #8950018 -
Flags: review?(jaws) → review-
Comment 13•6 years ago
|
||
mozreview-review |
Comment on attachment 8950082 [details] Bug 1418605 - Rename --url-and-searchbar-background-color and --url-and-searchbar-color for clarity as they are used elsewhere https://reviewboard.mozilla.org/r/219352/#review225072 Thanks!
Attachment #8950082 -
Flags: review?(mconley) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 16•6 years ago
|
||
mozreview-review |
Comment on attachment 8950082 [details] Bug 1418605 - Rename --url-and-searchbar-background-color and --url-and-searchbar-color for clarity as they are used elsewhere https://reviewboard.mozilla.org/r/219352/#review225848
Attachment #8950082 -
Flags: review?(jaws) → review+
Assignee | ||
Comment 17•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8950018 [details] Bug 1418605 - The toolbar, toolbar_text, toolbar_field, toolbar_field_text, toolbar_field_border properties should apply to the findbar https://reviewboard.mozilla.org/r/219302/#review225070 > What is the color set to without this rule? color should inherit by default already. It's necessary because .findbar-button defaults to @roundButtonColor@. This is set in the rule at osx/global/findBar.css:65
Comment 18•6 years ago
|
||
mozreview-review |
Comment on attachment 8950018 [details] Bug 1418605 - The toolbar, toolbar_text, toolbar_field, toolbar_field_text, toolbar_field_border properties should apply to the findbar https://reviewboard.mozilla.org/r/219302/#review225856 I don't think we should be making these changes under /toolkit. Can you please move the changes you have in /toolkit to be in their respective files under /browser? ::: browser/themes/osx/browser.css:669 (Diff revision 7) > - background: @scopeBarBackground@; > + background-image: @scopeBarBackgroundImage@; > + background-repeat: @scopeBarBackgroundRepeat@; You can leave scopeBarBackground unchanged now that you've changed to setting these properties in a separate ruleset below. ::: toolkit/components/extensions/test/browser/browser_ext_themes_findbar.js:78 (Diff revision 7) > + const testBorderColor = (element, prop_to_match, message) => { > + info(message); > + Assert.equal(window.getComputedStyle(element).borderTopColor, Connor has a patch in bug 1417880 that will add a similar function to test border colors to head.js, meaning that you won't have to define your own and can just reference the one from head.js in your file. You can rebase on top of his patch to start using it now. ::: toolkit/components/extensions/test/browser/browser_ext_themes_findbar.js:105 (Diff revision 7) > + document.getAnonymousElementByAttribute(gFindBar, "anonid", "find-previous"); > + > + let findbar_next_button = > + document.getAnonymousElementByAttribute(gFindBar, "anonid", "find-next"); > + > + info(`Checking findbar textbox background is set as toolbar field background color`); The backticks (``) define a "template literal". These strings allow you to insert variables without having to use the `+` to concatenate strings. You can read more about them here, https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals If the string isn't referencing any variables then you should just use a double-quoted string instead of the backticks. Please make that change here and below. ::: toolkit/themes/linux/global/findBar.css:43 (Diff revision 7) > > /* Search field */ > > .findbar-textbox { > -moz-appearance: none; > - border: 1px solid ThreeDShadow; > + background-color: var(--url-and-searchbar-background-color, -moz-Field); We should only reference --url-and-searchboar-background-color in a :-moz-lwtheme rule. ::: toolkit/themes/linux/global/findBar.css:47 (Diff revision 7) > -moz-appearance: none; > - border: 1px solid ThreeDShadow; > + background-color: var(--url-and-searchbar-background-color, -moz-Field); > + border: 1px solid; > + border-color: var(--lwt-toolbar-field-border-color, ThreeDShadow); > box-shadow: 0 0 1px 0 ThreeDShadow inset; > + color: var(--url-and-searchbar-color); This should also move to the :-moz-lwtheme ruleset. ::: toolkit/themes/osx/global/findBar.css:9 (Diff revision 7) > - background: @scopeBarBackground@; > + background-image: @scopeBarBackgroundImage@; > + background-repeat: @scopeBarBackgroundRepeat@; These don't need to be split. ::: toolkit/themes/osx/global/findBar.css:83 (Diff revision 7) > margin: 0; > } > > +.findbar-find-next:-moz-lwtheme, > +.findbar-find-previous:-moz-lwtheme { > + background: linear-gradient(rgba(255,255,255,.6) 1px, rgba(255,255,255,.4) 1px, rgba(255,255,255,.1)); This should just specify background-image. ::: toolkit/themes/osx/global/findBar.css:150 (Diff revision 7) > box-shadow: @roundButtonShadow@; > - background: url("chrome://global/skin/icons/search-textbox.svg") -moz-Field no-repeat 5px center; > + background-image: url("chrome://global/skin/icons/search-textbox.svg"); > + background-color: var(--url-and-searchbar-background-color, -moz-Field); > + background-repeat: no-repeat; > + background-position: 5px center; > + color: var(--url-and-searchbar-color); This should have a fallback of -moz-FieldText since your fallback above is -moz-Field. You can read more about the various system colors at https://developer.mozilla.org/en-US/docs/Web/CSS/color_value#Mozilla_System_Color_Extensions ::: toolkit/themes/osx/global/shared.inc:15 (Diff revision 7) > -%define scopeBarBackground linear-gradient(#E8E8E8, #D0D0D0) repeat-x > +%define scopeBarBackgroundImage linear-gradient(#E8E8E8, #D0D0D0) > +%define scopeBarBackgroundRepeat repeat-x These changes can be reverted.
Attachment #8950018 -
Flags: review?(jaws) → review-
Updated•6 years ago
|
Attachment #8950018 -
Flags: review?(dao+bmo)
Comment 19•6 years ago
|
||
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #18) > Comment on attachment 8950018 [details] > Bug 1418605 - The toolbar, toolbar_text, toolbar_field, toolbar_field_text, > toolbar_field_border properties should apply to the findbar > > https://reviewboard.mozilla.org/r/219302/#review225856 > > I don't think we should be making these changes under /toolkit. Why not?
Comment 20•6 years ago
|
||
mozreview-review |
Comment on attachment 8950082 [details] Bug 1418605 - Rename --url-and-searchbar-background-color and --url-and-searchbar-color for clarity as they are used elsewhere https://reviewboard.mozilla.org/r/219352/#review226030 ::: toolkit/modules/LightweightThemeConsumer.jsm:22 (Diff revision 2) > ["--lwt-background-tiling", "backgroundsTiling"], > ["--tab-loading-fill", "tab_loading", "tabbrowser-tabs"], > ["--lwt-tab-text", "tab_text"], > ["--toolbar-bgcolor", "toolbarColor"], > ["--toolbar-color", "toolbar_text"], > - ["--url-and-searchbar-background-color", "toolbar_field"], > + ["--toolbar-field-background-color", "toolbar_field"], These should probably use the --lwt prefix. It also just occurred to me that "input" might be clearer than "field", as the former is established Web jargon. Though changing all property names from field to input might be bit of a hassle :(
Attachment #8950082 -
Flags: review?(dao+bmo)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8950082 -
Attachment is obsolete: true
Attachment #8950082 -
Flags: review?(dao+bmo)
Assignee | ||
Comment 24•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8950018 [details] Bug 1418605 - The toolbar, toolbar_text, toolbar_field, toolbar_field_text, toolbar_field_border properties should apply to the findbar https://reviewboard.mozilla.org/r/219302/#review225856 Why should they not be added under toolkit?
Comment 25•6 years ago
|
||
I was thinking of not making changes to to toolkit because I didn't want the changes to affect Thunderbird but I see now that they are embracing the theming API so you can disregard my earlier request.
Comment 26•6 years ago
|
||
mozreview-review |
Comment on attachment 8950018 [details] Bug 1418605 - The toolbar, toolbar_text, toolbar_field, toolbar_field_text, toolbar_field_border properties should apply to the findbar https://reviewboard.mozilla.org/r/219302/#review228416 Hey Bogdan, I think this all looks really reasonable. Thanks! I think, however, there's some missing work here (specifically the rename of some variables), and other small notes. ::: toolkit/components/extensions/test/browser/browser_ext_themes_findbar.js:4 (Diff revision 9) > +"use strict"; > + > +// This test checks whether applied WebExtension themes that attempt to change > +// the background color of toolbars are applied properly. > "change the background color of toolbars are applied properly" I think we can be more precise here. You're testing that the findbar can be themed like other toolbars. ::: toolkit/components/extensions/test/browser/browser_ext_themes_findbar.js:100 (Diff revision 9) > + Assert.equal(window.getComputedStyle(findbar_prev_button).borderTopColor, > + hexToCSS(TOOLBAR_FIELD_BORDER_COLOR), > + "Findbar previous button top border color should be set as toolbar field border color."); > + Assert.equal(window.getComputedStyle(findbar_prev_button).borderBottomColor, > + hexToCSS(TOOLBAR_FIELD_BORDER_COLOR), > + "Findbar previous button bottom border color should be set as toolbar field border color."); Out of curiosity, why can't we use `testBorderColor` on `findbar_prev_button`? ::: toolkit/themes/linux/global/findBar.css:54 (Diff revision 9) > padding: 5px; > width: 14em; > } > > +.findbar-textbox:-moz-lwtheme { > + background-color: var(--url-and-searchbar-background-color, -moz-Field); I'm confused. Didn't I review a patch from you on this bug earlier that renamed this variable to something clearer (since it's used in more places)? What happened to that work?
Attachment #8950018 -
Flags: review?(mconley)
Comment 27•6 years ago
|
||
mozreview-review |
Comment on attachment 8950018 [details] Bug 1418605 - The toolbar, toolbar_text, toolbar_field, toolbar_field_text, toolbar_field_border properties should apply to the findbar https://reviewboard.mozilla.org/r/219302/#review229634 This is r+ from me once you fix mconley's issues. Pleaes also re-upload your patch for the variable renaming.
Attachment #8950018 -
Flags: review?(jaws) → review+
Reporter | ||
Comment 28•6 years ago
|
||
mozreview-review |
Comment on attachment 8950018 [details] Bug 1418605 - The toolbar, toolbar_text, toolbar_field, toolbar_field_text, toolbar_field_border properties should apply to the findbar https://reviewboard.mozilla.org/r/219302/#review229648 r+ with mconley's comments addressed.
Attachment #8950018 -
Flags: review?(ntim.bugs) → review+
Assignee | ||
Comment 29•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8950018 [details] Bug 1418605 - The toolbar, toolbar_text, toolbar_field, toolbar_field_text, toolbar_field_border properties should apply to the findbar https://reviewboard.mozilla.org/r/219302/#review228416 > Out of curiosity, why can't we use `testBorderColor` on `findbar_prev_button`? You're absolutely correct, we can use `testBorderColor`. I ended up changing the css from applying the color to the top and bottom borders to all sides, but did not update the test, good catch!
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8950018 -
Attachment is obsolete: true
Attachment #8950018 -
Flags: review?(mconley)
Attachment #8950018 -
Flags: review?(dao+bmo)
Reporter | ||
Comment 32•6 years ago
|
||
mozreview-review |
Comment on attachment 8954793 [details] Bug 1418605 - The toolbar, toolbar_text, toolbar_field, toolbar_field_text, toolbar_field_border properties should apply to the findbar https://reviewboard.mozilla.org/r/223912/#review229916
Attachment #8954793 -
Flags: review?(ntim.bugs) → review+
Comment hidden (mozreview-request) |
Comment 34•6 years ago
|
||
mozreview-review |
Comment on attachment 8954793 [details] Bug 1418605 - The toolbar, toolbar_text, toolbar_field, toolbar_field_text, toolbar_field_border properties should apply to the findbar https://reviewboard.mozilla.org/r/223912/#review230308 This looks good to me, but I think it needs a rebase. While you're doing that rebase, can you also put back the newline that got removed? Thanks! ::: browser/themes/shared/compacttheme.inc.css (Diff revision 2) > > .tab-icon-sound[soundplaying], > .tab-icon-sound[muted] { > filter: none !important; /* removes drop-shadow filter */ > } > - This newline should go back.
Attachment #8954793 -
Flags: review?(mconley) → review+
Comment hidden (mozreview-request) |
Comment 36•6 years ago
|
||
mozreview-review |
Comment on attachment 8954793 [details] Bug 1418605 - The toolbar, toolbar_text, toolbar_field, toolbar_field_text, toolbar_field_border properties should apply to the findbar https://reviewboard.mozilla.org/r/223912/#review230796 mconley and ntim's reviews here are sufficient.
Attachment #8954793 -
Flags: review?(jaws)
Comment 37•6 years ago
|
||
Dao, I think this is just waiting your review here. Requesting your review since you've done previous reviews on this bug. Feel free to defer to Mike and Tim who have already looked at the patch.
Flags: needinfo?(dao+bmo)
Comment 38•6 years ago
|
||
mozreview-review |
Comment on attachment 8954793 [details] Bug 1418605 - The toolbar, toolbar_text, toolbar_field, toolbar_field_text, toolbar_field_border properties should apply to the findbar https://reviewboard.mozilla.org/r/223912/#review231370 ::: browser/modules/ThemeVariableMap.jsm:16 (Diff revision 3) > ["--tab-loading-fill", "tab_loading", "tabbrowser-tabs"], > ["--lwt-tab-text", "tab_text"], > ["--toolbar-bgcolor", "toolbarColor"], > ["--toolbar-color", "toolbar_text"], > - ["--url-and-searchbar-background-color", "toolbar_field"], > - ["--url-and-searchbar-color", "toolbar_field_text"], > + ["--lwt-toolbar-field-background-color", "toolbar_field"], > + ["--lwt-toolbar-field-color", "toolbar_field_text"], Please move this to LightweightThemeConsumer.jsm. ::: browser/themes/linux/browser.css:456 (Diff revision 3) > } > > +.browserContainer > findbar:-moz-lwtheme { > + background-color: var(--toolbar-bgcolor); > + color: var(--toolbar-color); > +} We should probably do this in findBar.css, otherwise it doesn't really make sense to implement toolbar_field and toolbar_field_text there. ::: toolkit/themes/linux/global/findBar.css:43 (Diff revision 3) > > /* Search field */ > > .findbar-textbox { > -moz-appearance: none; > - border: 1px solid ThreeDShadow; > + background-color: -moz-Field; Why are you setting this here? ::: toolkit/themes/linux/global/findBar.css:45 (Diff revision 3) > > .findbar-textbox { > -moz-appearance: none; > - border: 1px solid ThreeDShadow; > + background-color: -moz-Field; > + border: 1px solid; > + border-color: var(--lwt-toolbar-field-border-color, ThreeDShadow); Please fold this into the border shorthand. ::: toolkit/themes/linux/global/findBar.css:47 (Diff revision 3) > -moz-appearance: none; > - border: 1px solid ThreeDShadow; > + background-color: -moz-Field; > + border: 1px solid; > + border-color: var(--lwt-toolbar-field-border-color, ThreeDShadow); > box-shadow: 0 0 1px 0 ThreeDShadow inset; > + color: -moz-FieldText; Why are you setting this here? ::: toolkit/themes/osx/global/findBar.css:73 (Diff revision 3) > .findbar-entire-word { > -moz-appearance: none; > border-radius: 10000px; > - border: @roundButtonBorder@; > + border-width: @roundButtonBorderWidth@; > + border-style: @roundButtonBorderStyle@; > + border-color: @roundButtonBorderColor@; Please use the border shorthand. ::: toolkit/themes/osx/global/findBar.css:142 (Diff revision 3) > .findbar-textbox { > position: relative; > -moz-appearance: none; > - border: @roundButtonBorder@; > + border-width: @roundButtonBorderWidth@; > + border-style: @roundButtonBorderStyle@; > + border-color: var(--lwt-toolbar-field-border-color, @roundButtonBorderColor@); Please use the border shorthand. ::: toolkit/themes/osx/global/findBar.css:148 (Diff revision 3) > border-radius: 10000px 0 0 10000px; > box-shadow: @roundButtonShadow@; > - background: url("chrome://global/skin/icons/search-textbox.svg") -moz-Field no-repeat 5px center; > + background-image: url("chrome://global/skin/icons/search-textbox.svg"); > + background-color: -moz-Field; > + background-repeat: no-repeat; > + background-position: 5px center; Please revert this.
Attachment #8954793 -
Flags: review?(dao+bmo) → review-
Updated•6 years ago
|
Flags: needinfo?(dao+bmo)
Assignee | ||
Comment 39•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8954793 [details] Bug 1418605 - The toolbar, toolbar_text, toolbar_field, toolbar_field_text, toolbar_field_border properties should apply to the findbar https://reviewboard.mozilla.org/r/223912/#review231370 > Please move this to LightweightThemeConsumer.jsm. The variable map was moved from LightweightThemeConsumer.jsm. I had to add it here after rebasing. I'm not sure where you want it in LightweightThemeConsumer.jsm > Please fold this into the border shorthand. With the shorthand you can't add CSS Variables. Jared told me to expand the property. > Please use the border shorthand. See comment above > Please revert this. THe background property was expanded so that the background-color would set properly on line 155.
Comment 40•6 years ago
|
||
(In reply to Bogdan Pozderca [:bogdan] from comment #39) > Comment on attachment 8954793 [details] > Bug 1418605 - The toolbar, toolbar_text, toolbar_field, toolbar_field_text, > toolbar_field_border properties should apply to the findbar > > https://reviewboard.mozilla.org/r/223912/#review231370 > > > Please move this to LightweightThemeConsumer.jsm. There is a `toolkitVariableMap` in LightweightThemeConsumer.jsm that is used by all toolkit applications. This is where it can be added, since these variables are used by /toolkit. > The variable map was moved from LightweightThemeConsumer.jsm. I had to add > it here after rebasing. I'm not sure where you want it in > LightweightThemeConsumer.jsm > > > Please fold this into the border shorthand. > > With the shorthand you can't add CSS Variables. Jared told me to expand the > property. The macro that was being used for substitution needed to be broken up to allow for it to be used with CSS variables. But the split-up version can still be used in border shorthand. For example, `border: @sampleBorderWidth@ @sampleBorderStyle@ var(--lwt-sample-border-color, @sampleBorderColor@);`
Assignee | ||
Comment 41•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8954793 [details] Bug 1418605 - The toolbar, toolbar_text, toolbar_field, toolbar_field_text, toolbar_field_border properties should apply to the findbar https://reviewboard.mozilla.org/r/223912/#review231370 > We should probably do this in findBar.css, otherwise it doesn't really make sense to implement toolbar_field and toolbar_field_text there. I'd have to move all of `.browserContainer > findbar` as it's more specific that just `findbar`. Adding the properties from `.browserContainer > findbar:-moz-lwtheme` to `findbar:-moz-lwtheme` in findbar.css will probably not work.
Comment hidden (mozreview-request) |
Comment 43•6 years ago
|
||
mozreview-review |
Comment on attachment 8954793 [details] Bug 1418605 - The toolbar, toolbar_text, toolbar_field, toolbar_field_text, toolbar_field_border properties should apply to the findbar https://reviewboard.mozilla.org/r/223912/#review233752 ::: toolkit/themes/osx/global/global.css:226 (Diff revision 4) > padding: 1px 10px; > min-width: 60px; > min-height: 16px; > -moz-appearance: none; > border-radius: 10000px; > - border: @roundButtonBorder@; > + border-width: @roundButtonBorderWidth@; > border: @roundButtonBorderWidth@ @roundButtonBorderStyle@ @roundButtonBorderColor@;
Attachment #8954793 -
Flags: review?(jaws) → review+
Assignee | ||
Comment 44•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8954793 [details] Bug 1418605 - The toolbar, toolbar_text, toolbar_field, toolbar_field_text, toolbar_field_border properties should apply to the findbar https://reviewboard.mozilla.org/r/223912/#review233752 > > border: @roundButtonBorderWidth@ @roundButtonBorderStyle@ @roundButtonBorderColor@; good catch!
Comment hidden (mozreview-request) |
Comment 46•6 years ago
|
||
mozreview-review |
Comment on attachment 8954793 [details] Bug 1418605 - The toolbar, toolbar_text, toolbar_field, toolbar_field_text, toolbar_field_border properties should apply to the findbar https://reviewboard.mozilla.org/r/223912/#review233938 Hmm, I just tested this for the first time with the Dark theme and it looks pretty broken (on its own as well as compared to Nightly without this patch)...
Attachment #8954793 -
Flags: review?(dao+bmo) → review-
Comment 47•6 years ago
|
||
(In reply to Bogdan Pozderca [:bogdan] from comment #41) > Comment on attachment 8954793 [details] > Bug 1418605 - The toolbar, toolbar_text, toolbar_field, toolbar_field_text, > toolbar_field_border properties should apply to the findbar > > https://reviewboard.mozilla.org/r/223912/#review231370 > > > We should probably do this in findBar.css, otherwise it doesn't really make sense to implement toolbar_field and toolbar_field_text there. > > I'd have to move all of `.browserContainer > findbar` as it's more specific > that just `findbar`. Adding the properties from `.browserContainer > > findbar:-moz-lwtheme` to `findbar:-moz-lwtheme` in findbar.css will probably > not work. Could you still put it in findBar.css and let browser.css just set it a second time for .browserContainer > findbar:-moz-lwtheme?
Assignee | ||
Comment 48•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8954793 [details] Bug 1418605 - The toolbar, toolbar_text, toolbar_field, toolbar_field_text, toolbar_field_border properties should apply to the findbar https://reviewboard.mozilla.org/r/223912/#review233938 Could you be more specific. What is broken about it?
Comment 49•6 years ago
|
||
(In reply to Bogdan Pozderca [:bogdan] from comment #48) > Comment on attachment 8954793 [details] > Bug 1418605 - The toolbar, toolbar_text, toolbar_field, toolbar_field_text, > toolbar_field_border properties should apply to the findbar > > https://reviewboard.mozilla.org/r/223912/#review233938 > > Could you be more specific. What is broken about it? The buttons looks disabled and the textbox looks at bit weird too although not as broken.
Comment hidden (mozreview-request) |
Reporter | ||
Comment 51•6 years ago
|
||
mozreview-review |
Comment on attachment 8954793 [details] Bug 1418605 - The toolbar, toolbar_text, toolbar_field, toolbar_field_text, toolbar_field_border properties should apply to the findbar https://reviewboard.mozilla.org/r/223912/#review235926 ::: browser/themes/linux/browser.css:455 (Diff revisions 5 - 6) > text-shadow: none; > } > > .browserContainer > findbar:-moz-lwtheme { > background-color: var(--toolbar-bgcolor); > - color: var(--toolbar-color); > + color: var(--toolbar-color, var(--chrome-color, -moz-DialogText)); Can you just specify a value for --toolbar-color in compacttheme.inc.css rather than using --chrome-color here ?
Attachment #8954793 -
Flags: review+ → review-
Reporter | ||
Updated•6 years ago
|
Priority: P5 → P2
Comment hidden (mozreview-request) |
Reporter | ||
Comment 53•6 years ago
|
||
mozreview-review |
Comment on attachment 8954793 [details] Bug 1418605 - The toolbar, toolbar_text, toolbar_field, toolbar_field_text, toolbar_field_border properties should apply to the findbar https://reviewboard.mozilla.org/r/223912/#review238912 ::: browser/themes/shared/compacttheme.inc.css:83 (Diff revision 7) > /* Default findbar text color doesn't look good - Bug 1125677 */ > .browserContainer > findbar .findbar-find-status, > .browserContainer > findbar .found-matches, > .browserContainer > findbar .findbar-button { > - color: inherit; > + color: var(--chrome-color, inherit); > } Could you remove this block altogether, and just define `--toolbar-color` in `:root` above ? :root { ... --toolbar-color: var(--chrome-color); }
Attachment #8954793 -
Flags: review?(ntim.bugs)
Comment hidden (mozreview-request) |
Reporter | ||
Comment 55•6 years ago
|
||
It would be nice if you could fix the low-visibility of the disabled arrows. Let me know if you need help doing so.
Reporter | ||
Comment 56•6 years ago
|
||
mozreview-review |
Comment on attachment 8954793 [details] Bug 1418605 - The toolbar, toolbar_text, toolbar_field, toolbar_field_text, toolbar_field_border properties should apply to the findbar https://reviewboard.mozilla.org/r/223912/#review238928 Looks mostly fine to me! It would be nice if you could address the icon color as well. ::: browser/themes/shared/compacttheme.inc.css:9 (Diff revision 8) > +:root { > + --toolbar-color: var(--chrome-color); > +} > + > :root:-moz-lwtheme { > --toolbar-bgcolor: var(--chrome-secondary-background-color); Sorry I was ambigous, could you use `:root:-moz-lwtheme {` and hence combine both rules ? Those selectors are equivalent in this context, but there's no reason to use different selectors here.
Attachment #8954793 -
Flags: review?(ntim.bugs) → review+
Assignee | ||
Comment 57•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8954793 [details] Bug 1418605 - The toolbar, toolbar_text, toolbar_field, toolbar_field_text, toolbar_field_border properties should apply to the findbar https://reviewboard.mozilla.org/r/223912/#review238928 I am not sure what you mean by this.
Comment hidden (mozreview-request) |
Comment 59•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 5c5c9a66e331a69e2787655dd6543521edf72af7 -d 951167bd0647: rebasing 457935:5c5c9a66e331 "Bug 1418605 - The toolbar, toolbar_text, toolbar_field, toolbar_field_text, toolbar_field_border properties should apply to the findbar r=jaws,mconley,ntim" (tip) merging browser/themes/shared/compacttheme.inc.css merging browser/themes/shared/urlbar-searchbar.inc.css merging toolkit/components/extensions/test/browser/browser.ini merging toolkit/modules/LightweightThemeConsumer.jsm warning: conflicts while merging toolkit/components/extensions/test/browser/browser.ini! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request) |
Comment 61•6 years ago
|
||
Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d625d007d124 The toolbar, toolbar_text, toolbar_field, toolbar_field_text, toolbar_field_border properties should apply to the findbar r=jaws,mconley,ntim
Comment 62•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d625d007d124
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Reporter | ||
Updated•6 years ago
|
Keywords: dev-doc-needed
Comment 63•6 years ago
|
||
Updated https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/theme to say that these 5 properties also affect the Find bar, and https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/theme#Browser_compatibility to note that this is only implemented from 61 onwards. Please let me know if you need anything else here.
Flags: needinfo?(jaws)
Reporter | ||
Comment 64•6 years ago
|
||
(In reply to Will Bamberg [:wbamberg] from comment #63) > Updated > https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/ > theme to say that these 5 properties also affect the Find bar, and > https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/ > theme#Browser_compatibility to note that this is only implemented from 61 > onwards. > > Please let me know if you need anything else here. Looks pretty good, thanks! One thing I would add is showing the findbar in the relevant screenshot.
Comment 65•6 years ago
|
||
That's a good suggestion, :ntim. I've updated those screenshots.
Comment 66•6 years ago
|
||
Looks good, thanks for adding the findbar in to the screenshots!
Flags: needinfo?(jaws)
Updated•6 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Reporter | ||
Updated•6 years ago
|
Attachment #8954793 -
Flags: review?(dao+bmo)
Updated•6 years ago
|
Product: Toolkit → WebExtensions
Comment 67•6 years ago
|
||
Just in case it might help get this fixed... When you test the Findbar, be sure to try the background visibility for the selected buttons "Highlight All", etc. (which is broken in many lightweight themes), AND the visibility of the "# of # matches" report (which was just broken by V61).
Reporter | ||
Updated•5 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•