Focusring isn't noticeable in devtools buttons on dark theme

VERIFIED FIXED in Firefox 53

Status

()

Firefox
Developer Tools
P2
normal
VERIFIED FIXED
a year ago
9 months ago

People

(Reporter: arni2033, Assigned: Honza)

Tracking

({regression})

Trunk
Firefox 55
regression
Points:
---

Firefox Tracking Flags

(firefox50 wontfix, firefox51 wontfix, firefox52 wontfix, firefox-esr52 wontfix, firefox53 verified, firefox54 verified, firefox55 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

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

Updated

a year ago
Component: Untriaged → Developer Tools
(Reporter)

Updated

a year ago
No longer blocks: 1277113
:bgrins, maybe we just need to choose a good color for a focus ring on dark theme?
status-firefox50: --- → wontfix
status-firefox51: --- → affected
status-firefox52: --- → affected
status-firefox53: --- → affected
Flags: needinfo?(bgrinstead)
Priority: -- → P2
Created attachment 8824456 [details]
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)
status-firefox51: affected → wontfix
status-firefox52: affected → fix-optional
status-firefox54: --- → affected
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Flags: needinfo?(odvarko)
(Assignee)

Comment 7

10 months ago
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

Comment 8

10 months ago
\o/ thanks for taking this on!
Attachment #8852441 - Flags: review?(ntim.bugs)

Comment 9

10 months ago
mozreview-review
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+

Comment 10

10 months ago
(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)
(Assignee)

Comment 11

10 months ago
(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)

Comment 12

10 months ago
(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 13

10 months ago
mozreview-review
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+

Comment 14

10 months ago
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/148723dfb3bf
Fix focusring on devtools buttons; r=jryans,ntim

Comment 15

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/148723dfb3bf
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Tiny patch that seems paper-cutty enough to warrant backport to affected branches I think?
status-firefox52: fix-optional → wontfix
status-firefox-esr52: --- → fix-optional
Flags: needinfo?(odvarko)
(Assignee)

Comment 17

10 months ago
(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
(Assignee)

Comment 18

10 months ago
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+

Comment 20

10 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/5a4c8c4ec6cf
status-firefox54: affected → fixed

Comment 21

10 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/5dbbaedfc0fb
status-firefox53: affected → fixed
Probably not worth an ESR52 backport. Feel free to set it back to affected and request approval if you feel otherwise.
status-firefox-esr52: fix-optional → wontfix

Comment 23

9 months ago
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]

Updated

9 months ago
Status: RESOLVED → VERIFIED

Updated

9 months ago
status-firefox53: fixed → verified
status-firefox54: fixed → verified
status-firefox55: fixed → verified
You need to log in before you can comment on or make changes to this bug.