1964736 - Search terms go through the URL formatter

Status: RESOLVED INVALID

(Firefox for Android :: Toolbar, defect)

Description

[Michel Le Bihan]

When using search, the URL is set to the search term in https://searchfox.org/mozilla-central/source/mobile/android/android-components/components/feature/toolbar/src/main/java/mozilla/components/feature/toolbar/ToolbarPresenter.kt#64 and goes through https://searchfox.org/mozilla-central/source/mobile/android/android-components/components/browser/toolbar/src/main/java/mozilla/components/browser/toolbar/BrowserToolbar.kt#120 and through the URL formatter (URLStringUtils.toDisplayUrl) in https://searchfox.org/mozilla-central/source/mobile/android/android-components/components/browser/toolbar/src/main/java/mozilla/components/browser/toolbar/display/DisplayToolbar.kt#524

Expected behavior

Search terms should not go through the URL formatter. Maybe the URL fomatter should be integrated into the URLRenderer.

Actual behavior

Search terms are passed through the URL formatter that modifies the way in which they are displayed.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:royang, could you have a look please?

For more information, please visit BugBot documentation.

Comment 2

[Michel Le Bihan]

You can see this issue by following those steps:

1. Enter http:// URI (with the space) in the toolbar
2. Notice that URI is displayed and not the full search term.

Comment 3

[Michel Le Bihan]

Could I get any guidelines on how I should proceed with this issue?

Comment 4

[Petru-Mugurel Lingurar [:petru]]

Agree that this not already being handled in the URLRenderer is a missed opportunity.
Right now, given that we're working to soon replace the toolbar view with a toolbar composable
I think the most straightforward fix would be to expose a new toolbar API for Fenix to use for this specific scenario.

Why:

• search terms are not URLs but rather just the text/query of the previous search in the current tab, see bug 1805164
• since they are not an URL they don't need to go through the urlFormatter.

What:

• expose a new text (is this a good name?) API from Toolbar, similar to the current url API.
• this will need a corresponding text property in the DisplayToolbar similar to the current url property.
• to limit change and since OriginView is marked as displaying an URL I would suggest not doing any changes here and just using it's url API exactly as it is to ultimately display the given text.

@Michel does this makes sense?
Also I think I saw this being marked as a potential fix for another ticket, could you add that to the See also of this ticket?

Comment 5

[Michel Le Bihan]

That seems like a good way to resolve this. However, how would cases where some other code sets text and then gets url or the other way around be handled. Obviously, that shouldn't happen but could due to a bug somewhere else.

Comment 6

[Petru-Mugurel Lingurar [:petru]]

I agree that there is a risk in integrators not using the "right" APIs but they would be in full control of the code.
With good documentation, a little attention and some tests (automated + manual) related issue should be avoided.

Think here we want a quick, straightforward fix to test the improvements we expect and then use this as learnings for how to support the same behaviour in the new toolbar based on Jetpack Compose that is soon to replace the toolbar View.

Comment 7

[Simon Friedberger [:simonf]]

I am not sure if it should be included here but as discussed in bug 1969937 it would be nice if Android could reuse the code from desktop for not displaying search terms when they might look like URLs.

Comment 8

[Petru-Mugurel Lingurar [:petru]]

(In reply to Simon Friedberger (:simonf) from comment #7)

I am not sure if it should be included here but as discussed in bug 1969937 it would be nice if Android could reuse the code from desktop for not displaying search terms when they might look like URLs.

That seems like a massive project in itself.
Created bug 1978517 to handle discussions and possible work for it.

Comment 9

[Moyin [:moyin]]

Attachment: share_3835862427478970657.png

The issue reported has been resolved in the composable toolbar (please see the attached screenshot). I'll close this ticket, but please reopen it if the issue still exists.