1445703 - Console input focus ring should be blue

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

[Erica Wright [:ewright]]

+++ This bug was initially created as a clone of Bug #1421395 +++

In the console, the input focus ring should also be `1px solid var(--blue-50);`

cc: @victoria

Comment 1

[Erica Wright [:ewright]]

Attachment: Bug 1445703 - Change console input focus ring from grey dotted line to blue.

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

Comment 2

[Erica Wright [:ewright]]

Attachment: input-focus.jpg

Attached is the result of the patch

Comment 3

[Victoria Wang [:victoria]]

Awesome, thanks Erica! 

One tiny thing (that exists in the current version as well) - the bottom corners of the border are being cut off due to the rounded window edges. I'm not sure if this is fixable by just adding bottom border radius since (I'm guessing) on some OSes, the window might not have rounded corners. If this is the case, I think it's fine to go ahead with this.

Comment 4

[Erica Wright [:ewright]]

Comment on attachment 8958903 [details]
Bug 1445703 - Change console input focus ring from grey dotted line to blue.

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

Comment 5

[Erica Wright [:ewright]]

Attachment: input-focus2.jpg

Updated how the focus ring looks. The top two are windows, the bottom one is mac with the corners rounded now. The rounded corner styling is only applied to mac.

Comment 6

[Victoria Wang [:victoria]]

Oh this is perfect! Thank you!

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Thanks Erica for the patch, and sorry for the delay.
The addition of the focus ring was discussed in https://bugzilla.mozilla.org/show_bug.cgi?id=1421395#c23 where I raised some concerns.
I'm fine with having the focus as long as we avoid the "focus flash" I screencasted.
What do you think ?

Comment 8

[Victoria Wang [:victoria]]

Ah, I didn't realize where that previous work had ended up. I'm not clear on how you want to avoid the focus flash - it doesn't seem like the blue outline will be much more intense than the current dotted outline. We could potentially go with a lighter blue for these focus rings to minimize that effect (e.g. the blue that the awesome bar uses).

Comment 9

[Erica Wright [:ewright]]

:nchevobbe okay, I missed that conversation in the last bug, too. So to avoid the focus flash... would you like it to not become unfocused on the down-click if that click is within the area that would put focus back on input? Or, the animation to bring in the focus could be slower/gentler?

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

I was thinking to add a small delay before firing the animation, so it won't flash if the user only do a quick click in the output.
If it is tricky, let's ship this, and then we can see if people complain about it.

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8958903 [details]
Bug 1445703 - Change console input focus ring from grey dotted line to blue.

https://reviewboard.mozilla.org/r/227774/#review235372

I tested your patch and I don't feels so strong about the flash than I did previously. In fact, it does highlight the the focus was put on the input again. Without it, one could think that clicking on the output is broken because it doesn't blur the input.
So I say, let's ship this, and we'll see if people complain :)
Thanks a lot Erica and sorry for the delay.

Comment 12

[Nicolas Chevobbe [:nchevobbe]]

The patch makes devtools/client/webconsole/new-console-output/test/mochitest/browser_jsterm_input_expansion.js fail.

Comment 13

[Erica Wright [:ewright]]

Comment on attachment 8958903 [details]
Bug 1445703 - Change console input focus ring from grey dotted line to blue.

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

Comment 14

[Erica Wright [:ewright]]

devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_context_menu_store_as_global.js fails with a timeout, but from what I can tell it already failed before this patch.

The fix I added for the other test is a little odd... I tried to find a nicer fix with just css but couldn't find anything. Let me know if you have a better suggestion.

Comment 15

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8958903 [details]
Bug 1445703 - Change console input focus ring from grey dotted line to blue.

https://reviewboard.mozilla.org/r/227774/#review236646

::: devtools/client/webconsole/jsterm.js:1038
(Diff revision 3)
> +    // add 2 extra pixels to account for the border
>      if (scrollHeight > 0) {
> -      inputNode.style.height = scrollHeight + "px";
> +      inputNode.style.height = (scrollHeight + 2) + "px";

would you think we could query the element to get its borders (after the init for example) and use it here ?
Having a 2px hardcoded directly here seems a bit risky to me (this is a part that is likely to change in the coming months, so it would be nice to have something less brittle).
What do you think of this ? I don't feel strongly about this since we have a test covering that, but still think it would be nice 
Your call to make :)

Comment 16

[Erica Wright [:ewright]]

Comment on attachment 8958903 [details]
Bug 1445703 - Change console input focus ring from grey dotted line to blue.

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

Comment 17

[Erica Wright [:ewright]]

:nchevobbe, thanks for the reminder on this. There doesn't seem to be a handy way to query the border measurements, so I'm measuring the outer height and the inner height and getting the difference, which currently gets the border.

Comment 18

[Nicolas Chevobbe [:nchevobbe]]

Thanks Erica. This is not ideal but I think it's fine.
I hope we'll be able to get rid of some of the magic for the input size soon, so we may be able to revisit this later.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ac45183a24f5
Change console input focus ring from grey dotted line to blue. r=nchevobbe

Comment 20

[Narcis Beleuzu [:NarcisB]]

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

Comment 21

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 61.0a1 (2018-03-14) on Windows 10, 64 Bit!

This bug's fix is verified with latest Beta!

Build ID   : 20180517141400   
User Agent : Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0