1546621 - Pseudo-element properties are not marked as overwritten in the rules inspector

Status: VERIFIED FIXED

(DevTools :: Inspector: Rules, defect, P2)

Description

[Cristian Fogel [:cfogel]]

Attachment: pseudoRules.png

Affected versions

• 67.0b13, 68.0a1 (2019-04-23), 66.0.3, 57.0b9, 52.9.esr

Affected platforms

• Windows10, Ubuntu 18.04

Steps to reproduce

1. Launch Firefox, DevTools inspector;
2. Access https://www.w3schools.com/css/tryit.asp?filename=trycss_firstline
3. Right Click and inspect on the Red text row from the example;
4. In the rules tab add multiple color properties

Expected result

• the previous color(s) become dashed and are no longer marked as active

Actual result

• no disable indicator for overwritten properties

Regression range

• will provide one asap;
• 31.0.a1 is not affected by this issue;

Additional notes

• attached screenshot with the issue;
• not limited to the color property, any other from the tested ones have the same behavior for;

Comment 1

[Razvan Caliman [:rcaliman]]

Attachment: Screenshot 2019-04-24 at 13.05.51.jpg

Additional context:

The issue manifests when the pseudo-element is not selected. ::first-line doesn't have a correspondent node in the markup view to be selected, but ::before and ::after do. If selecting the pseudo-element directly, not its parent element, the overwritten properties show as expected.

To test, run this in the address bar of a new tab:
data:text/html,<style>div::before { color:red; color:black; content: "TEST" }</style><div>

Open DevTools and inspect the <div>, then Inspect the ::before pseudo-element for the <div>. Notice the different styling on the pseudo-element's declarations in the Rules view.

Comment 2

[Razvan Caliman [:rcaliman]]

The issue manifests when the pseudo-element itself is not selected.

Comment 3

[Alice0775 White]

Regression window:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3435dd7ad71fe9003bdeee18fd38d815e033beef&tochange=dc4d7f68030e9edd22ad0bb0bd2244d047dd767d

Suspect: 7275ea886739 Nicolas Chevobbe — Bug 1253869 - Don't assume that rule in the Rules view match the selected node when checking for overidden properties. r=gl

Comment 4

[Tim Spurway [:tspurway]]

Hey Gabriel, can you look at this one?

Comment 5

[Tim Spurway [:tspurway]]

Sorry, Nicolas, could you look at this one?

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

That's quite an old patch regression :D I may have a look but can't promise anything.
Since it's a P1, feel free to have a look Inspector people :)

Comment 7

[Razvan Caliman [:rcaliman]]

I'll take a look at it. I have a bit more exposure to this part of the code.
The P1 priority signifies the intent that we should fix this during this cycle, not that an urgent fix is required. It's on my radar now.

Comment 8

[Patrick Brosset <:pbro>]

We're very close to 68 going to beta, and this is an old regression, let's mark it as fix-optional for 68 then. It would be very nice to fix it sooner rather than later as this is a case where the rule-view is showing incorrect information, so fails at doing its job.

Comment 9

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1546621 - Ensure pseudo-element declarations are marked as overridden when host element is selected. r=pbro

This patch adjust the method which marks CSS declaration as overridden to also consider pseudo-elements when they're not directly selected (i.e. their host element is selected).

For style rules on regular elements, there's a condition to only consider rules that match the selected element. This is done so that unmatched rules (you may get one after renaming a selector in the Rules view) don't influence the remaining matching rules. For pseudo-elements, this strict check caused pseudo-element rules to not show overridden declarations if they weren't directly selected in the markup view. For some types of pseudo-elements, it is impossible to ever select their nodes in the markup view because they don't exist, for example ::first-line, ::selection, ::first-letter, etc.

Also introduced an optimization to not call the logic to mark declarations as overridden for non-existent pseudo-elements. The previous code ran a minimum of 17 times, once for every entry in cssProperties.pseudoElements array which includes the full dictionary of pseudo-elements supported by Firefox.

Comment 10

[Razvan Caliman [:rcaliman]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c7ed23c6304414d30a006f7c1cf0d5158113f87

Comment 11

[Razvan Caliman [:rcaliman]]

Try again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1210c4ff26ea95f70ee92fb7754608912dbbb215

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2901438abf7c
Ensure pseudo-element declarations are marked as overridden when host element is selected. r=pbro

Comment 13

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/2901438abf7c

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider for Beta uplift or can this ride the trains?

Comment 15

[Razvan Caliman [:rcaliman]]

I believe this can safely ride the trains. It touches some sensitive code and it would be beneficial to let it go in slowly and notice any potential new bugs.

Comment 16

[Cristian Fogel [:cfogel]]

Fix confirmed with 69.0a1 (2019-07-01) on Windows 10, macOS 10.11, Ubuntu 16.04.