Closed Bug 925082 Opened 6 years ago Closed 6 years ago

"Edit" context menu item on pinned sites just launches an empty PinSiteDialog

Categories

(Firefox for Android :: Awesomescreen, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 28
Tracking Status
firefox26 --- verified
firefox27 --- verified
firefox28 --- verified
b2g-v1.2 --- fixed
fennec 26+ ---

People

(Reporter: Margaret, Assigned: Margaret)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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?
Flags: needinfo?(ibarlow)
Blocks: 931021
(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
Flags: needinfo?(ibarlow)
(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.
Assignee: nobody → margaret.leibovic
Attached patch WIP (obsolete) — Splinter Review
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.
Attachment #824854 - Flags: feedback?(wjohnston)
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)?
Attachment #824854 - Flags: feedback?(wjohnston) → feedback+
tracking-fennec: ? → 26+
Keywords: regression
Attached patch patchSplinter Review
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.
Attachment #824854 - Attachment is obsolete: true
Attachment #825587 - Flags: review?(wjohnston)
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...
Attachment #825587 - Flags: review?(wjohnston) → review+
(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
Attachment #825587 - Flags: approval-mozilla-beta?
Attachment #825587 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/c90796281722
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
Status: RESOLVED → VERIFIED
Attachment #825587 - Flags: approval-mozilla-beta?
Attachment #825587 - Flags: approval-mozilla-beta+
Attachment #825587 - Flags: approval-mozilla-aurora?
Attachment #825587 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.