1582339 - Let tip results hide the help button

Status: RESOLVED FIXED

(Firefox :: Address Bar, enhancement, P3)

Description

[Drew Willcoxon :adw]

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.

Comment 1

[Harry Twyford]

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.

Comment 2

[Harry Twyford]

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

Comment 3

[Drew Willcoxon :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.

Comment 4

[Harry Twyford]

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.

Comment 5

[Drew Willcoxon :adw]

Sounds good.

Comment 6

[Drew Willcoxon :adw]

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/010fd924d606
Let tip results hide the help button and remove the details object from onResultPicked. r=adw

Comment 8

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/010fd924d606

Comment 9

[Drew Willcoxon :adw]

Beta try push in preparation for an uplift request: https://treeherder.mozilla.org/#/jobs?repo=try&revision=82e8ab4a8061ef1a6d5d8f2949eea1d88364cc8d

Comment 10

[Drew Willcoxon :adw]

Attachment: Beta 71 patch

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:

Comment 11

[Pascal Chevrel:pascalc (PTO until April 26)]

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.

Comment 12

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fcafde7c2e47