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

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement)

Description

[Tim Nguyen :ntim]

Chrome has a checkbox and a menu-item for this.

Comment 1

[Tim Nguyen :ntim]

Attachment: Bug 1497969 - Add a way to emulate :focus-within pseudo-class. r?pbro

Comment 2

[Tim Nguyen :ntim]

I'm a bit surprised that you have to do changes in 12 files simply to add a new pseudo-class.

Comment 3

[Patrick Brosset <:pbro>]

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.

Comment 4

[Tim Nguyen :ntim]

(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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/06f4b6597f74
Add a way to emulate :focus-within pseudo-class. r=pbro

Comment 6

[Noemi Erli[:noemi_erli]]

Backed out changeset 06f4b6597f74 (Bug 1497969) for failures in devtools/client/inspector/rules/test/browser_rules_pseudo_lock_options.js

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed,busted,exception&classifiedState=unclassified&selectedJob=204752541&revision=06f4b6597f74d01d0dce767a83ed537612cb7173

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=204752541&repo=autoland&lineNumber=20744

Backout: https://hg.mozilla.org/integration/autoland/rev/b7fc0fb0f0e26404469cdddeef541b8154835ec0

Comment 7

[Tim Nguyen :ntim]

(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.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0ff0cb536428
Add a way to emulate :focus-within pseudo-class. r=pbro

Comment 9

[Cristian Brindusan [:cbrindusan]]

Backed out changeset 0ff0cb536428 (Bug 1497969) for devtools failures on browser_rules_add-rule-pseudo-class.js.

Backout: https://hg.mozilla.org/integration/autoland/rev/50d5dc05d31de264b9dac5193fe060336e80991b

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=0ff0cb5364283cb55240c42e476ca1e5458ec567&selectedJob=204891511

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=204873386&repo=autoland&lineNumber=1535

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0bad7b51e301
Add a way to emulate :focus-within pseudo-class. r=pbro

Comment 11

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/0bad7b51e301