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

[BUG] Incorrect order of HTML tags in strings #2815

Closed
IgorEisberg opened this issue May 21, 2022 · 5 comments · Fixed by #2816
Closed

[BUG] Incorrect order of HTML tags in strings #2815

IgorEisberg opened this issue May 21, 2022 · 5 comments · Fixed by #2816
Labels

Comments

@IgorEisberg
Copy link
Contributor

Information

  1. Apktool Version (apktool -version) - 2.6.X
  2. Operating System (Mac, Linux, Windows) - Any
  3. APK From? (Playstore, ROM, Other) - Dummy

Description

apktool 2.6.X messes up the order of HTML tags in strings when decompiling an APK, rendering it impossible to recompile without manually fixing the broken tags, like Android 11's Settings.apk (AOSP) with full translations.
<br/><br/> decompiled as <br></br><br></br> with 2.5.1 but <br><br></br></br> with 2.6.X
</b><br/> decompiled as </b><br></br> with 2.5.1 but </br></b><br> with 2.6.X
... and so on. Full example:

Original string:
<string name="test">Quickly zoom in on the screen to display content more clearly.<br/><br/> <b>To zoom in:</b><br/> 1. Use shortcut to start magnification<br/> 2. Tap the screen<br/> 3. Drag 2 fingers to move around screen<br/> 4. Pinch with 2 fingers to adjust zoom<br/> 5. Use shortcut to stop magnification<br/><br/> <b>To zoom in temporarily:</b><br/> 1. Use shortcut to start magnification<br/> 2. Touch &amp; hold anywhere on the screen<br/> 3. Drag finger to move around screen<br/> 4. Lift finger to stop magnification</string>
Decompiled with apktool 2.5.1:
<string name="test">Quickly zoom in on the screen to display content more clearly.<br></br><br></br> <b>To zoom in:</b><br></br> 1. Use shortcut to start magnification<br></br> 2. Tap the screen<br></br> 3. Drag 2 fingers to move around screen<br></br> 4. Pinch with 2 fingers to adjust zoom<br></br> 5. Use shortcut to stop magnification<br></br><br></br> <b>To zoom in temporarily:</b><br></br> 1. Use shortcut to start magnification<br></br> 2. Touch &amp; hold anywhere on the screen<br></br> 3. Drag finger to move around screen<br></br> 4. Lift finger to stop magnification</string>
Decompiled with apktool 2.6.X:
<string name="test">Quickly zoom in on the screen to display content more clearly.<br><br></br></br> <b>To zoom in:</br></b><br> 1. Use shortcut to start magnification<br></br> 2. Tap the screen<br></br> 3. Drag 2 fingers to move around screen<br></br> 4. Pinch with 2 fingers to adjust zoom<br></br> 5. Use shortcut to stop magnification<br><br></br></br> <b>To zoom in temporarily:</b><br></br> 1. Use shortcut to start magnification<br></br> 2. Touch &amp; hold anywhere on the screen<br></br> 3. Drag finger to move around screen<br></br> 4. Lift finger to stop magnification</string>

Steps to Reproduce

Simply compile the source with apktool b, then decompile the output APK with apktool d.

APK

Source for a minimal dummy APK uploaded here:
https://drive.google.com/file/d/1wZKVpsHRuOxV9Fdnj3iYovPyfpcoQ2t2/view?usp=sharing

@IgorEisberg
Copy link
Contributor Author

IgorEisberg commented May 21, 2022

The problem seems to be specifically related to the consecutive appearance of <br/> (i.e. <br/><br/>) which results in <br><br></br></br> and also effects the rest of the results.
Example:
Source: <string name="test">Test<br/><br/><b>Test</b><br/>Test</string>
Result: <string name="test">Test<br><br></br></br><b>Test</br></b><br>Test</string>

But if we add a space between the <br/>:
Source: <string name="test">Test<br/> <br/><b>Test</b><br/>Test</string>
Result: <string name="test">Test<br></br> <br></br><b>Test</b><br></br>Test</string>

@iBotPeaches
Copy link
Owner

Did some git blame, looks like this PR introduced regression - #2631

but I enjoy how much easier that code is to read than last, so will do some digging.

@IgorEisberg
Copy link
Contributor Author

Same, looking for a solution, tricky since both <br/> (essentially 2 pairs of <br></br>) have the same name, position and matchingTagPosition...

@iBotPeaches
Copy link
Owner

Okay, I've changed my mind. I believe I am going forward with a revert. I've noticed a few recent tickets are all related to this and it shows

  1. our test suite was weak with all these types of string patterns
  2. porting bundletool, while mixing with apkool methods is not bullet proof
  3. this stuff is confusing

@IgorEisberg
Copy link
Contributor Author

Hold on, I worked out a solution, will make a pull requests for you to try.

iBotPeaches pushed a commit that referenced this issue May 23, 2022
* fix: redesign StyledString decoding

* optimize: avoid calling span.getName() twice

* fix: order spans due to aapt1/2 discrepancy
* fixes: #2815
iBotPeaches added a commit that referenced this issue May 23, 2022
iBotPeaches added a commit that referenced this issue May 23, 2022
* style: reorg getAttributes (StyledString)

* test: assertion for aapt2 string (#2815)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants