Add a way to emulate :focus-within pseudo-class

RESOLVED FIXED in Firefox 64

Status

enhancement
RESOLVED FIXED
8 months ago
7 months ago

People

(Reporter: ntim, Assigned: ntim)

Tracking

({parity-chrome})

unspecified
Firefox 64

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

8 months ago
Chrome has a checkbox and a menu-item for this.
Assignee

Comment 2

8 months ago
I'm a bit surprised that you have to do changes in 12 files simply to add a new pseudo-class.
Hi Tim, thank you for working on this!

(In reply to Tim Nguyen :ntim (please use needinfo?) from comment #2)
> I'm a bit surprised that you have to do changes in 12 files simply to add a
> new pseudo-class.
There are usually very valid reasons for these things to occur.
In the present case, I'm guessing the feature was implemented long ago in only 1 place, and for only 3 pseudo-classes, and it therefore didn't make sense at the time to make it very generic and easily extensible.

And through years, we probably kept on adding to it (like more tests, more places where you can control pseudo-classes, etc.).
There probably was never a good time for anyone to refactor this part of the code so it would be just a few lines to add one more pseudo-class. This probably was never important enough, or a common-enough source of bugs.

Since you're adding a new pseudo-class now and noticed this duplication, maybe you'd be interested in doing a refactor?
It looks like a lot of places in this code could simply benefit by looping over a shared array of supported pseudo-classes rather than duplicating similar lines of code. That would make it already quite a bit simpler.

But I'll leave this up to you. I'll gladly accept the patch as is even if you don't plan on cleaning this code up.
I would simply ask that, in the future, you please try to avoid comments like this however. As I said, there are always reasons for code to look the way it does, it wasn't written in a complex or non-optimal way on purpose. Saying it's surprising doesn't really help.
Assignee

Comment 4

7 months ago
(In reply to Patrick Brosset <:pbro> from comment #3)
> I would simply ask that, in the future, you please try to avoid comments
> like this however. As I said, there are always reasons for code to look the
> way it does, it wasn't written in a complex or non-optimal way on purpose.
> Saying it's surprising doesn't really help.

Sorry, I didn't mean to offend anyone :/ I somewhat expected the code to be spread out, I simply didn't expect that it would not be easily searchable, and I was afraid that I would have missed one the files to edit (hence the importance of the review). I apologize for not expressing myself properly though :(

Comment 5

7 months ago
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/06f4b6597f74
Add a way to emulate :focus-within pseudo-class. r=pbro
Assignee

Comment 7

7 months ago
(In reply to Noemi Erli[:noemi_erli] from comment #6)
> Backed out changeset 06f4b6597f74 (Bug 1497969) for failures in
> devtools/client/inspector/rules/test/browser_rules_pseudo_lock_options.js

I had forgetten to the check the checkbox in that test.
Flags: needinfo?(ntim.bugs)

Comment 8

7 months ago
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0ff0cb536428
Add a way to emulate :focus-within pseudo-class. r=pbro
Assignee

Updated

7 months ago
Flags: needinfo?(ntim.bugs)

Comment 10

7 months ago
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0bad7b51e301
Add a way to emulate :focus-within pseudo-class. r=pbro

Comment 11

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0bad7b51e301
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Assignee

Updated

7 months ago
Assignee: nobody → ntim.bugs
You need to log in before you can comment on or make changes to this bug.