1253195 - Change text style in filter field to look like in other fields

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P3)

Description

[Sebastian Zartner [:sebo]]

The font within the filter field of the Rules side panel and the Computed side panel is monospaced, while in other filter fields the system font is used and it is displayed in italics.

To have a unified layout the font in those two fields should be adjusted accordingly.

Sebastian

Comment 1

[Arjun Jain]

Hi, I'm looking to pickup this bug, do you have any suggestions for where I can look to see the style that should be imitated? Also, is this referring to the font when the filter field is left blank with the default message "Filter Styles" or when the user types into the box?

Comment 2

[Wellington Cordeiro [:wldcordeiro]]

Looks like no one touched this since the comment above some months ago. I'll take a look.

Could you point me to the files that are relevant or the dirs? I'm not exactly sure where to look.

Comment 3

[Patrick Brosset <:pbro>]

Thanks for your interest. I'm going to forward this question to Tim who knows more than me on where the CSS for this is defined, and which theme styling we should be using for this.

Comment 4

[Sebastian Zartner [:sebo]]

Looks like one difference to the other search fields is that it's an HTML <input>[1] and not a XUL <textbox>, which requires different styling.
I think you need to add a .devtools-searchinput::-moz-placeholder rule to toolbars.css[2] to style the placeholder accordingly. Tim, is that the right approach?

Sebastian

[1] http://mxr.mozilla.org/mozilla-central/source/devtools/client/inspector/inspector.xul#215
[2] http://mxr.mozilla.org/mozilla-central/source/devtools/client/themes/toolbars.css

Comment 5

[Wellington Cordeiro [:wldcordeiro]]

Sebastian, is HTML preferred over XUL? The other tabs all use a XUL <textbox> and I was wondering if consistency should be the goal.

Comment 6

[Tim Nguyen :ntim]

