Closed Bug 1756162 Opened 2 years ago Closed 2 years ago

The "Top Pick" option is available by default on non US regions even if Best Match is disabled

Categories

(Firefox :: Address Bar, defect, P1)

Product:
Firefox
Component:
Address Bar
Platform:
Desktop
All
Type:
defect
Points:
2

Tracking

()

Status:
VERIFIED FIXED
Milestone:
99 Branch
Tracking Flags:
Tracking Status
firefox97 --- unaffected
firefox98 --- unaffected
firefox99 --- verified

People

(Reporter: cmuntean, Assigned: adw)

References

Details

(Keywords: perf-alert)

Attachments

(2 files)

Top Pick option available in non US by default.mov
467.03 KB, video/quicktime
Details
Bug 1756162 - Make the visibility of the "Top Pick" checkbox depend entirely on whether the best match feature is enabled.
48 bytes, text/x-phabricator-request
Details | Review

[Affected Versions]:

  • Nightly 99.0a1 (Build ID: 20220218090822);

[Affected Platforms]:

  • Windows 10 x64
  • Ubuntu 20.04 x64
  • macOS 10.15.7

[Prerequisites]:

  • Have a new Firefox profile with the following pref set:
  • Have the "browser.search.region" pref set to a non-US region, eg: RO.

[Steps to reproduce]:

  1. Open the Firefox browser with the profile from prerequisites.
  2. Navigate to "about:preferences#privacy" page.
  3. Scroll down to "Address Bar - FIrefox Suggest" section.
  4. Observe the available options.

[Expected result]:

  • The "Top Pick" option is NOT available.

[Actual result]:

  • The "Top Pick" option is available and it is checked.

[Notes]:

  • Attached is a screen recording of the issue.
Assignee: nobody → adw
Status: NEW → ASSIGNED
Points: --- → 2
Priority: -- → P1

We've made a few changes to this "Top pick" checkbox recently based on shifting
Product requirements, and the problem here is that the checkbox used to be
inside the Firefox Suggest container, but we recently moved it outside the
container. The Firefox Suggest container is properly hidden or shown depending
on whether the Suggest feature is enabled, so when the checkbox was inside the
container and Suggest was disabled, the checkbox properly got hidden too. Now
that the checkbox is outside that container, its visibility needs to entirely
depend on whether the best match feature is enabled.

So all this revision does is move the checkbox's hidden assignment from inside
the "is Suggest enabled" if-block to outside the if-block. It also sets
hidden=true on the checkbox in the markup for good measure.

I also improved the test so it checks every combination of enabled statuses
between the Suggest and best match features.

Depends on D138987

Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/15e745e8ec74
Make the visibility of the "Top Pick" checkbox depend entirely on whether the best match feature is enabled. r=preferences-reviewers,Gijs
Flags: qe-verify+
Flags: in-testsuite+

Backed out for causing mochitest failures on browser_privacy_firefoxSuggest.js.

Push with failures

Failure log

Backout link

[task 2022-02-23T01:28:54.219Z] 01:28:54     INFO - TEST-PASS | browser/components/preferences/tests/browser_privacy_firefoxSuggest.js | Learn-more link is visible - true == true - 
[task 2022-02-23T01:28:54.219Z] 01:28:54     INFO - Clicking learn-more link
[task 2022-02-23T01:28:54.219Z] 01:28:54     INFO - Buffered messages logged at 01:28:52
[task 2022-02-23T01:28:54.219Z] 01:28:54     INFO - Waiting for help page to load in a new tab
[task 2022-02-23T01:28:54.219Z] 01:28:54     INFO - Buffered messages logged at 01:28:53
[task 2022-02-23T01:28:54.219Z] 01:28:54     INFO - Console message: [JavaScript Warning: "This page is in Quirks Mode. Page layout may be impacted. For Standards Mode use “<!DOCTYPE html>”." {file: "http://127.0.0.1:8888/support-dummy/top-pick" line: 0}]
[task 2022-02-23T01:28:54.220Z] 01:28:54     INFO - Leaving test bound clickBestMatchLearnMore
[task 2022-02-23T01:28:54.220Z] 01:28:54     INFO - Buffered messages finished
[task 2022-02-23T01:28:54.220Z] 01:28:54     INFO - TEST-UNEXPECTED-FAIL | browser/components/preferences/tests/browser_privacy_firefoxSuggest.js | This test exceeded the timeout threshold. It should be rewritten or split up. If that's not possible, use requestLongerTimeout(N), but only as a last resort. - 
[task 2022-02-23T01:28:54.220Z] 01:28:54     INFO - GECKO(1587) | MEMORY STAT | vsize 130561027MB | residentFast 4483MB
[task 2022-02-23T01:28:54.221Z] 01:28:54     INFO - TEST-OK | browser/components/preferences/tests/browser_privacy_firefoxSuggest.js | took 136792ms
[task 2022-02-23T01:28:54.221Z] 01:28:54     INFO - checking window state
[task 2022-02-23T01:28:54.221Z] 01:28:54     INFO - TEST-START | browser/components/preferences/tests/browser_privacy_passwordGenerationAndAutofill.js
Flags: needinfo?(adw)
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2316e8b1ed0e
Make the visibility of the "Top Pick" checkbox depend entirely on whether the best match feature is enabled. r=preferences-reviewers,Gijs

