818501 - Some composition spans are not displayed

Status: RESOLVED FIXED

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

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

On Android >= 4, the stock IME can use SuggestionSpan instead of UnderlineSpan. The underline in Gecko disappears when the stock IME switches from UnderlineSpan to SuggestionSpan.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Get composition styles from styles returned by the system (v1)

This patch asks Android to tell us what styles to use and then translate those styles to Gecko equivalents. This way any type of CharacterStyle span is supported including SuggestionSpan.

It also fixes the Android-to-Gecko color conversion code. It switched red and blue components but I didn't realize it before.

Comment 2

[Chris Peterson [:cpeterson]]

Comment on attachment 688791 [details] [diff] [review]
Get composition styles from styles returned by the system (v1)

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

Looking good, but I have some questions:

::: mobile/android/base/GeckoEditable.java
@@ +282,5 @@
> +            } catch (Exception e) {
> +                // return a default value
> +                return type.newInstance();
> +            }
> +        } catch (Exception e) {

Why do you have nested try/catch blocks? Can type.newInstance() throw an exception (other than NullPointerException if type == null)? If that is the case you are handling, then I think the outer try/catch should be moved inside the inner catch around type.newInstance().

@@ +283,5 @@
> +                // return a default value
> +                return type.newInstance();
> +            }
> +        } catch (Exception e) {
> +            return null;

Is returning null an appropriate way to handle this error? The code that calls getField() does not check for null.

Should we Log.w() a warning message? Does this error condition indicate a bug in our code or an unexpectedly missing method in the Android framework classes?

@@ +336,3 @@
>          }
>          int rangeStart = composingStart;
> +        TextPaint tp = new TextPaint(), emptyTp = new TextPaint();

Please split these variable declarations onto two separate lines.

@@ +369,5 @@
> +                }
> +                int tpUnderlineColor = 0;
> +                float tpUnderlineThickness = 0.0f;
> +                // These TextPaint fields only exist on Android >= 4 and are not in the SDK
> +                if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.ICE_CREAM_SANDWICH) {

Please compare SDK_INT to 14 instead of ICE_CREAM_SANDWICH. I know our code is inconsistent, but the explanation given to me (which I [now] agree with :) is that the Android API docs only list the API level number. Comparing SDK_INT to a number should make reviewing code and the API docs easier. For example, we've had some bugs where our code was comparing SDK_INT with HONEYCOMB but should have been used HONEYCOMB_MR2.

Mentioning ICS by name in the comment is a good idea.

@@ +371,5 @@
> +                float tpUnderlineThickness = 0.0f;
> +                // These TextPaint fields only exist on Android >= 4 and are not in the SDK
> +                if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.ICE_CREAM_SANDWICH) {
> +                    tpUnderlineColor = (Integer)getField(tp, "underlineColor", Integer.class);
> +                    tpUnderlineThickness = (Float)getField(tp, "underlineThickness", Float.class);

Should these calls to getField() check for null return values? I would prefer NOT to, but maybe we need to (depending on your thoughts on my comments in the getField() method above).

::: widget/android/nsWindow.cpp
@@ +1689,5 @@
>  
>  static nscolor
>  ConvertAndroidColor(uint32_t c)
>  {
> +    return NS_RGBA((c & 0x00ff0000) >> 16,

Let's rename the `c` parameter to `argb` to make the intended conversion clearer.

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Get composition styles from styles returned by the system (v2)

(In reply to Chris Peterson (:cpeterson) from comment #2)
> Comment on attachment 688791 [details] [diff] [review]
> Get composition styles from styles returned by the system (v1)
> 
> Review of attachment 688791 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking good, but I have some questions:
> 
> ::: mobile/android/base/GeckoEditable.java
> @@ +282,5 @@
> > +            } catch (Exception e) {
> > +                // return a default value
> > +                return type.newInstance();
> > +            }
> > +        } catch (Exception e) {
> 
> Why do you have nested try/catch blocks? Can type.newInstance() throw an
> exception (other than NullPointerException if type == null)? If that is the
> case you are handling, then I think the outer try/catch should be moved
> inside the inner catch around type.newInstance().

Yeah, newInstance() can throw a couple more exceptions. I thought the nested try looked prettier than a try inside a catch :)  But I can change it to that.

> @@ +283,5 @@
> > +                // return a default value
> > +                return type.newInstance();
> > +            }
> > +        } catch (Exception e) {
> > +            return null;
> 
> Is returning null an appropriate way to handle this error? The code that
> calls getField() does not check for null.

I think it's reasonable, since Integer and Float should never throw up on newInstance(); we are just required to have a catch there. But come to think of it, maybe the better way is to just give a default value to getField. I changed it to that in the new patch.

> @@ +336,3 @@
> >          }
> >          int rangeStart = composingStart;
> > +        TextPaint tp = new TextPaint(), emptyTp = new TextPaint();
> 
> Please split these variable declarations onto two separate lines.

Done

> @@ +369,5 @@
> > +                }
> > +                int tpUnderlineColor = 0;
> > +                float tpUnderlineThickness = 0.0f;
> > +                // These TextPaint fields only exist on Android >= 4 and are not in the SDK
> > +                if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.ICE_CREAM_SANDWICH) {
> 
> Please compare SDK_INT to 14 instead of ICE_CREAM_SANDWICH. I know our code
> is inconsistent, but the explanation given to me (which I [now] agree with
> :) is that the Android API docs only list the API level number. Comparing
> SDK_INT to a number should make reviewing code and the API docs easier. For
> example, we've had some bugs where our code was comparing SDK_INT with
> HONEYCOMB but should have been used HONEYCOMB_MR2.

Done

> Mentioning ICS by name in the comment is a good idea.
> 
> @@ +371,5 @@
> > +                float tpUnderlineThickness = 0.0f;
> > +                // These TextPaint fields only exist on Android >= 4 and are not in the SDK
> > +                if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.ICE_CREAM_SANDWICH) {
> > +                    tpUnderlineColor = (Integer)getField(tp, "underlineColor", Integer.class);
> > +                    tpUnderlineThickness = (Float)getField(tp, "underlineThickness", Float.class);
> 
> Should these calls to getField() check for null return values? I would
> prefer NOT to, but maybe we need to (depending on your thoughts on my
> comments in the getField() method above).

Changed getField to take a default value.

> ::: widget/android/nsWindow.cpp
> @@ +1689,5 @@
> >  
> >  static nscolor
> >  ConvertAndroidColor(uint32_t c)
> >  {
> > +    return NS_RGBA((c & 0x00ff0000) >> 16,
> 
> Let's rename the `c` parameter to `argb` to make the intended conversion
> clearer.

Done

Comment 4

[Chris Peterson [:cpeterson]]

Comment on attachment 688953 [details] [diff] [review]
Get composition styles from styles returned by the system (v2)

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

LGTM! I like your getField() solution using a default value parameter! :)

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2329ce0da859

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2329ce0da859