Closed Bug 1510790 Opened 6 years ago Closed 6 years ago

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

Categories

(DevTools :: Inspector, defect, P2)

Product:
DevTools
Component:
Inspector
Type:
defect

Tracking

(firefox65 verified, firefox66 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 66
Tracking Flags:
Tracking Status
firefox65 --- verified
firefox66 --- verified

People

(Reporter: rcaliman, Assigned: rcaliman)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1510790 - Introduce Redux selector for tracked changes to return the nested tree structure; r=pbro
47 bytes, text/x-phabricator-request
RyanVM
: approval-mozilla-beta+
Details | Review
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
Blocks: track-changes
No longer blocks: 1503920
Blocks: 1518147
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
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

https://hg.mozilla.org/mozilla-central/rev/ebee32dc7abe

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox66: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66

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+

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.

status-firefox66: fixed → verified

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

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

status-firefox65: fixed → verified
Flags: qe-verify+
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: