Closed Bug 1582339 Opened 5 years ago Closed 5 years ago

Let tip results hide the help button

Categories

(Firefox :: Address Bar, enhancement, P3)

enhancement
Points:
3

Tracking

()

RESOLVED FIXED
Firefox 72
Iteration:
72.1 - Oct 21 - Nov 3
Tracking Status
firefox70 --- unaffected
firefox71 --- fixed
firefox72 --- fixed

People

(Reporter: adw, Assigned: bugzilla)

References

Details

Attachments

(2 files)

See the discussion in https://phabricator.services.mozilla.com/D46254#1403052. If we were to use tip results to implement nudges, we'd need to be able to hide the help button since the nudges spec doesn't include it. So we want to support three cases:

  • No help button
  • Help button with a URL
  • Help button without a URL

We could keep the helpUrl bool in the payload and add a showHelp bool.

This isn't a high priority right now since we don't need this for interventions.

Blocks: 1568594

Now that I think about this a little more, is there any situation when we would want a help button without a URL? Seeing as tips would only be shown in fairly specific contexts, I think we'd want the help button to act consistently across those contexts. This three-state toggle adds potentially unnecessary complexity. Drew, what do you think of just making this behave like you initially suggested in the Phabricator thread? That is, making helpUrl an optional property and not showing the help button when helpUrl is not present.

It would be relatively easy to add the three-state toggle in the future if we really need it, but in the meantime it makes the payload more complex while we run tip experiments.

Flags: needinfo?(adw)

This is fine with me, but this patch doesn't address the code path where the help button is picked. Right now, UrlbarInput.pickElement doesn't assume the help button has a URL, so that could probably be simplified a very little bit. And it passes a details object to UrlbarProvider.pickResult that indicates whether the help button was picked. (details.helpPicked is true if the user picked the help button and it doesn't have a URL.) The webext API also has the details object (via the onResultPicked event). There's no need for that with this change.

We could just leave all that in place -- especially the webext part, so we don't need to modify the webext API again and so that it's more future proof. I don't know though. I guess ideally this patch would remove the details object since we no longer have a use case for it. (We probably never did...)

What do you think?

You'll need to at least modify https://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_urlbar.js. There are two test tasks that don't specify a help URL but click the help button and check details.helpPicked.

Flags: needinfo?(adw)

I think we should remove the details object while we don't have a use case for it. We might need it if we were to implement a weather-widget-type-thing in the future, but we don't know how long it would be until we work on something like that. Even with that said, would a weather widget really allow selecting, say, each individual day of the week, necessitating details to distinguish them? Seems unlikely. Even in that case, the weather-payload (or whatever) could just carry a URL for each day of the week, much like the TIP help button. No details needed...

I've updated the revision to remove details. I just can't picture a result type with more than one selectable area that does not just lead to a URL; nor do I think we'd want one, seeing as users would have to wade through all those options in one result.

Attachment #9102606 - Attachment description: Bug 1582339 - Let tip results hide the help button. r?adw → Bug 1582339 - Let tip results hide the help button and remove the details object from onResultPicked. r?adw

Sounds good.

Assignee: nobody → htwyford
Status: NEW → ASSIGNED
Iteration: --- → 72.1 - Oct 21 - Nov 3
Points: --- → 3

I'll go ahead and land this since Harry is away for a bit. First, a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d88af4aa43e74ae4c44c07a0454110e37821b5f0

Pushed by dwillcoxon@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/010fd924d606 Let tip results hide the help button and remove the details object from onResultPicked. r=adw
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 72
Flags: qe-verify-
Flags: in-testsuite+
Attached patch Beta 71 patch (deleted) — Splinter Review

Beta/Release Uplift Approval Request

  • User impact if declined: We'd like the option of running quantumbar experiments on 71 that require this patch (bug 1564506, bug 1568594). Otherwise we'd need to wait until 72.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • 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 core of this patch is fairly small and most of it affects only tip results in the urlbar. Tip results would only be shown by experiment extensions and are not shown in normal Firefox. It has tests. Here's a try push on beta that shows no related failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=82e8ab4a8061ef1a6d5d8f2949eea1d88364cc8d
  • String changes made/needed:
Attachment #9106780 - Flags: approval-mozilla-beta?
Comment on attachment 9106780 [details] [diff] [review] Beta 71 patch Low risk, has tests and was on nightly for almost two weeks, uplift approved for 71 beta 8.
Attachment #9106780 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: