1262829 - Device list UI follow ups

Status: RESOLVED FIXED

(DevTools :: Responsive Design Mode, defect, P1)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Several UI follow ups for the device list were discussed in bug 1241714 by QA and UX:

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?)) from comment #43)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #42)
> > (In reply to Mihai Boldan, QA [:mboldan] from comment #41)
> > > I managed to test this RDM setting and found a few potential issues:
> > > - the arrows are not visible when the button is unselected - they should be
> > > darker
> > > - nothing happens when hoovering the mouse over the button - the button
> > > should be highlighted when hoovering the mouse over it
> > > - after selecting a device, the button remains highlighted - after selecting
> > > a device and changing the focus from the device selection button, in my
> > > opinion, the button should be gray again, since is not selected and
> > > highlighted when hoovering the cursor over it or when it's selected.
> I agree with most of these points:
> 
> - We definitely need a hover state. Let's go with #999797,
> var(----theme-body-color-inactive). This should affect the 'device list'
> string and the arrows.
> - When someone clicks out of the select list, the :active state doesn't seem
> like it's being removed, which is odd. Let's remove it.
> 
> The dark gray persisting after you've chosen a device was intentional; makes
> it more obvious that the viewport has particular (device-) settings applied
> to it, so let's leave that as-is.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Another style issue to consider:

* With the dark theme, the device selector <option>s are nearly full white, which feels too strong to me

Comment 2

[Jaideep Bhoosreddy [:jbhoosreddy]]

Attachment: 1262829.patch

I did not change the dark theme selected color as I could not choose a good color for that state. But I could do it in the next patch.

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8756435 [details] [diff] [review]
1262829.patch

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

As for what color to use for device selector <option>s in the dark theme, let's ask Helen by setting a needinfo? here.

It seems we should also double-check what do with the base color.

At the moment, the hover color for the <select> is also affecting the color of the options inside it, since color is an inherited CSS property.  Is that desired?  If you open the device selector, and then move the mouse in and out of the list, it appears to "flash" oddly due to the color change of the <option> list on hover.

It looks like this point from comment 1 is still unaddressed: "When someone clicks out of the select list, the :active state doesn't seem like it's being removed, which is odd. Let's remove it."

::: devtools/client/responsive.html/index.css
@@ +180,5 @@
>    background-position: 136px;
>    background-repeat: no-repeat;
>    background-size: 7px;
>    border: none;
> +  color: var(--viewport-color);

Why are we changing this color?  It seems to make the device selector harder to read when no device is selected.

I guess the issue is we were _already_ using #999797 for the base case, so you needed to change this if we are going to use #999797 for hover?

@@ +202,5 @@
>  }
>  
> +.viewport-device-selector:hover {
> +  background-image: var(--viewport-selection-arrow-hovered);
> +  /* Remove the outline from the select box */

Please remove these various commented out bits, no need to have them if they don't apply here.

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Helen, it looks like we have several details that need some more specifics here:

* It would be nice to have a better color to use for device selector <option>s in the dark theme.  It's currently full white, which seems too strong.  Any suggestions on what to use instead?

* You previously (see comment 0) suggested using #999797 / var(----theme-body-color-inactive) as the color for the device selector on hover.  However, that was already the color of the device selector in the base case (without hover).  What should we use as the base color if we change hover to that?

* In the current patch, the hover color for the <select> is also affecting the color of the options inside it, since color is an inherited CSS property.  What is the desired effect on hover for the <option>s inside the device selector?  Should their color change just like the outer <select> or should they be left alone?

Comment 5

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

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #4)
> Helen, it looks like we have several details that need some more specifics
> here:
> 
> * It would be nice to have a better color to use for device selector
> <option>s in the dark theme.  It's currently full white, which seems too
> strong.  Any suggestions on what to use instead?
I'd suggest var(--theme-splitter-color) or var(--theme-body-color).

> * You previously (see comment 0) suggested using #999797 /
> var(----theme-body-color-inactive) as the color for the device selector on
> hover.  However, that was already the color of the device selector in the
> base case (without hover).  What should we use as the base color if we
> change hover to that?
I believe that's because we increased contrast and bumped up body copy to what was the hover state.

The hover state should be darker on hover in light theme, lighter on hover in dark theme (improving contrast in each theme). I'll attach some suggestions for states in a moment. (Not all of them map to colors currently in our variables file...)

> * In the current patch, the hover color for the <select> is also affecting
> the color of the options inside it, since color is an inherited CSS
> property.  What is the desired effect on hover for the <option>s inside the
> device selector?  Should their color change just like the outer <select> or
> should they be left alone?
This is ultimately a <select> box, so we should look to <select> boxes for the rules on this.

Seems like on hover they just an active background with white text—I would imagine we could probably continue using the default blue with white body copy for this.

Comment 6

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

Attachment: rdm-states.png

Comment 7

[Jaideep Bhoosreddy [:jbhoosreddy]]

Attachment: 1262829.patch [2.0]

Helen, if the rdm-states.png file, for light regular state you have mentioned #99979 which I am assuming was #999797 as before. Please let me know if that is not correct.

Ryan, In this patch, I've added the new color scheme as given by Helen. I've also added something so it would remove the selected class. I'm still trying to prevent the color from propagating to it's child nodes. Let me know if you have any suggestions for that.
I'll remove all those the comments from the final patch.

Comment 8

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

(In reply to Jaideep Bhoosreddy [:jbhoosreddy] from comment #7)
> Created attachment 8758429 [details] [diff] [review]
> 1262829.patch [2.0]
> 
> Helen, if the rdm-states.png file, for light regular state you have
> mentioned #99979 which I am assuming was #999797 as before. Please let me
> know if that is not correct.
Yep, sorry for the mistake!

Comment 9

[Jaideep Bhoosreddy [:jbhoosreddy]]

Attachment: 1262829.patch [3.0]

Hey Ryan. I am uploading a new patch as per our discussion on IRC. Let me know what you think. I'll make further changes based on your comments.

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8758501 [details] [diff] [review]
1262829.patch [3.0]

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

Great, this is looking good to me, I think we're on the right track.

A few small tweaks to make, so one more round of review I think.

::: devtools/client/responsive.html/images/select-arrow.svg
@@ +13,5 @@
>        #light-selected {
> +        fill: #3b3b3b;
> +      }
> +      #light-hovered {
> +        fill: #393f4c;

Keep the comment next to this one, since it's derived from the variable.

::: devtools/client/responsive.html/index.css
@@ +9,5 @@
>    --rdm-box-shadow: 0 4px 4px 0 rgba(155, 155, 155, 0.26);
>    --submit-button-active-background-color: rgba(0,0,0,0.12);
>    --submit-button-active-color: var(--theme-body-color);
> +  --viewport-color: #999797;
> +  --viewport-inactive-color: var(--theme-body-color);

Let's call this `--viewport-hover-color` since that's what we're using it for.

@@ +198,5 @@
>    background-image: var(--viewport-selection-arrow-selected);
> +  color: var(--viewport-active-color);
> +}
> +
> +.viewport-device-selector:hover {

When the device list is open and you are hovering on the list, this :hover style is taking precedence, making the <select> use a different color than the <option>s inside.

Since the :focus style is the "stronger" one of the two, I think we want that applied instead.  Move this rule block above the :focus one, so the the :focus one takes over.

This is issue is more obvious with the dark theme colors.

@@ +208,1 @@
>    color: var(--viewport-active-color);

This line would then be gone from here because of the combined rule set below.

@@ +209,5 @@
>    text-align: left;
>    padding: 5px 10px;
>  }
>  
> +.viewport-device-selector > option:hover {

Let's use a single rule set for this and combine the selector, as in:

.viewport-device-selector > option,
.viewport-device-selector > option:hover,
.viewport-device-selector:hover > option:hover {
  color: ...
}

Comment 11

[Jaideep Bhoosreddy [:jbhoosreddy]]

Attachment: 1262829.patch [4.0]

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8758799 [details] [diff] [review]
1262829.patch [4.0]

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

Great, this looks good to me!  Thanks for sticking with it even after I changed my mind several times. :)

It should be safe to land without a try run since it's CSS only.

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I don't think we need QA testing of this, since we've only made some subjective color changes.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/585d451b7795
added a hover state to light/dark themes and change color scheme for device selector; r=jryans

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/585d451b7795

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/585d451b7795