Track Changes - Changes in rules with duplicate selectors are not shown

VERIFIED FIXED in Firefox 65

Status

defect
P2
normal
VERIFIED FIXED
6 months ago
4 months ago

People

(Reporter: rcaliman, Assigned: rcaliman)

Tracking

(Blocks 1 bug)

unspecified
Firefox 66
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox65 verified, firefox66 verified)

Details

Attachments

(1 attachment)

Assignee

Description

6 months ago
Steps to reproduce:
- Paste and run this in the address bar:
data:text/html,<style>body {color: red; }</style>test
- Open the DevTools Inspector, select the <body> element
- In the Rules panel, change the value of the color declaration
- Switch to the Fonts panel and change the font size using the slider
- Switch to the Changes panel

Expected result:
Both the color declaration and the font-size changes show up.

Actual result:
Only the color declaration shows as changed. The header for another set of changes is shown (the element inline style), but no changes are shown for it.

Cause:

The optimization in the Changes panel to prevent re-rendering rules with the same selector does not take into account the selector's source (stylesheet / element).

See: https://searchfox.org/mozilla-central/rev/f2028b4c38bff2a50ed6aa1763f6dc5ee62b0cc4/devtools/client/inspector/changes/components/ChangesApp.js#69-71

For changes in different rules with identical selectors, only changes for the first rule will show up. The source id needs to also be taken into account along with the rule id.

Found by Paul when testing the Timing Function Editor along with the Font Editor. See video: https://test-svqa.tinytake.com/sf/MzExODY3M185MzQ4NTYw
Assignee

Updated

6 months ago
Blocks: track-changes
No longer blocks: 1503920
Assignee

Updated

5 months ago
Duplicate of this bug: 1513828
Assignee

Updated

5 months ago
Blocks: 1518147
Assignee

Comment 2

5 months ago
This patch introduces a Redux selector for the Changes slice. A selecrtor  here is a fancy word for a method that returns a specific subset from the state without having to expose the state structure to the consumer. It is also useful for returning a derived version of the state which we're using here.

The changes are not stored in a nested tree in the Redux state. A shallow structure is easier to query and to work with in our use case. But the Changes panel needs to display CSS rules in a hierarchical structure (ex: a style rule which is a child of a @media rule). To do this, the React component used to build up the nested structure as it consumed the changes from the Redux state. To prevent rendering duplicates of rules (once as part of a descendant tree and once as a standalone rule) the React component kept a reference of rules it had previously rendered. This had a flaw because it didn't account for the rule's stylesheet. The problem was that rules with identical selectors from different stylesheets would not all be rendered because they would be accidentally marked as already rendered. This is too much business knowledge for the React component anyway. The Redux state should present itself in a way that's simple to render for the React component. Hence the introduction of the getChangesTree() selector in this patch. This method allows us to present a comfortable structure to React while keeping Redux structure comfortable to work with (though this separation enables increased flexibility to restructure the Redux state without impacting the React components). 

More about Redux selectors: https://redux.js.org/recipes/computing-derived-data

Comment 3

4 months ago
Pushed by rcaliman@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ebee32dc7abe
Introduce Redux selector for tracked changes to return the nested tree structure; r=pbro

Comment 4

4 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66
Assignee

Comment 5

4 months ago

Comment on attachment 9035367 [details]
Bug 1510790 - Introduce Redux selector for tracked changes to return the nested tree structure; r=pbro

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: Changes made in different CSS rules with the same selector won't all be shown in the Changes panel. This may constitute data loss if developers rely on the Changes panel for exporting changes. While initially thought to be an edge case, QA found multiple examples where this is an issue on popular websites such as YouTube.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce:

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): The information about tracked CSS changes was already collected. The issue was correctly rendering this information, which this patch addresses.

String changes made/needed: none

Attachment #9035367 - Flags: approval-mozilla-beta?
Flags: qe-verify+

Comment on attachment 9035367 [details]
Bug 1510790 - Introduce Redux selector for tracked changes to return the nested tree structure; r=pbro

[Triage Comment]
Not crazy about taking this in the last week of the Beta cycle, but the potential dataloss issues sound concerning. Thanks for including automated tests for this at least. Approved for 65.0b11. I would like QA to do some additional testing and verification around this if possible, too, however.

Attachment #9035367 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 8

4 months ago

I managed to reproduce the issue on older 66.0a1 Nightly build (20190102094850) and on the last available 65.0b10 DevEdition build (20190110221328).

I was not able to reproduce the issue on Mac OS 10.13.6.

Verified as fixed using the latest available 66.0a1 Nightly build - 20190114104248 on Windows 10, Windows 7 and Ubuntu 18.04.

Will reverify the fix on DevEdition once a new build is available.

Assignee

Comment 9

4 months ago

Thank you both for taking this on to beta and for the testing.

Assignee

Updated

4 months ago
Duplicate of this bug: 1518147

Comment 11

4 months ago

Verified as fixed using the latest available 65.0b11 DevEdition build - 20190114172331 on Windows 10, Windows 7 and Ubuntu 18.04.

Flags: qe-verify+

Updated

4 months ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.