Closed Bug 1578459 Opened 11 months ago Closed 11 months ago

[Protections Panel] Category item order flickers when panel is opened for the first time after a location change

Categories

(Firefox :: Site Identity, task, P1)

task

Tracking

()

VERIFIED FIXED
Firefox 71
Tracking Status
firefox70 --- fixed
firefox71 --- verified

People

(Reporter: nhnt11, Assigned: nhnt11)

References

(Blocks 1 open bug)

Details

(Whiteboard: [privacy-panel][skyline])

Attachments

(2 files)

Theoretically it could happen in any situation when the popup is opened if a content blocking event was processed while it was closed, but after a location change is the most obvious case.

This can be fixed by moving the reorderCategoryItems function call to the popupshowing event rather than popupshown.

Easiest way to test this is to open two sites with different category items (e.g. https://youtube.com and https://senglehardt.com/test/trackingprotection/test_pages/fingerprinting_and_cryptomining_and_cookies.html) and open the panel after switching between them.

On Nightly, you can see the items rearranging after the popup is shown. With the patch, they are reordered before the popup opens.

Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/84744d179773
[Protections Panel] Reorder category items in popupshowing instead of shown. r=johannh

The failure was intermittent but high frequency (8/11 runs). The screenshot showed that the category item was marked .notFound, but was still disabled - i.e. reorderCategoryItems was never called. This means that the content blocking event was received between onpopupshowing and onpopupshown - since if it was called after the popup was shown, the items would have reordered immediately.

I made two try pushes: [1] in which the test waits for the category item order to be invalidated before opening the popup, and [2] that makes onContentBlockingEvent call reorderCategoryItems immediately in the showing state of the popup in addition to open.

If [2] works, we should land it. If not, I have high confidence that [1] will work.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0f05f46b983e8e2ad511133f13c6d468b4c0564
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a0278faa55d4844c05117bed487fec0db056015

Flags: needinfo?(nhnt11)

(In reply to Nihanth Subramanya [:nhnt11] from comment #5)

The failure was intermittent but high frequency (8/11 runs). The screenshot showed that the category item was marked .notFound, but was still disabled - i.e. reorderCategoryItems was never called. This means that the content blocking event was received between onpopupshowing and onpopupshown - since if it was called after the popup was shown, the items would have reordered immediately.

I made two try pushes: [1] in which the test waits for the category item order to be invalidated before opening the popup, and [2] that makes onContentBlockingEvent call reorderCategoryItems immediately in the showing state of the popup in addition to open.

If [2] works, we should land it. If not, I have high confidence that [1] will work.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0f05f46b983e8e2ad511133f13c6d468b4c0564
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a0278faa55d4844c05117bed487fec0db056015

[1] Works when the timing issue happens, but fails when the timing is normal (i.e. test now fails on non-QR).
[2] Seems to work as expected, waiting for retriggers to confirm.

Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b21608129eeb
[Protections Panel] Reorder category items in popupshowing instead of shown. r=johannh
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 71

Reproduced this bug using the steps from comment 2, on macOS 10.13 and Windows 10 x64. I did not observed the issue on Ubuntu 18.04 x64.

This is verified fixed on latest Nightly 71.0a1.

Status: RESOLVED → VERIFIED

Comment on attachment 9090811 [details]
Bug 1578459 - [Protections Panel] Reorder category items in popupshowing instead of shown. r=johannh

Beta/Release Uplift Approval Request

  • User impact if declined: UI changes layout while visible, resulting in a visual jump/flicker.
  • Is this code covered by automated tests?: No
  • 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): Small change, polish, verified in Nightly a while ago and working fine.
  • String changes made/needed:
Attachment #9090811 - Flags: approval-mozilla-beta?

Comment on attachment 9090811 [details]
Bug 1578459 - [Protections Panel] Reorder category items in popupshowing instead of shown. r=johannh

Fix for Skyline feature, verified in Nightly. Let's take this for beta 13.

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

Conflict while grafting:
merging browser/base/content/browser-siteProtections.js
merging browser/components/controlcenter/content/protectionsPanel.inc.xul
warning: conflicts while merging browser/base/content/browser-siteProtections.js! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

<<<<<<< local
this._categoryItemOrderInvalidated = true;
if (["showing", "open"].includes(this._protectionsPopup.state)) {
this.reorderCategoryItems();
}

if (anyDetected) {
  this.noTrackersDetectedDescription.hidden = true;
}

graft
// Check whether the user has added an exception for this site.
let hasException = ContentBlockingAllowList.includes(
gBrowser.selectedBrowser
);

Flags: needinfo?(nhnt11)

Noemi: For this conflict, we need to keep the local version of the hunk. I'm on PTO today but please let me know if you need me to make a new rebased patch for beta. I can do that in the morning. Not sure about the process - i.e. whether you can resolve it based on what I said or if you need a new patch from me.

Flags: needinfo?(nhnt11) → needinfo?(nerli)

Nihanth, here's a better look at the conflict: https://pastebin.com/raw/QaYu5A52.
By keeping the local version you mean discard all the lines after ======= ?

Flags: needinfo?(nerli) → needinfo?(nhnt11)

Backed out for failing browser-chrome's browser_trackingUI_trackers_subview.js:

https://hg.mozilla.org/releases/mozilla-beta/rev/31e07561bea62b5452de74decb006bb4e38e6e73

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&group_state=expanded&selectedJob=269743614&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Cretry%2Csuperseded&revision=06ddf8fc87f7d6edae3eed1101cb82c1f8f47409

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=269743614&repo=mozilla-beta
[task 2019-10-04T09:04:27.426Z] 09:04:27 INFO - TEST-PASS | browser/base/content/test/trackingUI/browser_trackingUI_trackers_subview.js | Indicates whether the tracker was blocked or allowed -
[task 2019-10-04T09:04:27.426Z] 09:04:27 INFO - Received onSecurityChange event 1 of 1
[task 2019-10-04T09:04:27.427Z] 09:04:27 INFO - Testing trackers subview with TP enabled.
[task 2019-10-04T09:04:27.429Z] 09:04:27 INFO - Buffered messages logged at 09:03:01
[task 2019-10-04T09:04:27.430Z] 09:04:27 INFO - Console message: [JavaScript Warning: "The resource at “http://trackertest.org/” was blocked because content blocking is enabled." {file: "http://tracking.example.org/browser/browser/base/content/test/trackingUI/trackingPage.html" line: 0}]
[task 2019-10-04T09:04:27.431Z] 09:04:27 INFO - TEST-PASS | browser/base/content/test/trackingUI/browser_trackingUI_trackers_subview.js | TP category item is visible -
[task 2019-10-04T09:04:27.432Z] 09:04:27 INFO - Buffered messages finished
[task 2019-10-04T09:04:27.433Z] 09:04:27 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/trackingUI/browser_trackingUI_trackers_subview.js | Test timed out -

Confirmed locally that the old patch fails tests on my debug build and the new patch passes.

Flags: needinfo?(nhnt11)
You need to log in before you can comment on or make changes to this bug.