Closed Bug 1445703 Opened 2 years ago Closed 2 years ago

Console input focus ring should be blue

Categories

(DevTools :: Console, defect, P3)

58 Branch
defect

Tracking

(firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: ewright, Assigned: ewright)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

+++ 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
Assignee: nobody → ewright
Attached image input-focus.jpg (obsolete) —
Attached is the result of the patch
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.
Attached image 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.
Attachment #8958905 - Attachment is obsolete: true
Oh this is perfect! Thank you!
Attachment #8958903 - Flags: review?(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 ?
Flags: needinfo?(ewright)
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).
: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?
Flags: needinfo?(ewright)
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 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.
Attachment #8958903 - Flags: review?(nchevobbe) → review+
The patch makes devtools/client/webconsole/new-console-output/test/mochitest/browser_jsterm_input_expansion.js fail.
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 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 :)
Flags: needinfo?(ewright)
Flags: needinfo?(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.
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.
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ac45183a24f5
Change console input focus ring from grey dotted line to blue. r=nchevobbe
https://hg.mozilla.org/mozilla-central/rev/ac45183a24f5
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
QA Whiteboard: [good first verify]
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
QA Whiteboard: [good first verify] → [good first verify] [bugday-20180516]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.