Closed Bug 1438458 Opened 6 years ago Closed 6 years ago

Autocomplete items selected and hover have the same style

Categories

(DevTools :: Shared Components, defect)

Product:
DevTools
Component:
Shared Components
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox60 wontfix, firefox61 wontfix, firefox62 wontfix, firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: nchevobbe, Assigned: arthur.deschamps1208, Mentored)

Details

(Keywords: good-first-bug)

Attachments

(7 files, 4 obsolete files)

Filter autocomplete - confusing state
41.39 KB, image/png
Details
Awesome bar
37.75 KB, image/png
Details
Patch v1
167.31 KB, image/png
Details
Patch v2
153.16 KB, image/png
Details
Bug 1438458 - Differentiate hover and selected colors in the devtools autocomplete popup -
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details
Patch v3
53.67 KB, image/png
Details
Bug 1438458 - Differentiate hover and selected colors in the devtools autocomplete popup -
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details 
**Steps to reproduce**
1. On this page, open the netmonitor
2. Into the filter input, type "r"
3. You should see to items: regexp and remote-ip
4. Hover remote-ip with your mouse
5. Press <kbd>↓</kbd>


**Expected results**

The regexp item has a blue background color, the remote-ip one a grey one (like in the awesome bar in such case).

**Actual results**

Both elements have the same background color, which can confuse the user (See attachment)
Attached image Awesome bar — 
here's the awesome bar style for reference
Moving to Shared component since the issue is on the Autocomplete component.

So here lies the issue: 

https://searchfox.org/mozilla-central/rev/9011be0a172711bc243e50dfca16d42e877bf4ec/devtools/client/themes/common.css#133-141

We set exactly the same color for selected and hovered style, for both light and dark themes.

---

What need to be done:

Basically, we should keep the same rule for the :hover state, and we should create a new one for the selected state.
The background color could be --theme-selection-background and the color --theme-selection-color

Also, we should ensure that hovering a selected element keep the blue background and white color.
Mentor: nchevobbe
Has STR: --- → yes
Component: Developer Tools: Netmonitor → Developer Tools: Shared Components
Keywords: good-first-bug
Hi ! I'm new to Mozilla and would like to make my first contribution(s). Can I try solving this issue ? 

Thanks
Arthur
Flags: needinfo?(nchevobbe)
Hello Arthur , sure you can. Thanks for helping us !
You can have a look at http://docs.firefox-dev.tools/getting-started/ to see how to set up your environment.
Don't hesitate to ask any question.
Flags: needinfo?(nchevobbe)
So it took me the whole day to get the dev environment up and running but it's all good now, though I was wondering about two things: 
1) Is it possible to hot reload changes made to the code ? At least CSS code ? This time I used "./mach build faster && ./mach run" every time to reload my changes, but it's tiresome.
2) Is it possible to "inspect" the developer tools or the browser search bar ? I mean in the same way we would do for a normal HTML page.

I've created a patch for this issue following your instructions, I hope it's alright.
Hello Arthur, thanks for your patch !

> Is it possible to hot reload changes made to the code ? At least CSS code ? This time I used "./mach build faster && ./mach run" every time to reload my changes, but it's tiresome.

You can read https://groups.google.com/forum/#!topic/firefox-dev/_VTCgMO2B3Y , which provides a faster workflow.

> Is it possible to "inspect" the developer tools or the browser search bar ? I mean in the same way we would do for a normal HTML page.

Yes, you can use the browser toolbox for that :https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox . It will give you a devtools window which can inspect the browser UI, including the devtools.
Assignee: nobody → arthur.deschamps1208
Status: NEW → ASSIGNED
Comment on attachment 8951910 [details] [diff] [review]
Patch resolving the color issue when selecting items in the devtools autocomplete bar.

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

Thanks Arthur, this looks good in the Netmonitor.
However, there is a small issue on some other panel that use this component as well (console, inspector, style editor): 
- the color of a selected element is overridden (so we don't have the new white color)
- in some case, the text you entered is in another color, which is not really readable with the patch (see the attachment in the next comment).

So here we need to make sure we do have the selected element in white, and find a lighter gray for the matching text in the autocomplete menu
Attachment #8951910 - Flags: review?(nchevobbe) → review-
Attached image Patch v1 — 

    
    

    
  
Also, the way you posted your patch is totally fine (and a lot of mozillian do that), but I found using mozreview (http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/install.html) much simpler.

    
    

    
  
Thank you for all this information, it's way easier to work now :)

So, about your review, I'm not sure what to do. Do you want the color to be always white when an item is selected ? With no differentiation between "initial-value" and "autocomplete-value" (same white) ?
Also, I'm not sure I understand what you mean in the second remark ?

I appreciate the help.

    
    

    
  
So, we should always have a white color on blue background for an "active element".
I'd say the initial-value should be the "most" white, and "autocomplete-value" should be a very light grey, if that makes sense.

(Another tip in mozilla world, when asking something to someone, use the "Need more information from" checkbox underneath the comment input. That will warn the person that you are waiting on them :) )

    
    

    
  
I've tried to use mozreview this time, hope I did it correctly.

For the autocomplete value, I've chosen "whitesmoke" which seemed to be the most appropriate color.

    
    

    
  
Arthur, I don't see the mozreview here. Do you have an URL for it ?
This should have automatically appeared in the bug, I wonder if there's an issue.
Could you paste here what was prompted in your terminal when you pushed to mozreview ?
Flags: needinfo?(arthur.deschamps1208)

    
    

    
  
