Minor Photon changes to Console

RESOLVED FIXED in Firefox 60

Status

P3
normal
RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: victoria, Assigned: zigen)

Tracking

(Blocks: 1 bug)

58 Branch
Firefox 60
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

a year ago
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.
(Reporter)

Updated

a year ago
Blocks: 1389730
Priority: -- → P3
When doing this work, maybe take Bug 1395825 into consideration
See Also: → bug 1395825
(Reporter)

Comment 2

a year ago
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
(Assignee)

Comment 3

a year ago
Hi there.
I'm looking for my first bug. I would like to take this.
May I work on this?
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/ :)

Updated

a year ago
Assignee: nobody → hrlclb
Status: NEW → ASSIGNED
(Assignee)

Comment 5

a year ago
Created attachment 8935803 [details]
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?
Flags: needinfo?(victoria)
(Reporter)

Comment 6

a year ago
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!
Flags: needinfo?(victoria)
(Assignee)

Comment 7

a year ago
Thanks for replying, Victoria!
The mockup looks very cool!

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

Comment 8

a year ago
Created attachment 8936198 [details] [diff] [review]
change-focusring-patch
Attachment #8936198 - Flags: review?(nchevobbe)
(Assignee)

Comment 9

a year ago
Created 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
Attachment #8936198 - Attachment is obsolete: true
Attachment #8936198 - Flags: review?(nchevobbe)
Attachment #8936199 - Flags: review?(nchevobbe)
(Assignee)

Comment 10

a year ago
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 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
Attachment #8936199 - Flags: review?(nchevobbe) → review?(gl)
(Reporter)

Comment 12

a year ago
Just wanted to mention that this bug somewhat overlaps this: https://bugzilla.mozilla.org/show_bug.cgi?id=1395825
(Assignee)

Comment 13

a year ago
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 hidden (mozreview-request)

Comment 15

a year ago
mozreview-review
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.
Attachment #8939194 - Flags: review-
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.
Attachment #8936199 - Flags: review?(gl)
Attachment #8939194 - Flags: review?(gl) review?(gl)
Attachment #8936199 - Attachment is obsolete: true
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.
(Reporter)

Comment 18

a year ago
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 hidden (mozreview-request)
Attachment #8939194 - Flags: review?(gl) → review?(bgrinstead)
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

a year ago
mozreview-review
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
(Assignee)

Comment 22

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

Updated

a year ago
Attachment #8939194 - Flags: review?(bgrinstead) → review?(nchevobbe)
Created attachment 8942177 [details]
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.
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

a year ago
mozreview-review
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.
Attachment #8939194 - Flags: review?(nchevobbe) → review-
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 :)
Flags: needinfo?(hrlclb)
(Assignee)

Comment 27

a year ago
(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.
Flags: needinfo?(hrlclb)
No need to apologies, it's already great that you took time to help the project :)
Comment hidden (mozreview-request)

Comment 30

a year ago
mozreview-review
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.
Attachment #8939194 - Flags: review?(nchevobbe) → review-
Comment hidden (mozreview-request)

Comment 32

a year ago
mozreview-review
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
Attachment #8939194 - Flags: review?(nchevobbe) → review+
Thanks a lot zigen !

Comment 34

a year ago
Pushed by nchevobbe@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/fa3b4e722d29
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Nice work, zigen!
(Assignee)

Comment 37

a year ago
Thanks for helping me! 
I'll try to other bug.
Blocks: 1445703

Updated

7 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.