Cristian Fogel [:cfogel] Reporter
	Description • 5 years ago

[Affected versions]:
- Win10x64, Ubuntu 16.04 x64, macOS10.12.6

[Affected platforms]:
- Firefox: 65.0a1 (2018-12-07) 

[Steps to reproduce]:
1. Launch Firefox;
2. Access https://www.w3schools.com/tags/tryit.asp?filename=tryhtml_iframe
3. Right click on any element;
4. Click on the Inspect Element option;
5. In the inspector add any property (ex:  add to element {color: red;});
6. Click on the Changes tab;
7. Click on the page so that the inspector is no longer tagged as active;
8. Zoom in/out a couple of times;

[Expected result]:
- the tracked changes in Changes tab are still displayed;

[Actual result]:
- the tab (apparently) resets to the default state;

[Regression range]:
- bug 1506170 appears to be at fault

[Additional notes]:
- attached recording with the issue;
- Preference: devtools.inspector.changes.enabled - true | if the Changes tab not displayed in the inspector;
- managed to reproduce in other pages with iframes in them, but on a locally saved file the issue did not manifest.

	Cristian Fogel [:cfogel] Reporter
	Comment 1 • 5 years ago

Hey Razvan, mind taking a look at this as well?

	Razvan Caliman [:rcaliman] Assignee
	Comment 2 • 5 years ago

Thanks for filing, Cristi!

This is indeed an issue and it's caused by the iframes the advertising is loaded into. You can observe that in the video as well: notice the changes are lost as soon as the ad refreshes. 

The zooming causes a navigation in the ad's iframe. This triggers the "will-navigate" event on the TargetActor and trips the ChangesActor into believing the page itself navigates. This check needs to be made more strict:
https://searchfox.org/mozilla-central/source/devtools/server/actors/changes.js#32

You can replicate the issue without zooming by right-clicking the ad, in the context menu, select This Frame > Reload Frame. This also clears the Changes panel.

The zooming issue does not occur if you go to about:preferences#privacy and set Content Blocking to Strict. This blocks the ads in the mentioned page. Zooming no longer causes the ads to refresh/navigate and the issue does not occur. This explanations why the issue didn't manifest on a local file.

	Pulsebot
	Comment 5 • 5 years ago

Pushed by rcaliman@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7a5bfc35d789
Clear changes only when parent document navigates, not iframes; r=bradwerth

	Bogdan Tara[:bogdan_tara | bogdant]
	Comment 6 • 5 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/7a5bfc35d789

	Cristian Fogel [:cfogel] Reporter
	Comment 7 • 5 years ago

Verified with 66.0a1 (2018-12-13) on the mentioned OSs

	Razvan Caliman [:rcaliman] Assignee
	Comment 8 • 5 years ago

Comment on attachment 9031181 [details]
Bug 1512665 - Clear changes only when parent document navigates, not iframes; r=bradwerth

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1506170

User impact if declined: Tracked CSS changes may be lost if there's an iframe on the inspected page which reloads (like ads), thus losing this data for users in that scenario.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: Yes

If yes, steps to reproduce: - Run this the address bar of a new tab:
data:text/html,<style>h1 {color: red }</style><h1>TEST</h1><iframe src="https%3A%2F%2Fwww.example.com%2F" />

- Open the DevTools Inspector
- Select the <h1> in the host document
- Switch to the the Rules panel and make any change to CSS properties
- Select the <h1> in the iframe
- Switch to the the Rules panel and make any change to CSS properties
- Right-click on the iframe on the page
- From the context menu, pick This Frame > Reload Frame
- Switch to the Changes panel

Expected: The list of changes should not be cleared. Change entries for both <h1> elements should appear.

Ideally, only the iframe changes should be cleared, but that's going to be solved in Bug 1513940. For now, we just want to ensure that changes are not lost accidentally when iframes reload.

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): The Changes clearing behaviour was already implemented prior to Firefox 65 Release. This patch only restricts its triggering as a result of the host page reload.

String changes made/needed:

	Ryan VanderMeulen [:RyanVM]
	Comment 9 • 5 years ago

Comment on attachment 9031181 [details]
Bug 1512665 - Clear changes only when parent document navigates, not iframes; r=bradwerth

[Triage Comment]
Fixes a bug in the new track changes feature. Approved for 65.0b5.

	Ryan VanderMeulen [:RyanVM]
	Comment 10 • 5 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/23693d234e37

	Oana Botisan, Desktop QA
	Comment 11 • 5 years ago

I managed to reproduce the issue using Nightly (2018-12-07) and the method from comment 0 on Windows 10 x64.
Using Firefox 65.0b5 (Build ID 	20181216183345 - taskcluster) and the method from comment 0 and comment 8 on Windows 10 x64, macOS 10.13 and Ubuntu 16.04 x64, I verified the fix. The issue is not reproducing anymore.

	Razvan Caliman [:rcaliman] Assignee
	Comment 12 • 5 years ago

Thank you all!