(In reply to Sebastian Zartner [:sebo] from comment #4)
> Looks like one difference to the other search fields is that it's an HTML
> <input>[1] and not a XUL <textbox>, which requires different styling.
> I think you need to add a .devtools-searchinput::-moz-placeholder rule to
> toolbars.css[2] to style the placeholder accordingly. Tim, is that the right
> approach?
> 
> Sebastian
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/devtools/client/inspector/
> inspector.xul#215
> [2]
> http://mxr.mozilla.org/mozilla-central/source/devtools/client/themes/
> toolbars.css
This is slightly more complicated. The placeholder is different across various platforms (italic on Windows, no-italic on OSX)... But it is more or less the approach to take (depending on which style we actually want).
It would be possible to add a .devtools-searchinput::-moz-placeholder rule as you said, but it would get unnecessarily complicated with the platform specific styles. 
Ideally, I'd like to move to one common style for all inputs, and all platforms. I'll defer to Helen's UX expertise for this.

Helen, we currently have various searchbox styles across the toolbox. For example, we use the platform font for some searchboxes (inspector) and the monospace font for some others (inspector sidebar). Also, we use a italic text placeholder on Windows for XUL textboxes, whereas we use normal text everywhere else.
It would be nice to move to one common style for all search inputs across all platforms. What do you think ?

(In reply to Wellington Cordeiro from comment #5)
> Sebastian, is HTML preferred over XUL? The other tabs all use a XUL
> <textbox> and I was wondering if consistency should be the goal.
Ideally, we want to use HTML everywhere, but that's out of scope of this bug. This bug is just about making the styling consistent.

Comment 7

[Sebastian Zartner [:sebo]]

(In reply to Tim Nguyen :ntim from comment #6)
> Ideally, I'd like to move to one common style for all inputs, and all
> platforms.

I somewhat agree on that, though I guess there could be a distinction between search fields that search or filter code, (e.g. within the Rules panel) and non-code searches (e.g. like within the Network panel).

> Helen, we currently have various searchbox styles across the toolbox. For
> example, we use the platform font for some searchboxes (inspector) and the
> monospace font for some others (inspector sidebar). Also, we use a italic
> text placeholder on Windows for XUL textboxes, whereas we use normal text
> everywhere else.
> It would be nice to move to one common style for all search inputs across
> all platforms. What do you think ?

You forgot to ni Helen.

> (In reply to Wellington Cordeiro from comment #5)
> > Sebastian, is HTML preferred over XUL? The other tabs all use a XUL
> > <textbox> and I was wondering if consistency should be the goal.
> Ideally, we want to use HTML everywhere, but that's out of scope of this
> bug. This bug is just about making the styling consistent.

For reference, turning all search fields into HTML is tracked in bug 1265759 and overall movement of the DevTools to HTML is tracked in bug 1263750.

Sebastian

Comment 8

[Helen V. Holmes (:helenvholmes) (:✨)]

(In reply to Tim Nguyen :ntim from comment #6)
> Helen, we currently have various searchbox styles across the toolbox. For
> example, we use the platform font for some searchboxes (inspector) and the
> monospace font for some others (inspector sidebar). Also, we use a italic
> text placeholder on Windows for XUL textboxes, whereas we use normal text
> everywhere else.
> It would be nice to move to one common style for all search inputs across
> all platforms. What do you think ?
I 100% agree. I think that stylistically this will mean the sans-serif font family across all platforms, regular (not italic), and the --theme-comment for placeholder, --theme-body-color for user inputted text.


(In reply to Sebastian Zartner [:sebo] from comment #7)
> I somewhat agree on that, though I guess there could be a distinction
> between search fields that search or filter code, (e.g. within the Rules
> panel) and non-code searches (e.g. like within the Network panel).

Sebastian's right—we want to continue to have a distinction between searching and filtering. Stylistically, though, the only difference between these two will be the icon that appears next to them (http://cl.ly/2V1B1I3W3e0c for filtering, http://cl.ly/1x213h2T2q34 for search) and the placeholder string that appears next to the input field. Error states, text styling, etc. will all remain the same.

Comment 9

[Helen V. Holmes (:helenvholmes) (:✨)]

Tim, with the above info, do you think you have enough from me to point Wellington in the right direction?

Comment 10

[Tim Nguyen :ntim]

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #9)
> Tim, with the above info, do you think you have enough from me to point
> Wellington in the right direction?

Yes, thanks!

Wellington, you should be able to modify the `.devtools-searchinput` rules here: http://mxr.mozilla.org/mozilla-central/source/devtools/client/themes/toolbars.css to match Helen's design.

Comment 11

[Ruturaj Vartak]

Attachment: unfound-textbox.png

Hey Guys,

I'm willing to pickup this. I've listed out the following places in devtools where a search vs. filter could be used.

Search
- HTML Inspector
- Debugger
- Style Editor

Filter
- CSS Rules
- Console output
- Network
- DOM (currently has italics)

As well, the existing un-found / unmatched search/filter textbox CSS varies a lot, in certain places absent (ie DOM search). Please check attachment unfound-textbox.png

Comment 12

[Tim Nguyen :ntim]

(In reply to Ruturaj from comment #11)
> Created attachment 8767467 [details]
> unfound-textbox.png
> 
> Hey Guys,
> 
> I'm willing to pickup this. I've listed out the following places in devtools
> where a search vs. filter could be used.
> 
> Search
> - HTML Inspector
> - Debugger
> - Style Editor
> 
> Filter
> - CSS Rules
> - Console output
> - Network
> - DOM (currently has italics)
Thanks for the great overview!

> As well, the existing un-found / unmatched search/filter textbox CSS varies
> a lot, in certain places absent (ie DOM search). Please check attachment
> unfound-textbox.png
Looks like the filter box in the CSS rules view uses the .devtools-style-searchbox-no-match class, while the search box in the HTML inspector uses the .devtools-no-search-result class.
Can you change both to use `.devtools-style-searchbox-no-match` ?

https://dxr.mozilla.org/mozilla-central/search?q=devtools-no-search-result&redirect=false

Comment 13

[Ruturaj Vartak]

Attachment: fix-1253195.patch

Here is the summary

No match UX inconsistency
- I've completely removed the class definition ".devtools-no-search-result"
- replaced ".devtools-no-search-result" with "devtools-style-searchbox-no-match"

Filter vs Magnifier
- I've added 2 new classes "input-filter" and "input-search"
- I've paired the above classes with "devtools-searchinput" as required
- For the Filter image, I've used the existing "images/timeline-filter.svg"

Files modified
 devtools/client/canvasdebugger/canvasdebugger.xul             |  2 +-
 devtools/client/debugger/debugger.xul                         |  2 +-
 devtools/client/dom/content/components/search-box.css         |  4 ++--
 devtools/client/inspector/inspector-search.js                 |  6 +++---
 devtools/client/inspector/inspector.xul                       |  6 +++---
 devtools/client/inspector/test/browser_inspector_search-01.js |  2 +-
 devtools/client/inspector/test/browser_inspector_search-06.js |  2 +-
 devtools/client/memory/components/toolbar.js                  |  2 +-
 devtools/client/netmonitor/netmonitor.xul                     |  2 +-
 devtools/client/shared/widgets/VariablesView.jsm              |  2 +-
 devtools/client/storage/storage.xul                           |  2 +-
 devtools/client/themes/toolbars.css                           | 15 ++++++++++-----
 devtools/client/webconsole/webconsole.xul                     |  2 +-
 13 files changed, 27 insertions(+), 22 deletions(-)

Comment 14

[Ruturaj Vartak]

Attachment: dark-theme.png

Here is the preview of the changes. In the dark theme, the filter logo looks slightly brighter than its magnifying glass counterpart.

Comment 15

[Tim Nguyen :ntim]

Comment on attachment 8767501 [details] [diff] [review]
fix-1253195.patch

Will review this tomorrow :) Thanks for the quick patch!

Comment 16

[Tim Nguyen :ntim]

Attachment: search.svg

Comment 17

[Tim Nguyen :ntim]

Attachment: filter.svg

Comment 18

[Tim Nguyen :ntim]

Comment on attachment 8767501 [details] [diff] [review]
fix-1253195.patch

Review of attachment 8767501 [details] [diff] [review]:
-----------------------------------------------------------------

Most of the changes look good! I have 2 major comments though:

- In line with what we're already doing, I would like all filterboxes to *only* use the `.devtools-filterinput` class, like how all searchboxes currently use `.devtools-searchinput`
- The original bug report was about font inconsistency, so it would be nice to see that fixed as well.
To fix the placeholder style, you can add:
.devtools-searchinput .textbox-input::-moz-placeholder,
.devtools-filterinput .textbox-input::-moz-placeholder {
  font-style: normal;
}

To fix the font inconsistency (monospace vs serif):
You need to remove `font: inherit` here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/toolbars.css#456
and here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/fonts.css#73

You'll also add `font: message-box` here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/toolbars.css#379

::: devtools/client/dom/content/components/search-box.css
@@ +26,4 @@
>  .theme-dark .searchBox,
>  .theme-light .searchBox {
>    border: 1px solid rgb(170, 170, 170);
> +  background-image: url("chrome://devtools/skin/images/timeline-filter.svg");

Instead of this change, can you remove all the `.searchBox` related CSS and set the `.devtools-filterinput` class name here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/dom/content/components/search-box.js#56

Comment 19

[Tim Nguyen :ntim]

(In reply to Ruturaj from comment #14)
> Created attachment 8767502 [details]
> dark-theme.png
> 
> Here is the preview of the changes. In the dark theme, the filter logo looks
> slightly brighter than its magnifying glass counterpart.

I've attached two new icons to the bug: attachment 8767577 [details] (search) and attachment 8767578 [details] (filter). Can you use them? You'll probably need to increase the background-size and the start padding for those to look right. You'll also need to register the new images here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/jar.mn

Comment 20

[Ruturaj Vartak]

While I am at it...

> In line with what we're already doing, I would like all filterboxes to *only* use the `.devtools-filterinput` class, like how all searchboxes currently use `.devtools-searchinput`

I was looking at CSS, wouldn't that duplicate a lot of CSS? hence I thought of adding just input-filter and input-search class and taking away the background image from it. Would a rename of ".devtools-searchinput" to ".devtools-input" or something similar work if it was for semantics ? 

> Instead of this change, can you remove all the `.searchBox` related CSS and set the `.devtools-filterinput` class name here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/dom/content/components/search-box.js#56

Again, while I work on it, this module seems to be written in a completely different manner... Code doesn't look like typical XUL, JS. Looks more "Web-ish".

I'm a mozilla noob, I just want to understand.

Comment 21

[Tim Nguyen :ntim]

(In reply to Ruturaj from comment #20)
> While I am at it...
> 
> > In line with what we're already doing, I would like all filterboxes to *only* use the `.devtools-filterinput` class, like how all searchboxes currently use `.devtools-searchinput`
> 
> I was looking at CSS, wouldn't that duplicate a lot of CSS? hence I thought
> of adding just input-filter and input-search class and taking away the
> background image from it. Would a rename of ".devtools-searchinput" to
> ".devtools-input" or something similar work if it was for semantics ? 
That wouldn't duplicate CSS since it's just a matter of adding new selectors to the current lists.
Like how we share styles between .devtools-textinput and .devtools-searchinput

> > Instead of this change, can you remove all the `.searchBox` related CSS and set the `.devtools-filterinput` class name here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/dom/content/components/search-box.js#56
> 
> Again, while I work on it, this module seems to be written in a completely
> different manner... Code doesn't look like typical XUL, JS. Looks more
> "Web-ish".
Our devtools css is meant to be reused across the devtools (it's like a CSS framework). Reusing existing classes and styles reduces code duplication.

Comment 22

[Tim Nguyen :ntim]

(In reply to Ruturaj from comment #20)
> Would a rename of ".devtools-searchinput" to
> ".devtools-input" or something similar work if it was for semantics ? 
I don't really have a preference, but since the existing CSS already works the way I've described, it's more consistent.

Comment 23

[Ruturaj Vartak]

Attachment: fix-1253195-2.patch

The patch does the following
- Added .devtools-filterinput class for filters, equivalent of .devtools-searchinput
- Computed CSS filter font changed
- Added new filter.svg and search.svg.
- Removed .serachBox class completely for DOM search filter box

Comment 24

[Ruturaj Vartak]

Attachment: dark-vs-light.png

I've found another problem. Check the scrollbar size and positioning in dark vs light theme.

Comment 25

[Tim Nguyen :ntim]

Comment on attachment 8767854 [details] [diff] [review]
fix-1253195-2.patch

Review of attachment 8767854 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good overall! I'll test this later today.

::: devtools/client/dom/content/components/search-box.css
@@ +9,2 @@
>    float: right;
> +  font-size: -moz-use-system-font;

This shouldn't be needed since font: message-box; in toolbars.css should handle that already

::: devtools/client/themes/computed.css
@@ +22,5 @@
>    /* Bug 1200073 - extra space before the browser styles checkbox so
>       they aren't squished together in a small window. */
>    margin-inline-start: 5px;
> +  font: message-box;
> +  font-size: inherit;

Please undo this change, while the searchbox should use message-box, the rest of the panel should stay monospaced.

::: devtools/client/themes/toolbars.css
@@ +43,5 @@
>    --searchbox-border-color: #d99f2b;
>    --searcbox-no-match-background-color: #402325;
>    --searcbox-no-match-border-color: #cc3d3d;
> +  --magnifying-glass-image: url(images/search.svg);
> +  --magnifying-glass-image-2x: url(images/search.svg);

You can remove --magnifying-glass-image-2x entirely

@@ +548,2 @@
>    }
>  }

Can you remove the whole block please?

Comment 26

[Tim Nguyen :ntim]

(In reply to Ruturaj from comment #24)
> Created attachment 8767858 [details]
> dark-vs-light.png
> 
> I've found another problem. Check the scrollbar size and positioning in dark
> vs light theme.

This is unrelated to this bug, don't worry about it :)

Comment 27

[Ruturaj Vartak]

Attachment: fix-1253195-3.patch

Made the changes as suggested.

Comment 28

[Ruturaj Vartak]

Attachment: DOM inpsector search font (with moz use system font)

The DOM inpsector search font looks better with -moz-use-system-font

Comment 29

[Ruturaj Vartak]

Attachment: DOM inpsector search font (without moz use system font)

Comment 30

[Tim Nguyen :ntim]

(In reply to Ruturaj from comment #28)
> Created attachment 8767995 [details]
> DOM inpsector search font (with moz use system font)
> 
> The DOM inpsector search font looks better with -moz-use-system-font
If so, font-size: -moz-use-system-font; should be in toolbars.css, not specific to the dom panel.

Comment 31

[Tim Nguyen :ntim]

Comment on attachment 8767994 [details] [diff] [review]
fix-1253195-3.patch

Review of attachment 8767994 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine! Let's see what Helen thinks

Comment 32

[Tim Nguyen :ntim]

Attachment: Dark theme screenshot

Helen, here's a screenshot to ease up ui-review

Comment 33

[Tim Nguyen :ntim]

Attachment: Light theme screenshot

Comment 34

[Tim Nguyen :ntim]

Comment on attachment 8767994 [details] [diff] [review]
fix-1253195-3.patch

Review of attachment 8767994 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like this doesn't fix the fact that the placeholder text for XUL textboxes is italic on Windows.
This should fix it:
.devtools-searchinput .textbox-input::-moz-placeholder,
.devtools-filterinput .textbox-input::-moz-placeholder {
  font-style: normal;
}

Comment 35

[Ruturaj Vartak]

Attachment: Fixing Windows XUL textbox placeholder font-style to normal

Adding Tim's CSS suggestions for XUL textinput placeholder to have normal font-style in Windows

Comment 36

[Ruturaj Vartak]

Attachment: fix-1253195-5.patch

The devtools/client/shared/widgets/VariablesView.jsm was left out, which does a "filter" operation. Adding required CSS class for the same.

Comment 37

[Tim Nguyen :ntim]

Comment on attachment 8768657 [details] [diff] [review]
fix-1253195-5.patch

Sounds fine, thanks for catching that!

Comment 38

[Helen V. Holmes (:helenvholmes) (:✨)]

Comment on attachment 8768657 [details] [diff] [review]
fix-1253195-5.patch

Thanks for the screenshots, Tim!

I notice that the filter is starting to lose meaning at such a small size. I've been fiddling with a filter icon myself—do you mind if we swap it in and see if it works any better?

Comment 39

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: filter-expanded.svg

Comment 40

[Tim Nguyen :ntim]

Attachment: new-filter-icon.png

With the new icon, it looks like this.

Comment 41

[Tim Nguyen :ntim]

Ruturaj, can you update the patch to use the new icon? or if you'd like me to do it.

Comment 42

[Ruturaj Vartak]

Attachment: fix-1253195-6.patch

- Added the new filter.svg logo by Helen
- Changed the fill color to #aaaaaa to suit both dark and light themes.

Comment 43

[Ruturaj Vartak]

Hey Tim / Helen, What about the comment#29 ?

Comment 44

[Tim Nguyen :ntim]

(In reply to Ruturaj from comment #43)
> Hey Tim / Helen, What about the comment#29 ?

I replied in comment 30. Feel free to add it, but it shouldn't be specific to the DOM panel css file.

Comment 45

[Tim Nguyen :ntim]

Comment on attachment 8769042 [details] [diff] [review]
fix-1253195-6.patch

Review of attachment 8769042 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/themes/images/filter.svg
@@ +1,4 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16" fill="#aaaaaa">

This should have opacity="0.5" to match the other icon (or you can remove the opacity from the other icon)

Comment 46

[Ruturaj Vartak]

Attachment: fix-1253195-7.patch

- removing opacity from search.svg (for consistency with new filter.svg)
- in accordance to comment#30 , adding font-size: larger to toolbar.css (-moz-use-system-font) was little too large

Comment 47

[Tim Nguyen :ntim]

Comment on attachment 8769301 [details] [diff] [review]
fix-1253195-7.patch

Review of attachment 8769301 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with that changed.

::: devtools/client/jsonview/css/toolbar.css
@@ +12,2 @@
>    height: 22px;
>    font-family: "Helvetica Neue",Helvetica,Arial,sans-serif;

I'd rather see the font-family and font-size definitions here replaced with font: message-box;

Comment 48

[Ruturaj Vartak]

Attachment: fix-1253195-8.patch

- Changing devtools/css/toolbar.css font-family to use message-box instead of specific named fonts.

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/25eb09fd2fe7
Separate filter and search boxes visually, and make the text styling consistent. r=ntim

Comment 50

[Tim Nguyen :ntim]

I've merged your patch, it should be in Nightly in 2-3 days.
Thanks for your work! Let me know if you'd like some bug suggestions.

Comment 51

[Ruturaj Vartak]

Hey Tim, thanks a lot !!!

Sure, u can suggest me some more bugs to work on :)

Comment 52

[Tim Nguyen :ntim]

(In reply to Ruturaj Vartak from comment #51)
> Sure, u can suggest me some more bugs to work on :)

Here are some good candidates:
Bug 1285747
Bug 1224491
Bug 1253324
Bug 1205561
Bug 1253330

Comment 53

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/25eb09fd2fe7

Comment 54

[Md. Rahimul Islam]

I have successfully reproduce this bug on firefox nightly 47.0a1 (2016-03-03)
with windows 7 (32 bit)
Mozilla/5.0 (Windows NT 6.1; rv:47.0) Gecko/20100101 Firefox/47.0

I found this fix on latest nightly 50.0a1 (2016-07-25)

Mozilla/5.0 (Windows NT 6.1; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID : 20160725030248

[bugday-20160727]

Comment 55

[Rezaul Huque Nayeem]

Reproduced this bug in firefox nightly 47.0a1 (2016-03-03) with ubuntu 16.04 (64 bit)

Verified as this bug as fixed with latest firefox nightly 50.0a1 (Build ID: 20160726030213)
Mozilla/5.0 (X11; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0

Comment 56

[Ruturaj Vartak]

@Tim, I'm kinda lost here.. 
Has the latest nightly - 47.0a1 got the fix ? or would it come in 50 ?

Comment 57

[Tim Nguyen :ntim]

(In reply to Ruturaj Vartak from comment #56)
> @Tim, I'm kinda lost here.. 
> Has the latest nightly - 47.0a1 got the fix ? or would it come in 50 ?

It's in 50. Comments 55 and 54 only say that this bug isn't fixed in 47, but it is fixed in 50.