Closed Bug 1253195 Opened 4 years ago Closed 4 years ago

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

Categories

(DevTools :: Inspector, defect, P3, trivial)

defect

Tracking

(firefox47 wontfix, firefox50 verified)

VERIFIED FIXED
Firefox 50
Tracking Status
firefox47 --- wontfix
firefox50 --- verified

People

(Reporter: sebo, Assigned: ruturaj, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good first bug][lang=css][lang=html][btpp-backlog])

Attachments

(12 files, 7 obsolete files)

60.95 KB, image/png
Details
40.81 KB, image/png
Details
429 bytes, image/svg+xml
Details
239 bytes, image/svg+xml
Details
113.15 KB, image/png
Details
14.56 KB, image/png
Details
11.44 KB, image/png
Details
173.63 KB, image/png
Details
164.11 KB, image/png
Details
665 bytes, image/svg+xml
Details
34.07 KB, image/png
hholmes
: ui-review+
Details
19.33 KB, patch
Details | Diff | Splinter Review
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
Priority: -- → P3
Whiteboard: [good first bug][lang=css][lang=html] → [good first bug][lang=css][lang=html][btpp-backlog]
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?
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.
Flags: needinfo?(pbrosset)
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.
Flags: needinfo?(pbrosset) → needinfo?(ntim.bugs)
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
Sebastian, is HTML preferred over XUL? The other tabs all use a XUL <textbox> and I was wondering if consistency should be the goal.
(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.
Mentor: ntim.bugs
Flags: needinfo?(ntim.bugs)
(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
Flags: needinfo?(hholmes)
See Also: → 1265759
(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.
Flags: needinfo?(hholmes)
Tim, with the above info, do you think you have enough from me to point Wellington in the right direction?
Flags: needinfo?(ntim.bugs)
(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.
Flags: needinfo?(ntim.bugs)
Attached image 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
(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
Assignee: nobody → ruturaj
Attached patch fix-1253195.patch (obsolete) — Splinter Review
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(-)
Attached image 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 on attachment 8767501 [details] [diff] [review]
fix-1253195.patch

Will review this tomorrow :) Thanks for the quick patch!
Attachment #8767501 - Flags: review?(ntim.bugs)
Attached image search.svg
Attached image filter.svg
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
Attachment #8767501 - Flags: review?(ntim.bugs) → feedback+
(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
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.
(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.
(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.
Attached patch fix-1253195-2.patch (obsolete) — Splinter Review
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
Attachment #8767501 - Attachment is obsolete: true
Attached image dark-vs-light.png
I've found another problem. Check the scrollbar size and positioning in dark vs light theme.
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?
Attachment #8767854 - Flags: feedback+
(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 :)
Attached patch fix-1253195-3.patch (obsolete) — Splinter Review
Made the changes as suggested.
Attachment #8767854 - Attachment is obsolete: true
The DOM inpsector search font looks better with -moz-use-system-font
(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 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
Attachment #8767994 - Flags: ui-review?(hholmes)
Attachment #8767994 - Flags: review+
Attached image Dark theme screenshot
Helen, here's a screenshot to ease up ui-review
Attached image Light theme screenshot
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;
}
Adding Tim's CSS suggestions for XUL textinput placeholder to have normal font-style in Windows
Attachment #8767994 - Attachment is obsolete: true
Attachment #8767994 - Flags: ui-review?(hholmes)
Attachment #8768353 - Flags: ui-review?(hholmes)
Attached patch fix-1253195-5.patch (obsolete) — Splinter Review
The devtools/client/shared/widgets/VariablesView.jsm was left out, which does a "filter" operation. Adding required CSS class for the same.
Attachment #8768353 - Attachment is obsolete: true
Attachment #8768353 - Flags: ui-review?(hholmes)
Comment on attachment 8768657 [details] [diff] [review]
fix-1253195-5.patch

Sounds fine, thanks for catching that!
Attachment #8768657 - Flags: ui-review?(hholmes)
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?
Attachment #8768657 - Flags: ui-review?(hholmes) → feedback+
Attached image new-filter-icon.png
With the new icon, it looks like this.
Attachment #8768794 - Flags: ui-review?(hholmes)
Attachment #8768794 - Flags: ui-review?(hholmes) → ui-review+
Ruturaj, can you update the patch to use the new icon? or if you'd like me to do it.
Attached patch fix-1253195-6.patch (obsolete) — Splinter Review
- Added the new filter.svg logo by Helen
- Changed the fill color to #aaaaaa to suit both dark and light themes.
Attachment #8768657 - Attachment is obsolete: true
Hey Tim / Helen, What about the comment#29 ?
(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 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)
Attached patch fix-1253195-7.patch (obsolete) — Splinter Review
- 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
Attachment #8769042 - Attachment is obsolete: true
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;
Attachment #8769301 - Flags: review+
- Changing devtools/css/toolbar.css font-family to use message-box instead of specific named fonts.
Attachment #8769301 - Attachment is obsolete: true
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/25eb09fd2fe7
Separate filter and search boxes visually, and make the text styling consistent. r=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.
Hey Tim, thanks a lot !!!

Sure, u can suggest me some more bugs to work on :)
Depends on: 1285747
(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
https://hg.mozilla.org/mozilla-central/rev/25eb09fd2fe7
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
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]
Status: RESOLVED → VERIFIED
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
@Tim, I'm kinda lost here.. 
Has the latest nightly - 47.0a1 got the fix ? or would it come in 50 ?
Flags: needinfo?(ntim.bugs)
(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.
Flags: needinfo?(ntim.bugs)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.