Sorry I just realised I hadn't confirmed the "publish these review requests now (Yn)?"...
Flags: needinfo?(arthur.deschamps1208)

    
    

    
  
no worries :)

    
    

    
  
Comment on attachment 8952322 [details]
Bug 1438458 - Change devtools autocomplete selected items background color and font color to what is already in use in the search bar.

https://reviewboard.mozilla.org/r/221576/#review227476

Thanks for the patch on mozreview :)
You can amend commit if you want, in those case where some changes are requested (mozreview keeps track of all the patch you pushed, so we don't lose anything)

So, here --theme-selection-color and smokewhite are a bit too similar, we can't really tell the difference between initial-value and autocomplete-value.
I tried it quickly, and I think we should: 
 - have the default text for selected item --theme-selection-color
 - only override it for .initial-value with --theme-highlight-yellow which looks good to me in both light and dark mode.

Could you try that, ammend your patch and push again to review ? Thanks !

    
    

    
      
  

    
  

        Attachment #8951910 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8952322 [details]
Bug 1438458 - Change devtools autocomplete selected items background color and font color to what is already in use in the search bar.

https://reviewboard.mozilla.org/r/221578/#review227584

This looks good to me , thanks !

2 minor things: 
- could you squash those 2 commits together since they're about the same thing (if you use hg, the histedit or evolve extension make it easy)
- could the commit message be more about what is fixed, like : "Differentiate hover and selected colors in the autocomplete popup" ?

        Attachment #8952322 -
        Flags: review?(nchevobbe) → review+

    
    

    
      
  


  

    
  

        Attachment #8952322 -
        Attachment is obsolete: true

    
  

        Attachment #8952323 -
        Attachment is obsolete: true

        Attachment #8952323 -
        Flags: review?(nchevobbe)

    
    

    
  

      
      
      
      

        Attached image
          
        Patch v2
        — 
      

    


  
Victoria, are you fine with those changes ?

        Attachment #8952538 -
        Flags: ui-review?(victoria)

    
    

    
  
Comment on attachment 8952538 [details]
Patch v2

Wow, this looks great! Thanks so much for working on this Nicolas and Arthur! 

And I can't believe this just got done so quickly :). Similar to my mission to fix all the focus rings! I'm going to pass this along to the debugger team as they have the same problem.

One suggestion (could be for a new patch): for consistency, could we use the same hover background color from the Inspector's hover styling? It's a subtler blue shade.

Also (can also be for a new patch) I'm not sure about the highlighted partial match styling - the debugger is using an outline style for this that I'm also not sure about. Firefox awesome bar just applies bold weight - I wonder if that would be clear enough in these situations?

        Attachment #8952538 -
        Flags: ui-review?(victoria)

    
    

    
  
Thanks for the review Victoria.
Arthur, do you mind trying those changes ?
 - The hover background in the markup is var(--theme-selection-background-hover);
 - Do not override the color for initial-value, but only font-weight

I think it's nice to have consistency around all Firefox uis

    
  
Flags: needinfo?(arthur.deschamps1208)

    
    

    
  
Yes sure, I'll take a look this weekend. Sorry I have less time these days because I'm back in class.
Flags: needinfo?(arthur.deschamps1208)

    
    

    
  
sure, no worries, it's not urgent matter

    
    

    
  
Comment on attachment 8952449 [details]
Bug 1438458 - Differentiate hover and selected colors in the devtools autocomplete popup -

clearing review flag

        Attachment #8952449 -
        Flags: review?(nchevobbe)

    
    

    
      
  


  

    
  

        Attachment #8952449 -
        Attachment is obsolete: true

    
    

    
  
I've uploaded a new version of the patch, with Victoria's suggestions implemented. I hope this is what you had in mind.

    
    

    
  
Comment on attachment 8953776 [details]
Bug 1438458 - Differentiate hover and selected colors in the devtools autocomplete popup -

https://reviewboard.mozilla.org/r/222976/#review228958

OOps, seems like there is some leftovers from a conflict merge.
Could you remove them so I can apply the patch Arthur ?

        Attachment #8953776 -
        Flags: review?(nchevobbe) → review-

    
    

    
      
  

    
    

    
  
Comment on attachment 8953776 [details]
Bug 1438458 - Differentiate hover and selected colors in the devtools autocomplete popup -

https://reviewboard.mozilla.org/r/222976/#review228986

        Attachment #8953776 -
        Flags: review?(nchevobbe) → review+

    
    

    
  

      
      
      
      

        Attached image
          
        Patch v3
        — 
      

    


  
Looks great to me :)
I pushed to TRY (our CI), and if all the test pass, we'll land your patch.
Thanks !

    
  
Product: Firefox → DevTools

    
    

    
      
  

    
    

    
  
Comment on attachment 8988662 [details]
Bug 1438458 - Differentiate hover and selected colors in the devtools autocomplete popup -

https://reviewboard.mozilla.org/r/253890/#review260604

        Attachment #8988662 -
        Flags: review?(nchevobbe) → review+

    
    

    
  
Arthur. I'm very sorry this felt off my radar and didn't land it.
I rebased your patch and pushed to TRY again, and this time, will land it once TRY is green.

    
    

    
  
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fb58e941b632
Differentiate hover and selected colors in the devtools autocomplete popup - r=nchevobbe

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/fb58e941b632
Status: ASSIGNED → RESOLVED
Closed: 6 years ago

          status-firefox63:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: