Closed Bug 945521 Opened 6 years ago Closed 6 years ago

Regression: go button disappears from toolbar when typing on HTC or SwiftKey keyboard

Categories

(Firefox for Android :: Keyboards and IME, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 29
Tracking Status
firefox27 --- unaffected
firefox28 + fixed
firefox29 --- fixed
fennec 28+ ---

People

(Reporter: mcomella, Assigned: jchen)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

Using the HTC keyboard, the "Go button" (right arrow) disappears after typing some characters. For devices that do not provide animations, the go button does not appear at all.

This is because updateTextFromType calls InputMethods.shouldDisableUrlBarUpdate [1] which always returns true for HTC keyboards [2].

This was found in the work for bug 910859 where Robocop tests would fail because the missing Go Button cannot be clicked.

Regression from bug 909940.

[1]: https://hg.mozilla.org/mozilla-central/file/c93cfe704487/mobile/android/base/toolbar/ToolbarEditText.java#l188
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/ToolbarEditText.java#188
Presumably affects release (25) and up since bug 909940 was uplifted.
I use SwiftKey.
Summary: Regression: go button disappears from toolbar when typing on HTC keyboard → Regression: go button disappears from toolbar when typing on HTC or SwiftKey keyboard
tracking-fennec: --- → ?
Assignee: nobody → nchen
Status: NEW → ASSIGNED
tracking-fennec: ? → 27+
Jim, any update here?
Flags: needinfo?(nchen)
Going to put up a patch soon
Flags: needinfo?(nchen)
I think this is actually a regression from bug 871522, so we only need to uplift to Aurora 28. Mike, can you test/review this? I don't have an HTC device to verify the patch works.

The patch basically sets the text type to a default value instead of just returning, in cases when we don't want a new value for text type.
Attachment #8350712 - Flags: review?(michael.l.comella)
Renoming because I think this only affects 28+.
tracking-fennec: 27+ → ?
Comment on attachment 8350712 [details] [diff] [review]
Use default type when not updating URL bar text type (v1)

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

Works locally on HTC Wildfire and Galaxy Nexus - seems to work properly.

Can you add a comment to shouldDisableUrlBarUpdate as to why we disable url bar updates for HTC input methods (perhaps include a bug #)?

::: mobile/android/base/toolbar/ToolbarEditText.java
@@ +178,5 @@
>          mTextType = textType;
>  
> +        if (textType != TextType.EMPTY) {
> +            mDefaultTextType = textType;
> +        }

Why is this necessary? Doesn't this make mDefaultTextType more like mLastTextType? I figure a default (as opposed to last) is sufficient.

@@ +237,1 @@
>          }

Since mDefaultTextType is set to the last set text type, this assignment will have no effect, right? I think we can remove it.
(In reply to Michael Comella (:mcomella) from comment #7)
> @@ +237,1 @@
> >          }

lol,' how useful.

This refers to the (!restartInput) block and setTextType in ToolbarEditText, line 232 in my build.
(In reply to Michael Comella (:mcomella) from comment #7)
> Comment on attachment 8350712 [details] [diff] [review]
> Use default type when not updating URL bar text type (v1)
> 
> Review of attachment 8350712 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Works locally on HTC Wildfire and Galaxy Nexus - seems to work properly.
> 
> Can you add a comment to shouldDisableUrlBarUpdate as to why we disable url
> bar updates for HTC input methods (perhaps include a bug #)?

Will do.

> ::: mobile/android/base/toolbar/ToolbarEditText.java
> @@ +178,5 @@
> >          mTextType = textType;
> >  
> > +        if (textType != TextType.EMPTY) {
> > +            mDefaultTextType = textType;
> > +        }
> 
> Why is this necessary? Doesn't this make mDefaultTextType more like
> mLastTextType? I figure a default (as opposed to last) is sufficient.

Yeah it could be called mLastTextType. It should be the last value though. There are two icons, the button icon and the IME icon, and we want to match them. The IME icon is not under our direct control, and we want to use mDefaultTextType to track its state. And when we are not changing the IME icon (through restartInput), we should make sure to restore the button icon to match the current IME icon.

> @@ +237,1 @@
> >          }
> 
> Since mDefaultTextType is set to the last set text type, this assignment
> will have no effect, right? I think we can remove it.

mDefaultTextType is the last non-empty value. This assignment does do something if the last value happens to be empty.
Clarified on IRC.

(In reply to Jim Chen [:jchen :nchen] from comment #9)
> Yeah it could be called mLastTextType.

nit: Please call it that then! :)

> mDefaultTextType is the last non-empty value. This assignment does do
> something if the last value happens to be empty.

This value happens to be TextType.EMPTY if we empty the toolbar of text (which removes the toolbar icon). Then, when text is re-entered, the toolbar icon needs to get synced to the IME's icon (which never went away). We know what that is through mLastTextType and we set it here to update the toolbar icon. Explain this more clearly in the !restartInput block.

jchen said he would do both of the above.
I ended up renaming mTextType to mButtonTextType and adding mKeyboardTextType. This will hopefully make things clearer than "default" or "last". Also addressed other comments.
Attachment #8350712 - Attachment is obsolete: true
Attachment #8350712 - Flags: review?(michael.l.comella)
Attachment #8350766 - Flags: review?(michael.l.comella)
Comment on attachment 8350766 [details] [diff] [review]
Make sure button text type is consistent with keyboard text type (v2)

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

r+ w/ nits!

::: mobile/android/base/toolbar/ToolbarEditText.java
@@ +47,5 @@
>  
>      private final Context mContext;
>  
> +    // Type of the URL bar go/search button
> +    private TextType mButtonTextType;

nit: s/Button/Toolbar/ or s/Button/ToolbarButton/

If you'd like. I do *really* like this variable name change though.

@@ +233,5 @@
>  
> +        if (!restartInput) {
> +            // If the text was previously empty, the button and keyboard
> +            // types can become inconsistent, so we make sure that the button
> +            // type matches the keyboard type here.

nit: "If the text was previously empty, the toolbar button text type is empty and since the keyboard text type cannot be empty, their text types are inconsistent. We're re-displaying the toolbar button so we ensure consistency by setting the text types to match here." or similar.

I think the extra background detail helps a bit.
Attachment #8350766 - Flags: review?(michael.l.comella) → review+
Latest patch works on HTC Wildfire S and Galaxy Nexus.
Comment on attachment 8350795 [details] [diff] [review]
Make sure button text type is consistent with keyboard text type (v2.1)

[Approval Request Comment]

Bug caused by (feature/regressing bug #): bug 871522

User impact if declined: wrong UI behavior; URL bar Go button disappears in certain situations

Testing completed (on m-c, etc.): locally

Risk to taking this patch (and alternatives if risky): very small

String or IDL/UUID changes made by this patch: none
Attachment #8350795 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/edc671806cd0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
tracking-fennec: ? → 28+
Keywords: regression
Attachment #8350795 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.