1042199 - Widget for searching from error pages

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

If you type something like window.open into the urlbar, you'll get an error page saying its a bad url. Ignoring that we should know that and search instead, we can offer a simple search box on the error page itself.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: WIP Patch

Splitting up my old patch and adjusting to the new API to find holes in it.

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Screenshot

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Patch

We have to do some jiggling here to get the user entered search terms, but this works pretty well. As a bonus, tapping the urlbar after you hit an error page also shows the user entered term instead of the fixed up uri, which is nice.

Comment 4

[:Margaret Leibovic]

Comment on attachment 8477750 [details] [diff] [review]
Patch

Review of attachment 8477750 [details] [diff] [review]:
-----------------------------------------------------------------

From looking at this code (but without testing it), I think it will break the behavior that was originally added in bug 823230. It pains me to say it, but I think we might need another variable to keep track of "whatever the user entered".

This userSearch term is used to show search terms in the urlbar when the user did a search. This bit that was added here is for the case where the user taps on a search suggestion, but the case where they typed a search is handled here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#1616

By setting userSearch for anything that flows through the "Tab:Load" handler, users wouldn't be able to edit URLs that were loaded. I see you worked around that by resetting it on DOMContentLoaded instead of onLoacationChange, but by doing this, we won't be able to show it to the user when they tap on the urlbar, which is the original reason we added this in bug 823230.

I think the correct way to do this might be to turn userEntered into a string instead of a boolean, and use that to keep track of what the user typed in the urlbar, whether that's a search or a mangled URL.

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch

So I changed this to use two fields, a userTyped one that holds what they typed, and an isSearch boolean that keeps track of how a url was created. The userTyped one is cleared in DOMContentLoaded IFF this wasn't a search and it isn't an error page. We then send it to Java to update the record there as well.

isSearch is cleared after every DOMContentLoaded so that userSearch won't be kept for the next. This seems to work fairly well for me.

Comment 6

[:Margaret Leibovic]

Comment on attachment 8480871 [details] [diff] [review]
Patch

Review of attachment 8480871 [details] [diff] [review]:
-----------------------------------------------------------------

I want to think about this a bit more... it's not your fault that this code is so tricky to follow (actually it's probably my fault), but I'm still nervous we're going to end up breaking something. I guess we could always try our best to make it right, but then watch closely for regressions.

I would say we should wait until after the merge to land this, but it would be hard to uplift because of the strings. Is this something we'd really like to see in 34? I suppose we can land it and just back it out of Aurora if it causes any issues.

::: mobile/android/chrome/content/browser.js
@@ +952,5 @@
>  
>      let tab = this.getTabForBrowser(aBrowser);
>      if (tab) {
> +      if ("userTyped" in aParams) tab.userTyped = aParams.userTyped;
> +      tab.isSearch = aParams.isSearch;

My OCD makes me want to add a similar if statement to check if isSearch is in params before setting tab.isSearch. Either that, or explicitly set it to false if it's not in the params.

@@ +1576,5 @@
>            delayLoad: (delayLoad === true),
>            desktopMode: (data.desktopMode === true)
>          };
>  
> +        params.userTyped = url;

I still don't think this is quite right, since there are a lot of calls that go through here that aren't the user typing something.

For example, toggling reader mode:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tab.java#497

Or even loading an external URL.

But maybe we still want to let the user do a search for any busted URL? But if that's the case, I don't think we should be calling this "userTyped".

@@ +1616,5 @@
>        case "keyword-search":
>          // This event refers to a search via the URL bar, not a bookmarks
>          // keyword search. Note that this code assumes that the user can only
>          // perform a keyword search on the selected tab.
> +        this.selectedTab.userTyped = aData;

I don't think you need to do this in here anymore if you're storing every entry the user tries to load. The reason we currently have this logic here is that we don't know something is a search when the user enters it in the urlbar, so we wait until we get this notification to say "oh, yes, that is a search, let's store it".

@@ -4269,5 @@
>  
>      sendMessageToJava(message);
>  
> -    // The search term is only valid for this location change event, so reset it here.
> -    this.userSearch = "";

In the case that this was a search, I believe we'd still need to reset userTyped here, since it wasn't reset earlier in the DOMContentLoaded handler.

::: mobile/locales/en-US/overrides/netError.dtd
@@ +24,5 @@
>      <strong>www</strong>.example.com</li>
> +    <div id='searchbox'>
> +      <input id='searchtext' type='search'></input>
> +      <button id='searchbutton'>Search</button>
> +    </div>

You should probably add some localization notes here. The fact that we have HTML mark-up in dtd files is a bit unfortunate :/

Comment 7

[Wesley Johnston (:wesj)]

