1421395 - Minor Photon changes to Console

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P3)

Description

[Victoria Wang [:victoria]]

Change the Console focus rings (for both search and input) from a dotted line to a 1px Blue 50, which should be positioned directly within the gray border, like in this mockup: https://mozilla.invisionapp.com/share/2XEEY0RYA#/screens/263398480_Console-Network_-_Input_Field_Focus_Ring

Also, the active input caret color should be var(--theme-toolbar-checked-color); instead of green.

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

When doing this work, maybe take Bug 1395825 into consideration

Comment 2

[Victoria Wang [:victoria]]

Ah good point, and this is all very relevant to the Console toolbar refresh which I'm not sure is in a bug yet, but is slated for Q1? https://mozilla.invisionapp.com/share/2XEEY0RYA#/263400464_Console_With_Options

Comment 3

[Kentaro Teramoto [:zigen]]

Hi there.
I'm looking for my first bug. I would like to take this.
May I work on this?

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Hello Kentaro, 

Of course you can have a look.
If you haven't already, I encourage you to have a look at http://docs.firefox-dev.tools/getting-started/ so you can setup your machine and start contributing.

Don't hesitate to ask any question, either here, or in our slack https://devtools-html-slack.herokuapp.com/ :)

Comment 5

[Kentaro Teramoto [:zigen]]

Attachment: webconsole-filterbar-filterd-messages is too long

I'm trying to this bug, and I put the FilterCheckBox next to input.
When show the logs and filter it, filterbar-filterd-messages appeared: "11 items hidden by filter".

In your mockup, there is no message like this.
Would I remove this message?

Comment 6

[Victoria Wang [:victoria]]

Hi Kentaro, thanks for working on this! 

That mockup was actually from a new redesign that will happen next year, so you can disregard any of the changes that aren't the focus ring. (The new design has some extra settings and responsive design features that are better done all together.) I'm sorry for the confusion!

Here's a better mockup that shows what this bug is about: It's just to change how the field looks on focus, and doesn't change the field besides that. (Adding a little extra left padding to the field, as shown below, would look nice though.)

https://mozilla.invisionapp.com/share/2XEEY0RYA#/screens/268538664_Current_Console_With_Focus_Ring

Let me know if you have any other questions!

Comment 7

[Kentaro Teramoto [:zigen]]

Thanks for replying, Victoria!
The mockup looks very cool!

Now, I can understand what I do in this bug, so I'll try it!

Comment 8

[Kentaro Teramoto [:zigen]]

Attachment: change-focusring-patch

Comment 9

[Kentaro Teramoto [:zigen]]

Attachment: Bug 1421395 - change focus ring in console from gray dotted line to blue, change active input caret color to toolbar-checked-color. r=nchevobbe

Comment 10

[Kentaro Teramoto [:zigen]]

I created patch for this.
In bug 1395825, the submitted patch changes filter input ( .devtools-plaininput ) border too.
What should I do? 

This is my first patch, I don't know which should I use MozReview?

Comment 11

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8936199 [details] [diff] [review]
Bug 1421395 - change focus ring in console from gray dotted line to blue, change active input caret color to toolbar-checked-color. r=nchevobbe

I am gonna steal this review

Comment 12

[Victoria Wang [:victoria]]

Just wanted to mention that this bug somewhat overlaps this: https://bugzilla.mozilla.org/show_bug.cgi?id=1395825

Comment 13

[Kentaro Teramoto [:zigen]]

Sorry for late reply.

