Closed Bug 1220309 Opened 5 years ago Closed 4 years ago

Extend AppCompatActivity in GeckoApp

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: mcomella, Assigned: mcomella)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 1 obsolete file)

While doing research for bug 1220196, I discovered in bug 1220196 comment 3 that we don't extend AppCompatActivity in GeckoApp, which could be why AppCompat seems to have a lot of inconsistency issues. This could also be a reason for much of the AppCompat fallout.

However, due to a bug where the menu won't open on Gingerbread, we can't actually do this.

Potential solutions:
  * Wait for the bug to be fixed (https://code.google.com/p/android/issues/detail?id=185217)
    - We could also use an older version of the support library in the mean-time
  * Show the software menu button and use the v11+ menu: bug 1209967
Bug 1220309 - Have GeckoApp extend AppCompatActivity. r=sebastian

We're using the Theme.AppCompat styles so we have to extend AppCompatActivity.
It is a subclass of the previous class GeckoApp extended so we shouldn't run
into an ClassCastExceptions.
I noticed the long-press browser toolbar action bar changes styles when extending AppCompatActivity – I needed to remove the `android:` prefix to get it to work properly with the attached theme.

We may need to apply this to other attributes as well. For an official list of attributes supported by the support library, see:
  http://androidxref.com/6.0.0_r1/xref/frameworks/support/v7/appcompat/res/values/attrs.xml
Comment on attachment 8682174 [details] [diff] [review]
Fix browser toolbar action bar styles

Moved this work to bug 1220863 so we don't forget to do it, but it'll probably get duped to this bug.
Attachment #8682174 - Attachment is obsolete: true
In bug 1215079, we extend the built-in android themes on GB – we need to extend AppCompat again as to undo that before this lands, otherwise there will probably be some unexpected behavior.
The one side effect I see from this bug is the synced tabs setup home panel has the "Get started" button capitalized. There is no obvious reason for this. :\
Assignee: nobody → michael.l.comella
(In reply to Michael Comella (:mcomella) from comment #5)
> The one side effect I see from this bug is the synced tabs setup home panel
> has the "Get started" button capitalized. There is no obvious reason for
> this. :\

via stackoverflow [1]

> Button text is all-caps by default when using the Material (or DeviceDefault
> with API 21+) theme. This is working as intended.

It seems the fix is [2]:

> Set android:textAllCaps="false". If you are using an appcompat style, make sure
> textAllCaps comes before the style. Otherwise the style will override it.

Which we can add to the button style [3].

[1]: http://stackoverflow.com/questions/26958909/why-is-my-button-text-coerced-to-all-caps-on-lollipop#comment42460665_26958909
[2]: http://stackoverflow.com/a/29149790
[3]: http://stackoverflow.com/a/30607625
Comment on attachment 8681524 [details]
MozReview Request: Bug 1220309 - Have GeckoApp extend AppCompatActivity. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23843/diff/1-2/
Bug 1220309 - Correct all caps button text in Button from AppCompat. r=sebastian

AppCompat capitalizes all text in `Button`s so we have to override
that behavior to maintain the same UI. Ideally, we do this through
`android:buttonStyle` but the place I found the issue doesn't inherit
from that style so we can't and we change the style directly.

There may be issues with other `Button`s, but this is the only one I found.
Attachment #8698543 - Flags: review?(s.kaspari)
https://reviewboard.mozilla.org/r/28027/#review25169

r+ because it preserves the current behavior.

Does this actually break something or does it just look different with capitalized text?

The guidelines say:
"Button text should be capitalized in languages that have capitalization. For other languages, colored text on flat buttons distinguishes them from normal text."
https://www.google.de/design/spec/components/buttons.html#buttons-flat-raised-buttons

I wonder if we should follow the guidelines or if we have a compelling reason to differ. However: Land this. :)
NI-ing antlam, just to see if it's worth a follow-up: See comment 10.
Flags: needinfo?(alam)
Mh, reviewboard did not synchronize the r+. I'll manually r+ here.
Attachment #8681524 - Flags: review?(s.kaspari) → review+
Attachment #8698543 - Flags: review?(s.kaspari) → review+
(In reply to Sebastian Kaspari (:sebastian) from comment #10)
> Does this actually break something or does it just look different with
> capitalized text?

Just looks different – I was aiming to preserve the existing appearance.
This is going to get backed out for test failures. I forgot to run the tests, which probably use FragmentActivity rather than AppCompatActivity.
Caused Android build bustage: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=6f96c9f06a3d

Backed out in https://hg.mozilla.org/integration/fx-team/rev/0574c7d54cc3

13:33:02     INFO -  /builds/slave/fx-team-and-api-9-d-0000000000/build/src/mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testReadingListProvider.java:98: error: cannot access AppCompatActivity
13:33:02     INFO -          if (a instanceof BrowserApp) {
13:33:02     INFO -                ^
13:33:02     INFO -    class file for android.support.v7.app.AppCompatActivity not found
13:33:02     INFO -  /builds/slave/fx-team-and-api-9-d-0000000000/build/src/mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testReadingListProvider.java:99: error: inconvertible types
13:33:02     INFO -              ((BrowserApp) a).getReadingListHelper().disableBackgroundFetches();
13:33:02     INFO -                            ^
13:33:02     INFO -    required: BrowserApp
13:33:02     INFO -    found:    Activity
13:33:02     INFO -  /builds/slave/fx-team-and-api-9-d-0000000000/build/src/mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testGeckoProfile.java:49: error: inconvertible types
13:33:02     INFO -          verifyProfile(profile, GeckoProfile.DEFAULT_PROFILE, ((GeckoApp) getActivity()).getProfile().getDir(), true);
13:33:02     INFO -                                                                                      ^
13:33:02     INFO -    required: GeckoApp
13:33:02     INFO -    found:    Activity
13:33:02     INFO -  /builds/slave/fx-team-and-api-9-d-0000000000/build/src/mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testGeckoProfile.java:68: error: inconvertible types
13:33:02     INFO -              File defaultProfile = ((GeckoApp) getActivity()).getProfile().getDir();
13:33:02     INFO -                                                           ^
13:33:02     INFO -    required: GeckoApp
13:33:02     INFO -    found:    Activity
13:33:02     INFO -  /builds/slave/fx-team-and-api-9-d-0000000000/build/src/mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testGeckoProfile.java:104: error: inconvertible types
13:33:02     INFO -                  File defaultProfile = ((GeckoApp) getActivity()).getProfile().getDir();
13:33:02     INFO -                                                               ^
13:33:02     INFO -    required: GeckoApp
13:33:02     INFO -    found:    Activity
13:33:02     INFO -  /builds/slave/fx-team-and-api-9-d-0000000000/build/src/mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testGeckoProfile.java:134: error: inconvertible types
13:33:02     INFO -                  File defaultProfile = ((GeckoApp) getActivity()).getProfile().getDir();
13:33:02     INFO -                                                               ^
13:33:02     INFO -    required: GeckoApp
13:33:02     INFO -    found:    Activity
13:33:02     INFO -  Note: Some input files use or override a deprecated API.
13:33:02     INFO -  Note: Recompile with -Xlint:deprecation for details.
13:33:02     INFO -  Note: /builds/slave/fx-team-and-api-9-d-0000000000/build/src/mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/SessionTest.java uses unchecked or unsafe operations.
13:33:02     INFO -  Note: Recompile with -Xlint:unchecked for details.
13:33:02     INFO -  6 errors
13:33:02     INFO -  gmake[5]: *** [classes.dex] Error 1
13:33:02     INFO -  gmake[5]: Leaving directory `/builds/slave/fx-team-and-api-9-d-0000000000/build/src/obj-firefox/mobile/android/tests/browser/robocop'
13:33:02     INFO -  gmake[4]: *** [mobile/android/tests/browser/robocop/tools] Error 2
13:33:02     INFO -  gmake[4]: Leaving directory `/builds/slave/fx-team-and-api-9-d-0000000000/build/src/obj-firefox'
13:33:02     INFO -  gmake[3]: *** [tools] Error 2
13:33:02     INFO -  gmake[3]: Leaving directory `/builds/slave/fx-team-and-api-9-d-0000000000/build/src/obj-firefox'
13:33:02     INFO -  gmake[2]: *** [default] Error 2
13:33:02     INFO -  gmake[2]: Leaving directory `/builds/slave/fx-team-and-api-9-d-0000000000/build/src/obj-firefox'
13:33:02     INFO -  gmake[1]: *** [realbuild] Error 2
13:33:02     INFO -  gmake[1]: Leaving directory `/builds/slave/fx-team-and-api-9-d-0000000000/build/src'
13:33:02     INFO -  gmake: *** [build] Error 2
13:33:02     INFO -  341 compiler warnings present.
13:33:03     INFO -  Notification center failed: Install the python dbus module to get a notification when the build finishes.
Nick gave me a patch that fixed some build goop that he expects to make this work so I'm not going to bother with try (AppCompatActivity extends FragmentActivity so I don't actually expect any issues).
I can repro the failure in testPrivateBrowsing:
* Open fennec, making sure it's not in hanging around in memory
* Enter `about:blank`
* Click the 3-dot menu (not the hardware menu key)

And the menu is empty, meaning we can't find the menu item we're looking for. Looking into it...
testSessionHistory has a similar issue that I can repro on my local 2.3 device – the menu appears empty.
I think for now, if its small little action buttons like “clear site settings” in a full page dialog (for example) then we should follow system and go with all caps.

But if it's something more branded to Firefox, like our call-to-action buttons (in our in-product helper UI and First run), or our "Sign up for Sync!" action buttons (in empty Panels), then we should keep to our own guidelines.
Flags: needinfo?(alam)
We killed GB, let's see how we fair this time.
https://hg.mozilla.org/mozilla-central/rev/dabb7828c1ee
https://hg.mozilla.org/mozilla-central/rev/34227dc37f69
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Depends on: 1259479
No longer depends on: 1268603
You need to log in before you can comment on or make changes to this bug.