Planning to land these patches in 35. No hurry to do it now.

Comment 8

[Wesley Johnston (:wesj)]

(In reply to :Margaret Leibovic from comment #6)
> I still don't think this is quite right, since there are a lot of calls that
> go through here that aren't the user typing something.
> 
> For example, toggling reader mode:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tab.
> java#497
> 
> Or even loading an external URL.
> 
> But maybe we still want to let the user do a search for any busted URL? But
> if that's the case, I don't think we should be calling this "userTyped".

I am out of ideas then. I switched things to requestedURL at one point, but they may not be URL's and I didn't want to confuse someone who assumed they'd get a url back. Maybe just "userRequested"? or "requested"?

Comment 9

[:Margaret Leibovic]

(In reply to Wesley Johnston (:wesj) from comment #8)
> (In reply to :Margaret Leibovic from comment #6)
> > I still don't think this is quite right, since there are a lot of calls that
> > go through here that aren't the user typing something.
> > 
> > For example, toggling reader mode:
> > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tab.
> > java#497
> > 
> > Or even loading an external URL.
> > 
> > But maybe we still want to let the user do a search for any busted URL? But
> > if that's the case, I don't think we should be calling this "userTyped".
> 
> I am out of ideas then. I switched things to requestedURL at one point, but
> they may not be URL's and I didn't want to confuse someone who assumed
> they'd get a url back. Maybe just "userRequested"? or "requested"?

I think userRequested is better than userTyped, especially since I think userTyped is used in desktop Firefox to keep track of what the user actually typed.

If you address my other comments about where we set the value of this variable, I think I'd be ready to give it an r+. I'm hesitant to add the extra complexity, but I'm not sure how to avoid it if we want this functionality.

Let's also add lots of comments to help our future selves when we want to change this in a year ;)

Comment 10

[Wesley Johnston (:wesj)]

Attachment: Patch

Comment 11

[Wesley Johnston (:wesj)]

Attachment: Patch

qref problems. Splitting off the tests because there are some weird failures I don't understand.

Comment 12

[Wesley Johnston (:wesj)]

Attachment: Tests

Tests. Just makes sure the url in java is set to the right thing. Doesn't test the error page yet. I had to modify assertUrl because getText() returns an Editable, not a String. The compares were failing. I also had to change dismissEditingMode for unknown reasons. Clicking the cancel button was just ending the tests entirely. You have any idea why that would happen?

Comment 13

[:Margaret Leibovic]

Comment on attachment 8485195 [details] [diff] [review]
Patch

Review of attachment 8485195 [details] [diff] [review]:
-----------------------------------------------------------------

This looks pretty good, I just have a few more questions.

::: mobile/android/base/Tabs.java
@@ +487,5 @@
>                  tab.handleContentLoaded();
>                  notifyListeners(tab, Tabs.TabEvents.LOAD_ERROR);
>              } else if (event.equals("Content:PageShow")) {
>                  notifyListeners(tab, TabEvents.PAGE_SHOW);
> +                tab.updateUserRequested(message.getString("userRequested"));

Why do we need to update this in Java here now?

::: mobile/android/chrome/content/browser.js
@@ +3799,5 @@
> +          errorType: errorType,
> +          metadata: this.metatags,
> +        });
> +
> +        // Reset isSearch so that the userRequested term will be erased on next page load

I think this comment was intended for a different line.

@@ +4080,4 @@
>          sendMessageToJava({
>            type: "Content:PageShow",
> +          tabID: this.id,
> +          userRequested: this.userRequested

As I mentioned before, I'm not sure why you need to pass this along to Java, since the new use for this value is a JS-only thing.

@@ -4260,5 @@
>  
>      sendMessageToJava(message);
>  
> -    // The search term is only valid for this location change event, so reset it here.
> -    this.userSearch = "";

Why did you move this to pageshow (in the form of this.isSearch = false)? Won't this break showing the search term in the urlbar when the user taps on it again?

(I tried applying this to test myself, but it didn't apply.)

Comment 14

[Wesley Johnston (:wesj)]

(In reply to :Margaret Leibovic from comment #13)
> (I tried applying this to test myself, but it didn't apply.)
Yes. Its a one line change from the recent Messaging.jsm changes.

I don't understand all your comments, but I wanted the urlbar to continue to reflect the user typed term for searches and error pages. i.e. if the page loads unsuccessfully or was a search, we need to tell java the string to show (we still do this in onLocationChange, and that should continue to work). After the page loads successfully (and isn't a search) we need to erase the userRequested term in Java so that the urlbar shows the current url. That's what this does. That used to happen in LocationChange, but we don't know about the error page state there. This is the simple way to do that, and as a benefit I anticipate some set of users won't even see their textbox and will immediately tap the urlbar on receiving an error. Pre-filling it with their last typed term (instead of 'http://window.open' for instance) goes along with the bugs goal of making it easy to fixup the url/search.

PageShow is better for this because it fires even for hitting the back(/forward) button (fast back/forward means DOMContentLoaded may not fire when you do this).

Comment 15

[:Margaret Leibovic]

(In reply to Wesley Johnston (:wesj) from comment #14)
> (In reply to :Margaret Leibovic from comment #13)
> > (I tried applying this to test myself, but it didn't apply.)
> Yes. Its a one line change from the recent Messaging.jsm changes.
> 
> I don't understand all your comments, but I wanted the urlbar to continue to
> reflect the user typed term for searches and error pages. i.e. if the page
> loads unsuccessfully or was a search, we need to tell java the string to
> show (we still do this in onLocationChange, and that should continue to
> work).

Okay. It seems a bit out of the scope of this bug to change the urlbar behavior, but I can see it makes sense for improving the error state case.

> After the page loads successfully (and isn't a search) we need to
> erase the userRequested term in Java so that the urlbar shows the current
> url. That's what this does. That used to happen in LocationChange, but we
> don't know about the error page state there. This is the simple way to do
> that, and as a benefit I anticipate some set of users won't even see their
> textbox and will immediately tap the urlbar on receiving an error.
> Pre-filling it with their last typed term (instead of 'http://window.open'
> for instance) goes along with the bugs goal of making it easy to fixup the
> url/search.

Okay. I just wanted to make sure we weren't accidentally resetting the search case as well.

> PageShow is better for this because it fires even for hitting the
> back(/forward) button (fast back/forward means DOMContentLoaded may not fire
> when you do this).

Good call.

Comment 16

[:Margaret Leibovic]

Comment on attachment 8485195 [details] [diff] [review]
Patch

Review of attachment 8485195 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the drawn out review process, but hopefully thinking about this a lot will mean lower risk of regressions :)

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8485197 [details] [diff] [review]
Tests

Review of attachment 8485197 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay! Also, I accidentally did this as in-depth as a review - sorry for the extra barrage of comments!

Nice catch on the String comparisons.

(In reply to Wesley Johnston (:wesj) from comment #12)
> I also had to change dismissEditingMode for unknown reasons.
> Clicking the cancel button was just ending the tests entirely.
> You have any idea why that would happen?

Maybe it's a regression, to be fixed by bug 1063914?

::: mobile/android/base/tests/robocop.ini
@@ +1,1 @@
> +[testUserRequested]

nit: alphabetize as best as you can. Not sure why testGeckoProfile is up there.

::: mobile/android/base/tests/testUserRequested.java
@@ +3,5 @@
> +import org.mozilla.gecko.Actions;
> +import org.mozilla.gecko.tests.helpers.GeckoHelper;
> +import org.mozilla.gecko.tests.helpers.NavigationHelper;
> +
> +/* Tests that the urlbar contains the correct words after

You don't mean the title here but the actual URL? I would clarify that.

Also, to avoid adding extra tests (and thus the amount of time it takes to run the test suite), I wonder if this shouldn't be part of testSessionHistory - what do you think? I think it's different enough that I'm on the edge of whether it makes sense or not.

@@ +7,5 @@
> +/* Tests that the urlbar contains the correct words after
> + * typing in an invalid url. */
> +public class testUserRequested extends UITest {
> +
> +    private static final String TEST_URL = "test.test";

Is this the invalid URL? I would name the variable to make this apparent.

@@ +17,5 @@
> +        mToolbar.enterEditingMode()
> +                .assertUrl(getAbsoluteHostnameUrl(StringHelper.ROBOCOP_BLANK_PAGE_01_URL))
> +                .dismissEditingMode();
> +
> +        // NavigationHelper will try to clean up our URL, so we manually enter one here.

What do you mean by this?

Comment 18

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/a4b979000940

I'll try to get back to the test soon....

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a4b979000940

Comment 20

[Kevin Brosnan [Ex-Mozilla]]

Release Note Request (optional, but appreciated)
[Why is this notable]: New feature
[Suggested wording]: Search dialog added to network error pages
[Links (documentation, blog post, etc)]: this bug

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Added to the release notes with "Search dialog added to network error pages" as wording. thanks

Comment 22

[Cristina Madaras, QA [:CristinaM]]

Verified as fixed in latest builds:
Firefox for Android 37.0a1 (2014-12-09)
Firefox for Android 36.0a2 (2014-12-09)
Firefox for Android 35.b1

Device: Asus Transformer Pad TF300T (Android 4.2.1)