https://hg.mozilla.org/mozilla-central/rev/2316e8b1ed0e

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
status-firefox99: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → 99 Branch

Hi Drew! I have verified this issue, but I can still reproduce it. When I start a new Firefox profile the "browser.search.region" pref is set to "RO" by default and I still have the "Top Pick" option available in the "about:preferences#privacy" page. I have also checked the prefs and the "browser.urlbar.bestMatch.enabled" is set to false.

Is there a chance that the patch didn't land in the latest Nightly build yet or the fix is not completed yet?

I updated the Phabricator revision with the wrong final diff. What landed in https://hg.mozilla.org/integration/autoland/rev/2316e8b1ed0e was just a change to the requested timeout in the test. The actual fix didn't land at all. :-(

I'll ask sheriffs to back this out again and then I'll land the proper diff.

Thanks Cosmin.

Flags: needinfo?(adw)

Backed out on @Drew's request

Backout link: https://hg.mozilla.org/integration/autoland/rev/a1ee11df1dc9664c570cd3a06723628efd2eee77

Status: RESOLVED → REOPENED
status-firefox99: fixed → ---
Resolution: FIXED → ---
Target Milestone: 99 Branch → ---
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/536fddf7da05
Make the visibility of the "Top Pick" checkbox depend entirely on whether the best match feature is enabled. r=preferences-reviewers,Gijs

https://hg.mozilla.org/mozilla-central/rev/536fddf7da05

Status: REOPENED → RESOLVED
Closed: 2 years ago → 2 years ago
status-firefox99: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 99 Branch

I have verified this issue on the latest Nightly 99.0a1 build (Build ID: ) on Windows 10 x64, macOS 10.15.7 and Linux Mint 20.

  • The "Top Pick" option is no longer available while the feature is disabled.
Status: RESOLVED → VERIFIED
status-firefox99: fixed → verified
Flags: qe-verify+

(In reply to Sandor Molnar from comment #8)

Backed out on @Drew's request

Backout link: https://hg.mozilla.org/integration/autoland/rev/a1ee11df1dc9664c570cd3a06723628efd2eee77

== Change summary for alert #33531 (as of Tue, 15 Mar 2022 08:47:45 GMT) ==

Improvements:

Ratio Test Platform Options Absolute values (old vs new)
18% office fnbpaint macosx1015-64-shippable-qr cold fission webrender 396.32 -> 323.88
15% office fnbpaint macosx1015-64-shippable-qr cold fission webrender 396.00 -> 338.33
10% office FirstVisualChange macosx1015-64-shippable-qr cold fission webrender 578.67 -> 523.33
9% office PerceptualSpeedIndex macosx1015-64-shippable-qr cold fission webrender 617.67 -> 564.67
8% office SpeedIndex macosx1015-64-shippable-qr cold fission webrender 606.93 -> 556.00
8% office ContentfulSpeedIndex macosx1015-64-shippable-qr cold fission webrender 802.08 -> 740.17

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=33531

You need to log in before you can comment on or make changes to this bug.