Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TextInputLayout] fix crash when focusing TextInputEditText on Meizu devices #358

Closed

Conversation

CmoaToto
Copy link
Contributor

Don't return parent hint from TextInputEditText.getHint() if the manufacturer is Meizu as their modifications in Textview leads to a crash

solve https://issuetracker.google.com/issues/112105087
(closed because duplicated Github issue: #216 )

Copy link
Contributor

@cketcham cketcham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens on Meizu devices now that we aren't calling getHint() on the TextInputLayout if the hint was only specified on the TextInputLayout?

Can you debug, step through this function, and step into layout.getHint() and see if that is called correctly?

Maybe the problem is that super.getHint() isn't called? What happens if we always call super.getHint() for Meizu devices but then return the value from layout.getHint()

thanks for working on this bug!

@CmoaToto
Copy link
Contributor Author

CmoaToto commented May 13, 2019

@cketcham I explained my investigation and my tests in the Google issue here https://issuetracker.google.com/issues/112105087

What happens on Meizu devices now that we aren't calling getHint() on the TextInputLayout if the hint was only specified on the TextInputLayout?

The layout works correctly, same behaviour as if we had put the hint on the TextInputLayout wich is the same behaviour as on other devices

Can you debug, step through this function, and step into layout.getHint() and see if that is called correctly?

After further investigation, the crashing Meizu proprietary Editor.updateCursorPositionMz() method (see Google issue link) calls getHint(), but more important, it calls the final method TextView.getHintLayout() which is null as there is no Hint in the TextView.
--> To Sum up:

  1. TextInputEditText gets focus
  2. onDraw is called on the TextInputEditText
  3. Editor.updateCursorPositionMz() is called (Meizu homemade closed source method)
  4. there should be something like that in this method:
void updateCursorsPositionsMz() {
    [...]
    if (mTextView.getHint() != null) { // getHint() return the TextInputLayout so the test is true
        mLayout = mTextView.getHintLayout(); // getHintLayout is a final TextView class.
                                   // As TextView.mHint is null, it returns null, but the
                                   // developer assumed it will always return a non-null value
        Int value = mLayout.getLineForOffset(offset); // leads to the crash
    }
}

So there is 2 solutions:

  • Meizu doesn't check if the mHintLayout is null as it thinks it is not as getHint() returns a value (the TextInputLayout's one). So the getHint() method is wrong and in this case we must return null (or super.getHint()) for Meizu devices (done by the above commit)
  • Excluding a constructor is wrong so I'll try to find a way to make the TextView create a blank mHintLayout in order to let the Meizu device work as expected, but this workaround will create an unneeded Layout on all other devices

Maybe the problem is that super.getHint() isn't called? What happens if we always call super.getHint() for Meizu devices but then return the value from layout.getHint()

Not working

@cketcham
Copy link
Contributor

Thanks for the info. This definitely looks like a crash because the device is expecting getHintLayout() to not be null even though it could be.

If we can't find anything better, I your current approach could be ok.

One other thought I had is to call something like:

TextInputLayout layout = getTextInputLayout();
if ((layout != null) && layout.isProvidingHint() && super.getHint() == null && Build.MANUFACTURER.equals("Meizu")) {
  setHint("");
}

in onDraw() or onAttachedToWindow() or some where early enough to create the hint layout for these devices. Does that cause the hint layout to exist?

@CmoaToto
Copy link
Contributor Author

CmoaToto commented May 14, 2019

Indeed it is a very good balance between solutions 1 and 2. I tested it and it worked perfectly on several devices.
I tested step by step and the mLayout.getLineForOffset(offset) is called without any NPE.
I updated the PR accordingly.

Copy link
Contributor

@cketcham cketcham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! That looks good to me, I can merge this internally and we'll push the fix to github soon.

@CmoaToto
Copy link
Contributor Author

Great! Thank you :)

@cketcham
Copy link
Contributor

@CmoaToto We're considering moving the call to onAttachedToWindow() instead of onDraw() can you verify the fix still works if we do that? thanks.

@CmoaToto
Copy link
Contributor Author

@cketcham You're right, this way the fix will be called only once per View creation on not at each onDraw.
I tested and it works perfectly.
I updated the PR accordingly

@awenger
Copy link

awenger commented May 17, 2019

thanks so much @CmoaToto and @cketcham ! This will fix 25% of all our crashes we saw in the last 7 days. Do you already know when this will be released?

@cketcham
Copy link
Contributor

The next release should happen around the end of May

@adriancretu
Copy link

Crash still occurs on some Meizu devices, with all the fixes in place. Looking at the fix vs the crash logs, this happens because Build.MANUFACTURER.equals("Meizu") doesn't pass the test when the manufacturer is "meizu". This happens on Meizu M5c unrooted devices.

@CmoaToto
Copy link
Contributor Author

@adriancretu According to several dumps of FlymOS (Meizu) found on Github (https://github.com/search?p=4&q=%22ro.build.display.id%3DFlyme%22&type=Code) you're right, there is like 1 Meizu device on 20 that uses ro.product.manufacturer=meizu instead of ro.product.manufacturer=Meizu. I'll do a PR to fix this.

FYI I searched but didn't find other ro.product.manufacturer from Meizu.

@CmoaToto
Copy link
Contributor Author

@adriancretu Here is the PR #411

@cketcham Please review it to completely fix this problem :)

@robertlevonyan
Copy link

robertlevonyan commented Dec 21, 2019

I have this crash on material:1.2.0-alpha02. I'm using AutoCompleteTextView, inside TextInputLayout

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants