The toolbar, toolbar_text, toolbar_field, toolbar_field_text, toolbar_field_border properties should apply to the findbar

RESOLVED FIXED in Firefox 61

Status

enhancement
P2
normal
RESOLVED FIXED
2 years ago
a month ago

People

(Reporter: ntim, Assigned: bogdan, Mentored)

Tracking

(Blocks 1 bug, {dev-doc-complete})

unspecified
mozilla61
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Reporter

Description

2 years ago
No description provided.

Updated

2 years ago
Priority: -- → P5
Assignee: nobody → masinico
Mentor: mconley, jaws, ntim.bugs
Actually, I think bug 1417880 is higher priority to have Connor work on. Sorry for the churn!
Assignee: masinico → nobody
Assignee: nobody → bogdan
Status: NEW → ASSIGNED
Reporter

Updated

a year 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
Component: WebExtensions: Frontend → WebExtensions: Themes
Comment hidden (mozreview-request)
Reporter

Comment 4

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

a year 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+
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

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

a year 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 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-
(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

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

a year ago
Attachment #8950082 - Attachment is obsolete: true
Attachment #8950082 - Flags: review?(dao+bmo)
Assignee

Comment 24

a year 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?
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

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

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

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

a year ago
Attachment #8950018 - Attachment is obsolete: true
Attachment #8950018 - Flags: review?(mconley)
Attachment #8950018 - Flags: review?(dao+bmo)
Reporter

Comment 32

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

a year 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 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)
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

a year 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-
Flags: needinfo?(dao+bmo)
Assignee

Comment 39

a year 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.
(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

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

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

a year 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-
(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

a year 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?
(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

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

a year ago
Priority: P5 → P2
Comment hidden (mozreview-request)
Reporter

Comment 53

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

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

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

a year 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)
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

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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d625d007d124
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Reporter

Updated

a year ago
Depends on: 1455922
Reporter

Updated

a year ago
Keywords: dev-doc-needed
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

a year 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.
That's a good suggestion, :ntim. I've updated those screenshots.
Looks good, thanks for adding the findbar in to the screenshots!
Flags: needinfo?(jaws)
Reporter

Updated

a year ago
Attachment #8954793 - Flags: review?(dao+bmo)

Updated

11 months ago
Product: Toolkit → WebExtensions

Updated

11 months ago
Depends on: 1471705

Comment 67

11 months 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).

Updated

4 months ago
Depends on: 1525476

Updated

2 months ago
Depends on: 1506913
Reporter

Updated

a month ago
No longer depends on: 1506913
Regressions: 1506913
You need to log in before you can comment on or make changes to this bug.