Closed Bug 895828 Opened 11 years ago Closed 11 years ago

URL bar should not always be in focused state in editing mode

Categories

(Firefox for Android Graveyard :: Awesomescreen, defect, P1)

All
Android
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: lucasr, Assigned: mcomella)

References

Details

(Whiteboard: abouthome-hackathon [fixed-fig])

Attachments

(1 file, 5 obsolete files)

For instance, if you dismiss the keyboard while in editing mode, the URL bar should lose its orange border and the text selection background should disappear. Safe for when you dismiss the keyboard by scroll a list of pages in about:home.
Whiteboard: abouthome-hackathon
Assignee: nobody → michael.l.comella
Status: NEW → ASSIGNED
(In reply to Lucas Rocha (:lucasr) from comment #0)
> Safe for when you dismiss the keyboard by scroll a list of pages
> in about:home.

What do you mean by this? Do you just mean that when a list is scrolled or the tab is switched (by scrolling left or right), the focus should also be lost? This is the current behavior in Nightly.
Flags: needinfo?(lucasr.at.mozilla)
(In reply to Michael Comella (:mcomella) from comment #1)
> (In reply to Lucas Rocha (:lucasr) from comment #0)
> > Safe for when you dismiss the keyboard by scroll a list of pages
> > in about:home.
> 
> What do you mean by this? Do you just mean that when a list is scrolled or
> the tab is switched (by scrolling left or right), the focus should also be
> lost? This is the current behavior in Nightly.

Yep.
Flags: needinfo?(lucasr.at.mozilla)
As for the back-button, when in editing mode with the soft keyboard up, what is the expected behavior?

I would expect (1) the back button would exit editing mode entirely (immediately allowing access to the tab bar and settings buttons) but clearing user input, though I can imagine a scenario (2) where only focus is cleared, notably keeping the user's input (which is the same effect that scrolling would cause), with another back button press actually exiting editing.

There is a possibility of a mix of both (1) and (2) where (1) occurs if the user has not typed anything and (2) occurs if the user has typed anything (and possibly another split on whether you're on a new tab or not, which, imo, has a slightly different feel than tapping the url bar when on a page).

I believe (1) most closely mimics the current implementation and is the most reasonable, and I would disagree with any of the split behavior since it just makes everything inconsistent and thus more confusing.
Flags: needinfo?(lucasr.at.mozilla)
Attached patch 01: Unfocus url bar (obsolete) — Splinter Review
This is a patch containing (1). You can try it out at [1].

Before landing, I'm going to look into writing a test for this.

Concerns:
    1) An alternate implementation would be to add a private enum State { IDLE, EDITING, TRANSITIONING }, with mIsEditing and mAnimatingEntry getting replaced by the last two states respectively. This is cleaner but less flexible (e.g. what if the back button is hit during the animation? Do we just drop the keypress?), which is why I opted for this implementation. Let me know if it's worth switching.
    2) The "open app -> click Visited -> click URL bar -> click History -> click Back" interaction result is unintuitive. I'm not sure what we can do about that though.
    3) I opted to clearFocus from BrowserToolbar rather than requestFocus by the touched elements because otherwise the keyboard would not drop on the empty Reading List screen (presumably because there's no content). Do you know if there might be any side effects to this (accessibility, hardware keyboard, etc.)?

[1]: http://people.mozilla.org/~mcomella/apks/mcomella-895828_01.apk
Attachment #780713 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 780713 [details] [diff] [review]
01: Unfocus url bar

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

Need an alternative to the directly coupling with BrowserToolbar. But overall looks good.

::: mobile/android/base/BrowserToolbar.java
@@ +150,5 @@
>      private String mAutoCompleteResult = "";
>      // The user typed part of the autocomplete result
>      private String mAutoCompletePrefix = null;
>  
> +    private boolean mIsEditing;

I would probably have refactored BrowserToolbar to use mIsEditing as a way to track editing mode in a separate patch first. It would make this patch easier to review.

@@ +369,5 @@
>  
> +                if (keyCode == KeyEvent.KEYCODE_BACK && isEditing()) {
> +                    if (mDismissListener != null) {
> +                        mDismissListener.onDismiss();
> +                    }

Does this change the 'back' behaviour to always dismiss editing mode? Not sure if that's what ibarlow wants. Double-check with him. How does that interact with BrowserApp's onBackPressed?

@@ +407,5 @@
> +                    return;
> +                }
> +
> +                if (hasFocus) {
> +                    setSelected(true);

Shouldn't this be as simple as:

setSelected(hasFocus)

if (hasFocus) {
    return;
}

::: mobile/android/base/home/HomeListView.java
@@ +56,5 @@
>          mUrlOpenListener = null;
>      }
>  
>      @Override
>      public boolean onInterceptTouchEvent(MotionEvent event) {

Maybe this onInterceptTouchEvent actually belongs to HomePager so that we can apply the same behaviour if the user touches anywhere in the about:home UI. This would also cover/replace the OnPageChangeListener that you added. Maybe just call requestFocus() on the HomePager's onInterceptTouchEvent and that will be enough to cause the edit text to lose input focus?

@@ +58,5 @@
>  
>      @Override
>      public boolean onInterceptTouchEvent(MotionEvent event) {
>          if (event.getActionMasked() == MotionEvent.ACTION_DOWN) {
> +            if (BrowserApp.mBrowserToolbar != null) {

Hmm, this direct coupling with mBrowserToolbar is a no go. HomePager/HomeListView should not need to know anything about BrowserToolbar. This is a matter of focus handling, let's keep it at this level.

@@ +60,5 @@
>      public boolean onInterceptTouchEvent(MotionEvent event) {
>          if (event.getActionMasked() == MotionEvent.ACTION_DOWN) {
> +            if (BrowserApp.mBrowserToolbar != null) {
> +                BrowserApp.mBrowserToolbar.clearFocus(); // Hides soft keyboard.
> +                BrowserApp.mBrowserToolbar.setSelected(false);

Simply setSelected(false) when the entry loses focus. See my other comment.
Attachment #780713 - Flags: review?(lucasr.at.mozilla) → review-
Flags: needinfo?(lucasr.at.mozilla)
Attached patch Patch 01a: [WIP] Unfocus url bar (obsolete) — Splinter Review
(In reply to Lucas Rocha (:lucasr) from comment #5)
> Does this change the 'back' behaviour to always dismiss editing mode? Not
> sure if that's what ibarlow wants. Double-check with him.

You're right that ibarlow didn't want that - he wants (2) from comment 3, which this patch now implements.

> How does that interact with BrowserApp's onBackPressed?

BrowserApp.onBackPressed only gets called when the keyboard is hidden so the initial back button press (to clear selection) is handled in BrowserToolbar.onKeyPreIme and the editing mode dismissal is handled in onBackPressed.

> Shouldn't this be as simple as:
> 
> setSelected(hasFocus)
> 
> if (hasFocus) {
>     return;
> }

Now that the url bar is always deselected when dismissed, yes - changed.

> Maybe this onInterceptTouchEvent actually belongs to HomePager so that we
> can apply the same behaviour if the user touches anywhere in the about:home
> UI. This would also cover/replace the OnPageChangeListener that you added.
> Maybe just call requestFocus() on the HomePager's onInterceptTouchEvent and
> that will be enough to cause the edit text to lose input focus?

Done. It works correctly, except for any interaction from ReadingList -> Bookmarks, after which the url bar is still selected. Swiping does not work, nor does clicking the "Bookmarks" tab. I put on "Show Layout Bounds", which shows that the Reading List does not actually contain any views. I wonder if it's related?

Do you know why this issue might be occurring? I'm going to keep looking into it, but I figured I'd save time and ask.

The above question is my primary reason for requesting feedback, so you don't have to look back at the patch until it's more finalized.
Attachment #780713 - Attachment is obsolete: true
Attachment #781232 - Flags: feedback?(lucasr.at.mozilla)
It's worth noting that onInterceptTouchEvent() is being called on the reading list with DOWN events, meaning HomePager.requestFocus() is not stealing focus from the url bar.

This reinforces the idea that this is related to the Reading List having no views, but I'll investigate.
Confirmed: I added an item (and thus a view) to the Reading List and now it works fine. I'll look into some alternative solutions for a screen with no views, but if you have one to suggest, I'm all ears.
Attached patch 01b: Unfocus url bar (obsolete) — Splinter Review
Solved.

It is worth noting that, in this updated patch, I have one setSelected in BrowserToolbar.startEditing (and updated the comment), but otherwise let onFocusChange handle the selection. If you can't figure out why, tell me because then I imagine the comment is still not good enough. :)

I'm going to look into adding tests, otherwise this patch should be complete, barring review.
Attachment #781232 - Attachment is obsolete: true
Attachment #781232 - Flags: feedback?(lucasr.at.mozilla)
Attachment #781278 - Flags: review?(lucasr.at.mozilla)
Attached patch 01c: Unfocus url bar (obsolete) — Splinter Review
Sorry, selected the wrong file.
Attachment #781278 - Attachment is obsolete: true
Attachment #781278 - Flags: review?(lucasr.at.mozilla)
Attachment #781279 - Flags: review?(lucasr.at.mozilla)
Attached patch 01d: Unfocus url bar (obsolete) — Splinter Review
Sorry for review spam - I just updated one inefficient bit of code I overlooked.
Attachment #781279 - Attachment is obsolete: true
Attachment #781279 - Flags: review?(lucasr.at.mozilla)
Attachment #781292 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 781292 [details] [diff] [review]
01d: Unfocus url bar

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

Awesome.

::: mobile/android/base/BrowserToolbar.java
@@ +192,2 @@
>          mAnimatingEntry = false;
> +

new line is unnecessary here I guess?

@@ +359,5 @@
>                          return true;
>                      }
>                  }
>  
> +                if (keyCode == KeyEvent.KEYCODE_BACK && isSelected()) {

Is the isSelected() part really necessary here? This code path is only reached if the entry has focus, right? If there's some edge case covered by this isSelected() check, please add a comment explaining why.

@@ -1352,5 @@
>  
>              @Override
>              public void onPropertyAnimationEnd() {
> -                // Turn off selected state on the entry
> -                setSelected(false);

Is it safe to assume that stopping editing mode will necessarily move focus somewhere else? I guess it is because we hide the entry. Just wondering.

::: mobile/android/base/home/HomePager.java
@@ +173,5 @@
> +        if (event.getActionMasked() == MotionEvent.ACTION_DOWN) {
> +            // XXX: When an adapter is empty (e.g. Reading List), there are no elements focusable
> +            // in touch mode so instead of requestFocus() we use requestFocusFromTouch(). However,
> +            // now we may be focusing an object whose focused state is potentially visible to the
> +            // user but should not be (e.g. the background), so we clear the focus.

Sounds like a smart workaround ;-)

@@ +176,5 @@
> +            // now we may be focusing an object whose focused state is potentially visible to the
> +            // user but should not be (e.g. the background), so we clear the focus.
> +            requestFocusFromTouch();
> +            clearFocus();
> +        }

nit: add empty line here.
Attachment #781292 - Flags: review?(lucasr.at.mozilla) → review+
Moved r+ and rebased. In order for this not to bit-rot, I'm going to file a followup bug for the tests.

(In reply to Lucas Rocha (:lucasr) from comment #12)
>
> new line is unnecessary here I guess?

mShowUrl is a preference while the previous code block (with mAnimatingEntry and mIsEditing) contains states so I disagree. However, it's bike shedding so I'll take it out. If we want to put it back in, feel free to comment on the followup.

> Is the isSelected() part really necessary here?

Nope! Removed.

> Is it safe to assume that stopping editing mode will necessarily move focus
> somewhere else? I guess it is because we hide the entry. Just wondering.

In theory, as we leave editing mode, we're no longer interested in editing the url and thus we should remove focus from the url bar.

In practice, you are correct that we hide the View, which clears focus (I verified with the android source).

> nit: add empty line here.

Done.
Attachment #781292 - Attachment is obsolete: true
Attachment #781826 - Flags: review+
https://hg.mozilla.org/projects/fig/rev/6c4b716fe189
Whiteboard: abouthome-hackathon → abouthome-hackathon [fixed-fig]
https://hg.mozilla.org/mozilla-central/rev/6c4b716fe189
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.