Closed Bug 1432716 Opened 6 years ago Closed 6 years ago

"Show search suggestions ahead of browsing history in address bar results" checkbox shouldn't be in the Search Bar groupbox

Categories

(Firefox :: Settings UI, defect, P1)

Product:
Firefox
Component:
Settings UI
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox58 --- unaffected
firefox59 + verified
firefox60 + verified

People

(Reporter: dao, Assigned: dao)

References

Details

Attachments

(1 file)

Bug 1432716 - Group the "Show search suggestions ahead of browsing history in address bar results" checkbox with other search suggestion checkboxes. ui=shorlander
59 bytes, text/x-review-board-request
adw
: review+
shorlander
: ui-review+
lizzard
: approval-mozilla-beta+
Details 
This checkbox is about the address bar, so putting it in the Search Bar groupbox hardly makes any sense. There are other checkboxes related to search suggestions in the Default Search Engine groupbox where this fits much better.
Attachment #8945023 - Flags: ui-review?(shorlander)
Javaun, you posted the mockup for this UI in bug 1426216 comment 0.  Could you please comment on Dao's change?

(In reply to Dão Gottwald [::dao] from comment #0)
> This checkbox is about the address bar, so putting it in the Search Bar
> groupbox hardly makes any sense. There are other checkboxes related to
> search suggestions in the Default Search Engine groupbox where this fits
> much better.
Flags: needinfo?(jmoradi)
Assigning a priority to get this off the triage list.
Priority: -- → P3
[Tracking Requested - why for this release]:
We should fix this before shipping and advertising this feature in 59 (bug 1426216 comment 29)
tracking-firefox59: --- → ?
If we want to fix this for 59, it probably can't be P3 (we would need to land and verify a fix in the next couple of weeks).
tracking-firefox59: ? → +
tracking-firefox60: --- → +
Priority: P3 → P1
Comment on attachment 8945023 [details]
Bug 1432716 - Group the "Show search suggestions ahead of browsing history in address bar results" checkbox with other search suggestion checkboxes. ui=shorlander

Looks good to me. It definitely makes more sense with that grouping. Thank you!


Just a note: This does do interesting things with the visual display of pref nesting/dependency. If we followed the pattern it would be indented, but indenting three rows of single line prefs doesn't look great.
Attachment #8945023 - Flags: ui-review?(shorlander) → ui-review+
Drew, I think UX is more Stephen's strong suit than Javaun's, with all due respect, so let's take the ui-r+ and continue the review process. Thanks!
Flags: needinfo?(jmoradi)
Comment on attachment 8945023 [details]
Bug 1432716 - Group the "Show search suggestions ahead of browsing history in address bar results" checkbox with other search suggestion checkboxes. ui=shorlander

https://reviewboard.mozilla.org/r/215222/#review224650
Attachment #8945023 - Flags: review?(adw) → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/98b4549200aa
Group the "Show search suggestions ahead of browsing history in address bar results" checkbox with other search suggestion checkboxes. ui=shorlander r=adw
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/58ad0c23fc93
Group the "Show search suggestions ahead of browsing history in address bar results" checkbox with other search suggestion checkboxes. ui=shorlander r=adw
https://hg.mozilla.org/mozilla-central/rev/58ad0c23fc93
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Flags: qe-verify+
Keywords: verifyme
Comment on attachment 8945023 [details]
Bug 1432716 - Group the "Show search suggestions ahead of browsing history in address bar results" checkbox with other search suggestion checkboxes. ui=shorlander

Approval Request Comment
[Feature/Bug causing the regression]: bug 1426216
[User impact if declined]: search suggestion checkboxes are confusing
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: STR: open about:preferences#search and play with the checkboxes
[List of other uplifts needed for the feature/fix]: /
[Is the change risky?]: not much
[Why is the change risky/not risky?]: mostly moving code around. The only slightly tricky part is that I had to adjust the JS logic so that this checkbox would get disabled depending on the other checkboxes.
[String changes made/needed]: /
Attachment #8945023 - Flags: approval-mozilla-beta?
Comment on attachment 8945023 [details]
Bug 1432716 - Group the "Show search suggestions ahead of browsing history in address bar results" checkbox with other search suggestion checkboxes. ui=shorlander

Makes sense, let's uplift this for beta 10. 
We should verify it in beta.
Attachment #8945023 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I have verified this on Firefox 60.0a1 (BuildId:20180213100127) using Windows 10 64bit, Ubuntu 16.04 64bit and macOS 10.13. 

Testing was performed around the "Show search suggestions ahead of browsing history in address bar results" button, checking both functionality over the address bar and if the button gets successfully disabled/enabled in accordance with the other checkboxes.

Leaving a ni on myself just for a reminder to verify this in 59.b10.
Status: RESOLVED → VERIFIED
status-firefox60: fixed → verified
Flags: needinfo?(emil.ghitta)
Keywords: verifyme
Verified using Firefox 59.0b10 (BuildId:20180215111455) on Windows 10 64bit, macOS 10.13 and Ubuntu 16.04 64bit.
status-firefox59: fixed → verified
Flags: qe-verify+
Flags: needinfo?(emil.ghitta)
You need to log in before you can comment on or make changes to this bug.