1707614 - Remove some unused usage of :-moz-devtools-highlighted.

Status: REOPENED

(DevTools :: Inspector, task, P3)

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1707614 - Remove one unused usage of :-moz-devtools-highlighted. r=nchevobbe

This internal pseudo-class used to be to highlight nodes in the
inspector, but the inspector no longer sets it. Now the only thing that
sets it is devtools/server/actors/accessibility/audit/contrast.js to
remove the text color / shadow from the test.

We should probably find another way to implement that that doesn't
misuse this pseudo-class (and then presumably remove this pseudo-class),
but for now this code is dead.

Comment 2

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1707614 - Don't use -moz-devtools-highlighted for background contrast checks in the accessibility inspector. r=nchevobbe

It's the wrong pseudo-class name! Plus, it's not really needed, just
override the page styles temporarily.

Remove the pseudo-class, since it's its only remaining usage.

Depends on D113371

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1e3e0d3d04ba
Remove one unused usage of :-moz-devtools-highlighted. r=nchevobbe

Comment 4

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/1e3e0d3d04ba

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d3056dc50fe3
Don't use -moz-devtools-highlighted for background contrast checks in the accessibility inspector. r=nchevobbe

Comment 6

[Iulian Moraru]

Backed out changeset d3056dc50fe3 (bug 1707614) for causing devtools failures at browser_rules_colorpicker-release-outside-frame.js .

Push with failures

Failure log

Backout link

Comment 7

[Emilio Cobos Álvarez [:emilio]]

So the test is timing out because the a11y contrast check runs while the picker is open, and basically changes the color value. This would be an issue with the existing implementation, I think, if "Show browser styles" was the default (which it should be fwiw, but that's a different thing :P)... So it's a bit unfortunate IMO.

But I just realized that this way of computing contrast (by changing the computed style) is fundamentally broken, since changing the color can affect arbitrary properties, so text with e.g. background-color: currentColor will be miscomputed... Do you know of a good way forward here / who's more familiar with this code? If not I guess I'll just not touch this for now...

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #7)

So the test is timing out because the a11y contrast check runs while the picker is open, and basically changes the color value. This would be an issue with the existing implementation, I think, if "Show browser styles" was the default (which it should be fwiw, but that's a different thing :P)... So it's a bit unfortunate IMO.

But I just realized that this way of computing contrast (by changing the computed style) is fundamentally broken, since changing the color can affect arbitrary properties, so text with e.g. background-color: currentColor will be miscomputed...

That's unfortunate indeed, and we should probably open a bug about it to keep track of this. In the meantime, would you happen to be able to fix the test, or does the new patch makes the situation worse than it was before?

Do you know of a good way forward here / who's more familiar with this code? If not I guess I'll just not touch this for now...

I think yzen did most of the work on this, and he left a few months ago :/