(In reply to Victoria Wang [:victoria] from comment #12)
> Just wanted to mention that this bug somewhat overlaps this:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1395825

Both patches apply border/outline to filter input (.devtools-plaininput).

bug 1395825 removes outline and adds "border-color: var(--theme-focus-border-color-textbox);"
This bug just adds "outline: 1px solid var(--blue-50);"

Comment 14

[Kentaro Teramoto [:zigen]]

Attachment: Bug 1421395 - change focus ring in console from gray dotted line to blue, change active input caret color to toolbar-checked-color.

Review commit: https://reviewboard.mozilla.org/r/209594/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/209594/

Comment 15

[Tim Nguyen :ntim]

Comment on attachment 8939194 [details]
Bug 1421395 - change focus ring in console from gray dotted line to blue, change active input caret color to toolbar-checked-color.

https://reviewboard.mozilla.org/r/209594/#review215900

::: devtools/client/themes/webconsole.css:464
(Diff revision 1)
> +  outline: 1px solid var(--blue-50);
> +  outline-offset: -1px;

Please use a 1px transparent border by default, and then change the border-color to blue on focus, rather than using an outline and positioning it as a border.

::: devtools/client/themes/webconsole.css:858
(Diff revision 1)
>    flex: 1 1 100%;
>  }
>  
> +.devtools-plaininput:-moz-focusring {
> +  outline: 1px solid var(--blue-50);
> +  outline-offset: -1px;

Same here.

Comment 16

[[e-mail]]

I guess the focusring style comes from
https://searchfox.org/mozilla-central/source/devtools/client/themes/common.css#214

Maybe its fine to patch the focusring style in common.css directly since UX wants that new focus style applied everywhere.

Comment 17

[Gabriel Luong [:gl] (ΦωΦ)]

The code changes look since it follows what was done in common.css, but we might not need the 1px solid. I would like to ask Victoria if we should only change the focusring style for the webconsole or everything.

Comment 18

[Victoria Wang [:victoria]]

Ah, I see the problem. I'd like all the other inputs to match this rectangle style eventually, but it requires a little more thinking for some of them because of the switch to left-aligned and borderless. For now, those rounded inputs should keep their existing focus rings. Let's just do the 1px Blue50 focus ring for the currently rectangular fields: Console search, Console input. (This is also happening for the in-progress rectangular Network search field, so it may be a good idea to check with them on code sharing.)

Comment 19

[Kentaro Teramoto [:zigen]]

Comment on attachment 8939194 [details]
Bug 1421395 - change focus ring in console from gray dotted line to blue, change active input caret color to toolbar-checked-color.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209594/diff/1-2/

Comment 20

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8939194 [details]
Bug 1421395 - change focus ring in console from gray dotted line to blue, change active input caret color to toolbar-checked-color.

I am gonna pass this review off to bgrins. I am not sure if we want to use the border or outline method for styling the focusring since we use the outline method in common.css

Comment 21

[Brian Grinstead [:bgrins]]

Comment on attachment 8939194 [details]
Bug 1421395 - change focus ring in console from gray dotted line to blue, change active input caret color to toolbar-checked-color.

https://reviewboard.mozilla.org/r/209594/#review217636

Forwarding to Nicolas, since he's also reviewing Bug 1395825 which seems to interact with this

Comment 22

[Kentaro Teramoto [:zigen]]

Thank you for reviewing my patch.
I have no confidence which is better to use border or outline, and also :focus or :-moz-focusring.

Comment 23

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Screencast with patch

Si, I'm not sure about having the border for the input. 
Since we always redirect the focus to the input when we click on the output, the "blink" that occurs feel pretty strong to me (see attachment).

Also, there seems to be a toolbar added with this patch in the input, not sure what is causing this.

For the filter input, it looks good. But it also shows that the input does not take the whole height of the toolbar, and can make it feel a bit too small (see space between input border and toolbar border in http://prntscr.com/hzhp9t). It is already the case today, but the blue border make it more visible in my opinion. The input should take the whole height, and maybe have some padding.

For the technical solution, I'm fine with using border, and if we can :focus rather than :-moz-focusring since it's unprefixed.

Comment 24

[Nicolas Chevobbe [:nchevobbe]]

Sorry, I was not clear enough, I'm not sure for the border on the console input (at the botton).
Also, I just saw that in the mockup, the filter input does take up all the height of the toolbar, so I think we can go with that.

And thanls for working on this zigen !

Comment 25

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8939194 [details]
Bug 1421395 - change focus ring in console from gray dotted line to blue, change active input caret color to toolbar-checked-color.

https://reviewboard.mozilla.org/r/209594/#review218178

r- because of minor visual issues.

Comment 26

[Nicolas Chevobbe [:nchevobbe]]

Zigen, are you blocked on this ? If you don't have time to work on this no worries, we'll pick up the work or make it available for someone else :)

Comment 27

[Kentaro Teramoto [:zigen]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #26)
> Zigen, are you blocked on this ? If you don't have time to work on this no
> worries, we'll pick up the work or make it available for someone else :)

Thanks Nicolas, and I'm sorry for late, I was too busy.
But now I have a time, so I want to try this.

About "blink", I think it's better to use "transition" like searchinput in inspector.
https://searchfox.org/mozilla-central/source/devtools/client/themes/common.css#488

I missed the difference of height between my patch and the mockup.
I'll try to add padding to adjust the height.

Comment 28

[Nicolas Chevobbe [:nchevobbe]]

No need to apologies, it's already great that you took time to help the project :)

Comment 29

[Kentaro Teramoto [:zigen]]

Comment on attachment 8939194 [details]
Bug 1421395 - change focus ring in console from gray dotted line to blue, change active input caret color to toolbar-checked-color.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209594/diff/2-3/

Comment 30

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8939194 [details]
Bug 1421395 - change focus ring in console from gray dotted line to blue, change active input caret color to toolbar-checked-color.

https://reviewboard.mozilla.org/r/209594/#review221702

Thanks a lot Zigen for working on this !
I have a few comments, but nothing too important :).
Let's remove the border from the jsterm-input-node for now because it seems a bit intense to me. 
We can revisit that in a later bug if we feel the need to have it.

::: devtools/client/themes/webconsole.css:463
(Diff revision 3)
> +textarea.jsterm-input-node:focus {
> +  border: 1px solid var(--blue-50);
> +  transition: all 0.2s ease-in-out;
> +  outline: none;
> +}

I still don't think we should have this border. This is a bit too distracting and we already have the icon changing color to indicate the input is focused.

::: devtools/client/themes/webconsole.css:853
(Diff revision 3)
>  .webconsole-filterbar-primary .devtools-plaininput {
>    flex: 1 1 100%;
> +  min-height: calc(var(--primary-toolbar-height) - 1px);
> +  margin-left: 1px;
> +  border: 1px solid transparent;
> +}

with the new border, it feels like the placeholder (or the text), is a bit too close to the filter icon.
Could we put a `padding-inline-start: 4px;` on the input so the text has a bit more room ?

::: devtools/client/themes/webconsole.css:855
(Diff revision 3)
>    -moz-user-select: none;
>  }
>  
>  .webconsole-filterbar-primary .devtools-plaininput {
>    flex: 1 1 100%;
> +  min-height: calc(var(--primary-toolbar-height) - 1px);

I think we can replace this height with : 
align-self: stretch;

which will take all the available height of the container.

Comment 31

[Kentaro Teramoto [:zigen]]

Comment on attachment 8939194 [details]
Bug 1421395 - change focus ring in console from gray dotted line to blue, change active input caret color to toolbar-checked-color.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209594/diff/3-4/

Comment 32

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8939194 [details]
Bug 1421395 - change focus ring in console from gray dotted line to blue, change active input caret color to toolbar-checked-color.

https://reviewboard.mozilla.org/r/209594/#review221940

Comment 33

[Nicolas Chevobbe [:nchevobbe]]

Thanks a lot zigen !

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fa3b4e722d29
change focus ring in console from gray dotted line to blue, change active input caret color to toolbar-checked-color. r=nchevobbe

Comment 35

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/fa3b4e722d29

Comment 36

[Hiroyuki Ikezoe (:hiro)]

Nice work, zigen!

Comment 37

[Kentaro Teramoto [:zigen]]

Thanks for helping me! 
I'll try to other bug.