Splitting this off from bug 917273. The current behavior for the "Edit" context menu item for pinned sites is different that what we currently ship in beta/aurora. In the old about:home, we'd pre-populate the edit interface (previous the awesomescreen) with the currently pinned term/url, but in the new about:home, we always launch an empty dialog. Is this intentional, or do we want to restore the old behavior?

(In reply to :Margaret Leibovic from comment #0) > Splitting this off from bug 917273. > > The current behavior for the "Edit" context menu item for pinned sites is > different that what we currently ship in beta/aurora. In the old about:home, > we'd pre-populate the edit interface (previous the awesomescreen) with the > currently pinned term/url, but in the new about:home, we always launch an > empty dialog. Is this intentional, or do we want to restore the old behavior? Well, neither actually... I would refer to the new designs in bug 931021: https://bugzilla.mozilla.org/attachment.cgi?id=822334

(In reply to Ian Barlow (:ibarlow) from comment #1) > (In reply to :Margaret Leibovic from comment #0) > > Splitting this off from bug 917273. > > > > The current behavior for the "Edit" context menu item for pinned sites is > > different that what we currently ship in beta/aurora. In the old about:home, > > we'd pre-populate the edit interface (previous the awesomescreen) with the > > currently pinned term/url, but in the new about:home, we always launch an > > empty dialog. Is this intentional, or do we want to restore the old behavior? > > Well, neither actually... I would refer to the new designs in bug 931021: > https://bugzilla.mozilla.org/attachment.cgi?id=822334 That sounds like a good plan for Fx28 and beyond, but is there anything we should do (without a string change) before we release the new about:home in 26? I feel like we should just keep the behavior consistent with the old about:home, but if it's not a big deal, maybe we should just leave it as is and focus our energy back on developing on m-c.

(In reply to :Margaret Leibovic from comment #2) > (In reply to Ian Barlow (:ibarlow) from comment #1) > > (In reply to :Margaret Leibovic from comment #0) > > > Splitting this off from bug 917273. > > > > > > The current behavior for the "Edit" context menu item for pinned sites is > > > different that what we currently ship in beta/aurora. In the old about:home, > > > we'd pre-populate the edit interface (previous the awesomescreen) with the > > > currently pinned term/url, but in the new about:home, we always launch an > > > empty dialog. Is this intentional, or do we want to restore the old behavior? > > > > Well, neither actually... I would refer to the new designs in bug 931021: > > https://bugzilla.mozilla.org/attachment.cgi?id=822334 > > That sounds like a good plan for Fx28 and beyond, but is there anything we > should do (without a string change) before we release the new about:home in > 26? I feel like we should just keep the behavior consistent with the old > about:home. I see what you mean now. I agree that if it's tiny amount of effort to fix, tweaking the interaction to leave the URL in the edit field would be better.

It probably wouldn't be hard, I can do it.

Well, this is actually more complicated than I thought... this patch puts the current url/search term in the pin site dialog, but it doesn't automatically open the keyboard.

Comment on attachment 824854 [details] [diff] [review] WIP Review of attachment 824854 [details] [diff] [review]: ----------------------------------------------------------------- I think this is probably fine. We should try something like: http://stackoverflow.com/questions/2403632/android-show-soft-keyboard-automatically-when-focus-is-on-an-edittext to show the keyboard. ::: mobile/android/base/home/PinSiteDialog.java @@ +162,5 @@ > + mSearch.setText(mInitialSearchTerm); > + mSearch.selectAll(); > + } else { > + // Otherwise use a default filter. > + filter(""); I think we want to filter (with an empty search) regardless? @@ +167,5 @@ > + } > + } > + > + public void setInitialSearchTerm(String searchTerm) { > + mInitialSearchTerm = searchTerm; If the dialog is already showing, do we want this to work? If so, maybe its better to just call it setSearchTerm(String) or setText(String)?

This patch incorporates your feedback, and it works well. I had to pull the logic to set the search term out of filter, otherwise the filter("") call would just overwrite the search term.

Comment on attachment 825587 [details] [diff] [review] patch Review of attachment 825587 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! Tests for this would be nice! ::: mobile/android/base/home/TopSitesPage.java @@ +338,5 @@ > } > > if (itemId == R.id.top_sites_edit) { > + // Decode "user-entered" URLs before showing them. > + mEditPinnedSiteListener.onEditPinnedSite(info.position, decodeUserEnteredUrl(info.url)); I wonder if we should do the filtering if its a userEntered search, and not if its a url...

(In reply to Wesley Johnston (:wesj) from comment #8) > Comment on attachment 825587 [details] [diff] [review] > patch > > Review of attachment 825587 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good! Tests for this would be nice! I agree. However, I think we should wait for bug 926394 to get finished first, since right now we don't have any tests that deal with this top sites grid. > ::: mobile/android/base/home/TopSitesPage.java > @@ +338,5 @@ > > } > > > > if (itemId == R.id.top_sites_edit) { > > + // Decode "user-entered" URLs before showing them. > > + mEditPinnedSiteListener.onEditPinnedSite(info.position, decodeUserEnteredUrl(info.url)); > > I wonder if we should do the filtering if its a userEntered search, and not > if its a url... For consistency I think it makes sense to just not filter in either case, especially since user-entered can also be a url.

Comment on attachment 825587 [details] [diff] [review] patch [Approval Request Comment] Bug caused by (feature/regressing bug #): new about:home User impact if declined: the dialog to edit a pinned site won't contain the current pinned url/search term, which is a regression from the old about:home Testing completed (on m-c, etc.): just landed on fx-team Risk to taking this patch (and alternatives if risky): change limited to edit pinned site dialog, but we should be sure to test keyboard interaction with different devices String or IDL/UUID changes made by this patch: none

https://hg.mozilla.org/releases/mozilla-aurora/rev/2a13504c19cd Needs some fixing up for beta.