Closed Bug 1328001 Opened 3 years ago Closed 3 years ago

Focusring isn't noticeable in devtools buttons on dark theme

Categories

(DevTools :: General, defect, P2)

defect

Tracking

(firefox-esr52 wontfix)

VERIFIED FIXED
Firefox 55
Tracking Status
firefox-esr52 --- wontfix

People

(Reporter: arni2033, Assigned: Honza)

References

Details

(Keywords: regression)

Attachments

(3 files)

>>>   My Info:   Win7_64, Nightly 53, 32bit, ID 20161119030204 (2016-11-19)
STR_1:
1. Open url   data:,
2. Open devtools -> options, set dark theme
3. Click on the page, then press Tab key to focus element picker button

AR:  Focusring in element picker button isn't visible. Actually, all buttons (not tabs!) are affected
ER:  Focusring should be noticeable, just like before bug 1266419

This is regression from bug 1266419. Regression range:
> https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=3a87296fe4145138c2ce15512bb31f76fe869cb4&tochange=42fab251fe111d5f891c9bde0ee1fb6f7f946a50@ Jan Honza Odvarko [:Honza] (PTO: 12/09-01/01):
It seems that this is a regresion caused by your change. Please have a look.
Component: Untriaged → Developer Tools
No longer blocks: 1277113
:bgrins, maybe we just need to choose a good color for a focus ring on dark theme?
Flags: needinfo?(bgrinstead)
Priority: -- → P2
Attached image focusring.png
There is already a focusring that can be seen on the toolbox tabs (see sceenshot).  Just need to make sure it also applies to command buttons and buttons within the tools themselves if necessary
Flags: needinfo?(bgrinstead)
(In reply to (Unavailable until Apr 3) [:bgrins] from comment #2)
> 
> There is already a focusring that can be seen on the toolbox tabs (see
> sceenshot).  Just need to make sure it also applies to command buttons and
> buttons within the tools themselves if necessary

:jryans, can you do this given Brian's lack of availability?
Flags: needinfo?(jryans)
Yura, are you able to look into this?

I'm having trouble understanding the expected focus order locally (I seem to see the same behavior on dark and light theme...), and I haven't been involved in most of that work lately.  Maybe you know what's needed here?
Flags: needinfo?(jryans) → needinfo?(yzenevich)
Confirmed, some buttons do not have the focus ring , though the tabs still do.

So what happens is buttons (not tabs) pretty much in most places have no focus ring. For example, console tab's clear and toggle filter buttons. Or as described in summary, buttons in the top level toolbar.

I don't have the time at the moment to take a look at this. ni? Jan since he worked on the bug that caused the regression.
Flags: needinfo?(yzenevich) → needinfo?(odvarko)
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Flags: needinfo?(odvarko)
Attached image focus-ring.png
Patch ready for review. Here is a screenshot that shows focus-ring for various buttons in the Toolbox.

Honza
\o/ thanks for taking this on!
Comment on attachment 8852441 [details]
Bug 1328001 - Fix focusring on devtools buttons;

https://reviewboard.mozilla.org/r/124670/#review127698

It looks reasonable to me, but I'd like :ntim to check as well, since he's done a lot with the CSS here.
Attachment #8852441 - Flags: review?(jryans) → review+
(In reply to Jan Honza Odvarko [:Honza] from comment #7)
> Created attachment 8852446 [details]
> focus-ring.png
> 
> Patch ready for review. Here is a screenshot that shows focus-ring for
> various buttons in the Toolbox.
> 
> Honza

There seems to be a couple of black focusrings on the last screenshot. What do these correspond to ? It doesn't really make sense to have multiple elements focused at once. Even if those focusrings are valid, they should match the current text color.
Flags: needinfo?(odvarko)
(In reply to Tim Nguyen :ntim from comment #10)
> There seems to be a couple of black focusrings on the last screenshot. What
> do these correspond to ? It doesn't really make sense to have multiple
> elements focused at once. Even if those focusrings are valid, they should
> match the current text color.

These are trails after moving the focus from button to button. It disappears if you force repaint (e.g. by changing browser widow size). I don't know why it isn't properly repainted, but the problem was there before (I am on Win10).

Honza
Flags: needinfo?(odvarko)
(In reply to Jan Honza Odvarko [:Honza] from comment #11)
> (In reply to Tim Nguyen :ntim from comment #10)
> > There seems to be a couple of black focusrings on the last screenshot. What
> > do these correspond to ? It doesn't really make sense to have multiple
> > elements focused at once. Even if those focusrings are valid, they should
> > match the current text color.
> 
> These are trails after moving the focus from button to button. It disappears
> if you force repaint (e.g. by changing browser widow size). I don't know why
> it isn't properly repainted, but the problem was there before (I am on
> Win10).
Ah, might be related to the dark theme inversion filter.
Comment on attachment 8852441 [details]
Bug 1328001 - Fix focusring on devtools buttons;

https://reviewboard.mozilla.org/r/124670/#review127746
Attachment #8852441 - Flags: review?(ntim.bugs) → review+
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/148723dfb3bf
Fix focusring on devtools buttons; r=jryans,ntim
https://hg.mozilla.org/mozilla-central/rev/148723dfb3bf
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Tiny patch that seems paper-cutty enough to warrant backport to affected branches I think?
(In reply to Ryan VanderMeulen [:RyanVM] from comment #16)
> Tiny patch that seems paper-cutty enough to warrant backport to affected
> branches I think?

Yes, agree, thanks for the note. Let's give it some time in Nightly and backport if success.

I'll keep my NI on.

Honza
Comment on attachment 8852441 [details]
Bug 1328001 - Fix focusring on devtools buttons;

Approval Request Comment
[Feature/Bug causing the regression]: n/a
[User impact if declined]: Focus ring/rectangle not visible
[Is this code covered by automated tests?]: n/a
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: Very low risk
[Why is the change risky/not risky?]: Just CSS change
[String changes made/needed]: n/a
Flags: needinfo?(odvarko)
Attachment #8852441 - Flags: approval-mozilla-aurora?
Attachment #8852441 - Flags: approval-mozilla-beta?
Comment on attachment 8852441 [details]
Bug 1328001 - Fix focusring on devtools buttons;

Tiny css change, not a new regression but, ok, why not. 

My only reason why not would be that we've lived with it since Firefox 49 and I am very overloaded with work at the end of the beta cycle with security and stability and major feature fixes that are somewhat risky and stressful. So, most of the time, could we please let this kind of thing ride the train if it is late in a release cycle?
Attachment #8852441 - Flags: approval-mozilla-beta?
Attachment #8852441 - Flags: approval-mozilla-beta+
Attachment #8852441 - Flags: approval-mozilla-aurora?
Attachment #8852441 - Flags: approval-mozilla-aurora+
Probably not worth an ESR52 backport. Feel free to set it back to affected and request approval if you feel otherwise.
I have reproduced this bug with Nightly 53.0a1 (2017-01-01) on Windows 10, 64 bit!

The bug's fix is now verified on Latest 

Nightly 55.0a1
Build ID 	20170418030220
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0

Aurora 54.0a2
Build ID 	20170419004020
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:54.0) Gecko/20100101 Firefox/54.0

Beta 53.0b9
Build ID 	20170403072723
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0

[bugday-20170419]
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.