Let tip results hide the help button
Categories
(Firefox :: Address Bar, enhancement, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox70 | --- | unaffected |
firefox71 | --- | fixed |
firefox72 | --- | fixed |
People
(Reporter: adw, Assigned: bugzilla)
References
Details
Attachments
(2 files)
47 bytes,
text/x-phabricator-request
|
Details | Review | |
25.65 KB,
patch
|
pascalc
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•5 years ago
|
||
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.
Assignee | ||
Comment 2•5 years ago
|
||
Reporter | ||
Comment 3•5 years ago
|
||
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
.
Assignee | ||
Comment 4•5 years ago
|
||
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.
Updated•5 years ago
|
Reporter | ||
Comment 5•5 years ago
|
||
Sounds good.
Reporter | ||
Updated•5 years ago
|
Reporter | ||
Comment 6•5 years ago
|
||
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 8•5 years ago
|
||
bugherder |
Reporter | ||
Comment 9•5 years ago
|
||
Beta try push in preparation for an uplift request: https://treeherder.mozilla.org/#/jobs?repo=try&revision=82e8ab4a8061ef1a6d5d8f2949eea1d88364cc8d
Reporter | ||
Comment 10•5 years ago
|
||
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•5 years ago
|
||
Comment 12•5 years ago
|
||
bugherder uplift |
Description
•