Closed Bug 1042199 Opened 6 years ago Closed 5 years ago

Widget for searching from error pages

Categories

(Firefox for Android :: General, defect)

x86_64
Linux
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 35
Tracking Status
firefox35 --- verified
firefox36 --- verified
firefox37 --- verified
relnote-firefox --- 35+

People

(Reporter: wesj, Assigned: wesj)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(3 files, 4 obsolete files)

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.
Blocks: 940453
Attached patch WIP Patch (obsolete) — Splinter Review
Splitting up my old patch and adjusting to the new API to find holes in it.
Attached image Screenshot
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #8460515 - Attachment is obsolete: true
Attachment #8477750 - Flags: review?(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.
Attachment #8477750 - Flags: review?(margaret.leibovic) → review-
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #8477750 - Attachment is obsolete: true
Attachment #8480871 - Flags: review?(margaret.leibovic)
Assignee: nobody → wjohnston
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 :/
Planning to land these patches in 35. No hurry to do it now.
(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"?
(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 ;)
Attachment #8480871 - Flags: review?(margaret.leibovic) → feedback+
Attached patch Patch (obsolete) — Splinter Review
Attachment #8480871 - Attachment is obsolete: true
Attachment #8485193 - Flags: review?(margaret.leibovic)
Attached patch PatchSplinter Review
qref problems. Splitting off the tests because there are some weird failures I don't understand.
Attachment #8485193 - Attachment is obsolete: true
Attachment #8485193 - Flags: review?(margaret.leibovic)
Attachment #8485195 - Flags: review?(margaret.leibovic)
Attached patch TestsSplinter Review
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?
Attachment #8485197 - Flags: feedback?(michael.l.comella)
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.)
(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).
(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 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 :)
Attachment #8485195 - Flags: review?(margaret.leibovic) → review+
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?
Attachment #8485197 - Flags: feedback?(michael.l.comella) → feedback+
https://hg.mozilla.org/integration/fx-team/rev/a4b979000940

I'll try to get back to the test soon....
https://hg.mozilla.org/mozilla-central/rev/a4b979000940
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Depends on: 1071076
Depends on: 1072732
Depends on: 1073776
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
relnote-firefox: --- → ?
Added to the release notes with "Search dialog added to network error pages" as wording. thanks
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)
Status: RESOLVED → VERIFIED
Depends on: 1293226
You need to log in before you can comment on or make changes to this bug.