Fix Navigation Bar Buttons Not Visible with Signup Sheet #11051

ashiagr · 2020-01-07T09:35:58Z

This PR tries to fix #10908

It is an existing Android issue which has become obsolete because of lower priority. Seems like we need to go for a hack to fix it. Tried different solutions, this one seems to be working well in both portrait and landscape mode.

To test:

Clear app data.
Launch app.
Tap Sign Up for WordPress.com button.
Notice navigation buttons are visible.

API	Orientation	Before	After
27	Portrait
27	Landscape
29	Portrait
29	Landscape

PR submission checklist:

I have considered adding unit tests where possible.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

peril-wordpress-mobile · 2020-01-07T09:36:05Z

	Messages
📖	This PR contains changes in the subtree `libs/login/`. It is your responsibility to ensure these changes are merged back into `wordpress-mobile/WordPress-Login-Flow-Android`. Follow these handy steps! WARNING: Make sure your git version is 2.19.x or lower - there is currently a bug in later versions that will corrupt the subtree history! `cd WordPress-Android` `git checkout issue/10908-navigation-bar-buttons-not-visible-with-signup-sheet` `git subtree push --prefix=libs/login/ https://github.com/wordpress-mobile/WordPress-Login-Flow-Android.git merge/WordPress-Android/11051` Browse to https://github.com/wordpress-mobile/WordPress-Login-Flow-Android/pull/new/merge/WordPress-Android/11051 and open a new PR.

Generated by 🚫 dangerJS

peril-wordpress-mobile · 2020-01-07T09:39:07Z

You can test the changes on this Pull Request by downloading the APK here.

theck13

The navigation bar looks good in the screenshots for this pull request in portrait orientation, but the landscape orientation looks odd since only part of the navigation bar background is unshadowed.

We should be able to fix this without hacks. We updated the navigation bar background color including bottom sheets in Simplenote in Automattic/simplenote-android#792. The changes included updating the base class from BottomSheetDialog to BottomSheetDialogFragment as well as updating styles.xml files. See the screenshots in the pull request for illustration.

ashiagr · 2020-01-08T03:07:55Z

The changes included updating the base class from BottomSheetDialog to BottomSheetDialogFragment as well as updating styles.xml files.

Thank you for the directions @theck13 🙇‍♀️. I'll take a look and update it.

ashiagr · 2020-01-13T04:21:52Z

Updated screenshots after applying changes requested.

API	Portrait	Landscape
26
27
29

malinajirka

Note: @theck13 I hope you don't mind me jumping in. Riccardo would like to merge a feature which is blocked by this bug into the next release so I wanted to speed thing up. Since the changes are touching the Login flow I think it's good to have it reviewed by at least two developers anyway.

Thanks @ashiagr! I've reviewed most of the code and tested the changes. I've left some in-code comments and I found a bug during the tests. Let me know what you think ;)

Emulator, Nexus 9, API 28

the buttons are white on light background
the width of the view seems a bit weird
I'm not sure we want to change the width of the bottom navigation bar

Don't worry if we don't merge the changes before the code freeze - we can create a hotfix if it's necessary to include the fix in the next release.

WordPress/src/main/java/org/wordpress/android/ui/accounts/LoginActivity.java

...ressLoginFlow/src/main/java/org/wordpress/android/login/SignupBottomSheetDialogFragment.java

malinajirka · 2020-01-13T09:01:35Z

...ressLoginFlow/src/main/java/org/wordpress/android/login/SignupBottomSheetDialogFragment.java

+            dialog.setOnDismissListener(new DialogInterface.OnDismissListener() {
+                @Override
+                public void onDismiss(DialogInterface dialog) {
+                    mSignupSheetListener.onSignupSheetCanceled();


It seems this call is duplicated - we invoke the same callback in public void onDismiss(DialogInterface dialog). Is this intentional?

dialog.setOnDismissListener was initially added referring to SimpleNote dialog code. It didn't seem to cover a scenario when dialog fragment was dismissed by touching on the screen and screen was rotated (dialog fragment reappeared even though it was dismissed) which is why I added public void onDismiss(DialogInterface dialog) callback. I'll test it further and see if we can eliminate one of them.

Fixed here d15dd92

...LoginFlow/src/main/java/org/wordpress/android/login/widgets/WPBottomSheetDialogFragment.java

If Android decides to recreate a Fragment later, it's going to call the no-argument constructor of the fragment.

...ressLoginFlow/src/main/java/org/wordpress/android/login/SignupBottomSheetDialogFragment.java

ashiagr · 2020-01-14T12:16:47Z

Thank you for reviewing the changes @malinajirka 🙏! Fixed issues related to dialog to fragment conversion.

However this design issue still remains,

Emulator, Nexus 9, API 28
the buttons are white on light background

Noticed it appears in SimpleNote android app as well

As we're using a light themed bottom sheet style, ideally navigation bar icons should have been darker in color. But it isn't the case here, Android platform provides little support in customising navigation bar icons color. @theck13 are you aware of the issue in SimpleNote app? Do you have any suggestions?

theck13 · 2020-01-14T14:24:13Z

No, I wasn't aware of it. No, I don't have any suggestions.

malinajirka · 2020-01-15T08:42:02Z

Thanks @ashiagr! I've looked into this and I found what is causing the issue. It's the hack we use in WPBottomSheetDialogFragment to adjust the width of the view/window. Tbh I'd personally discuss this with a designer as I believe it's ok too keep the sheet fullwidth - it's the default OS behavior, right? If they think it's better to use a maxWidth, we'll need to come up with another solution - eg. different layout/dimens for tablets. Wdyt?
P.S. Besides I don't understand why we change the layout parameters of the window and not just parameters of the view. 🤷‍♂

ashiagr · 2020-01-15T10:14:05Z

Good catch @malinajirka ! Thanks for looking into it.

I believe it's ok too keep the sheet fullwidth - it's the default OS behavior, right

Right, it is the default OS behavior. I agree we should keep it fullwidth.

Screenshots for the fullwidth layout (for design review):

Landscape	Portrait

Besides I don't understand why we change the layout parameters of the window and not just parameters of the view.

Tried setting it on the fragment's view, it doesn't seem to be covering full width

It could be because full width setting for dialog window window.setLayout(-1, -1); inside BottomSheetDialog. But we can investigate it more.

protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        Window window = this.getWindow();
        if (window != null) {
            if (VERSION.SDK_INT >= 21) {
                window.clearFlags(67108864);
                window.addFlags(-2147483648);
            }
            window.setLayout(-1, -1);
        }
    }

osullivanchris

@ashiagr Hey this looks like a great starting point, good job!

If we find a scenario where this isn't working for us later I think we can add more breakpoints or definitions at that point. Would you agree?

Also do you think we should extend this to landscape on phones in any cases or keep it just for tablet? cc @theck13 as well on that point.

ashiagr · 2020-01-28T12:27:02Z

If we find a scenario where this isn't working for us later I think we can add more breakpoints or definitions at that point. Would you agree?

I agree.

do you think we should extend this to landscape on phones in any cases or keep it just for tablet?

I would want it to be consistent with our other apps like SimpleNote, and like to know @theck13's inputs.

theck13 · 2020-01-28T13:35:05Z

Simplenote is different since it uses the large and large-land modifiers.

Personally, I think the bottom sheet looks a little stretched on phones in landscape orientation. To adjust that and since the horizontal button layout only occurs in landscape orientation, we could add bottom_sheet_dialog_width with a value around 512dp and the w600dp-land qualifier. I haven't tried that out though so we'll want to test it to make sure it looks good. Otherwise, it looks good to me.

ashiagr · 2020-01-29T06:28:14Z

Thanks @theck13 !

I tested 512dp for w600dp-land on few phone emulators. @osullivanchris, can you review these screenshots?

Device/ Resolution/ ViewPort/ Density	Landscape
Nexus6p/ 1440x2560/ 412x738/ xxhdpi
Pixel 3/ 1080x2160/ 411x823/ xxhdpi
Pixel 3 XL/ 1440x2960/ 411x846/ xxxhdpi
Samsung S10/ 1440x3040/ 360x760/ xxxhdpi
Galaxy Fold/ 840x1960/ /xhdpi

osullivanchris

LGTM @ashiagr !

cc @develric as we are trying to solve some landscape issues with sheets that are related.

@develric and I are also making some stylistic changes to bottom sheets for the iA project - I think the style changes should probably apply across the board as part of a bottom sheet component. But not sure which task that would be a part of - its probably separate to this.

…sible-with-signup-sheet # Conflicts: # libs/login/WordPressLoginFlow/src/main/java/org/wordpress/android/login/SignupBottomSheetDialog.java

theck13 · 2020-01-30T23:36:44Z

Now that we have bottom_sheet_dialog_width with 512dp for sw600dp-land and w600dp-land, I think we can delete the sw600dp-land instance since it's redundant. @ashiagr and @malinajirka, correct me if I'm wrong or I'm forgetting a scenario for which we need to keep sw600dp-land.

ashiagr · 2020-01-31T07:30:34Z

Thanks for the suggestion @theck13 . I removed the value from sw600dp-land and tested it on a Nexus 10 tablet emulator, buttons looked squished again.

According to Android's best-matching resource guide , it seems we have this order of precedence:

values-sw600dp-land
values-sw600dp
values-w600dp-land

If we remove values-sw600dp-land, it picks up next precedence which is values-sw600dp - 300dp for the tablet. Based on the test, I suggest we keep sw600dp-land as well.

ashiagr · 2020-02-06T09:21:44Z

@theck13 do you find the solution good enough to be merged? If yes, can you approve these changes?

theck13 · 2020-02-06T12:31:07Z

I haven't found a better solution, but I haven't been researching this either. Therefore, we're going to use this is Simplenote (Automattic/simplenote-android#933). I haven't reviewed the code as extensively as @malinajirka and he had requested changes in his review. So I'll leave the final approval to him.

malinajirka · 2020-02-10T11:53:20Z

Thanks @ashiagr for all the improvements!! ;) The final solution is great.

I've been testing the sign-up flow and I noticed two bugs.

The first issue is already being tracked so it apparently isn't related your changes. I'm just mentioning it here to keep thinks linked. Bottom sheet animation glitch when app comes from background. #11242
The bottom sheet doesn't disappear after rotation.

Click on "SIGN UP FOR WORDPRESS.COM"
Rotate your device
Click on "SIGN UP WITH EMAIL"
notice the bottom sheet doesn't disappear (Do we perhaps show two dialogs on top of one another?)

jkmassel · 2020-02-10T22:22:37Z

We're freezing 14.2 today, so this PR is being bumped to 14.3. If you'd like it to ship with 14.2, please merge it into release/14.2 and ping me – I'll be happy to cut a new beta release.

…sible-with-signup-sheet

ashiagr · 2020-02-11T08:43:19Z

The bottom sheet doesn't disappear after rotation.

Thanks for catching the bug @malinajirka ! Fixed here f963bc2

malinajirka

Thanks @ashiagr! It all looks good now.

I think we might want to keep an eye on Sign-Up/Login tracks after 14.3 gets released. Just to make sure we didn't introduce any crashes. Could you please add a reminder and check it when the time comes. Thanks!

P.S. We should also push the changes to the login lib repo.

malinajirka · 2020-02-12T10:33:12Z

@theck13 It seems, I can't merge the PR, since you requested some changes. Could you please either dismiss the review or approve it and merge the PR. Thanks!

Fix signup bottom sheet navigation bar buttons not visible issue

93f7493

ashiagr added [Type] Bug Signup labels Jan 7, 2020

ashiagr requested review from theck13 and develric January 7, 2020 09:35

ashiagr self-assigned this Jan 7, 2020

ashiagr added the [Status] Not Ready for Review label Jan 7, 2020

theck13 reviewed Jan 7, 2020

View reviewed changes

Convert BottomSheetDialog to BottomSheetDialogFragment and update styles

98c445c

ashiagr requested a review from malinajirka January 13, 2020 04:22

ashiagr removed the [Status] Not Ready for Review label Jan 13, 2020

malinajirka self-assigned this Jan 13, 2020

malinajirka requested changes Jan 13, 2020

View reviewed changes

ashiagr added 5 commits January 13, 2020 16:58

Avoid using a parameter in Fragment's constructor

4fd8960

If Android decides to recreate a Fragment later, it's going to call the no-argument constructor of the fragment.

Remove dialog.setOnDismissListener

d15dd92

Eliminate setRetainInstance(true)

1d51b1f

Eliminate logic to manually recreate the dialog(fragment)

8dcfbc7

Extract parts of code from onCreateView to onViewCreated

e0e8327

wpmobilebot reviewed Jan 14, 2020

View reviewed changes

...ressLoginFlow/src/main/java/org/wordpress/android/login/SignupBottomSheetDialogFragment.java Show resolved Hide resolved

Remove empty line after brace

db32c52

ashiagr added this to the 14.1 milestone Jan 15, 2020

ashiagr added the [Status] Needs Design Review A designer needs to sign off on the implemented design. label Jan 15, 2020

maxme added this to the 14.2 milestone Jan 27, 2020

ashiagr requested a review from osullivanchris January 28, 2020 07:57

osullivanchris approved these changes Jan 28, 2020

View reviewed changes

ashiagr requested a review from osullivanchris January 29, 2020 06:28

Add max-width for bottom sheet in landscape mode for phones

0666345

osullivanchris approved these changes Jan 29, 2020

View reviewed changes

Merge branch 'develop' into issue/10908-navigation-bar-buttons-not-vi…

081ca1a

…sible-with-signup-sheet # Conflicts: # libs/login/WordPressLoginFlow/src/main/java/org/wordpress/android/login/SignupBottomSheetDialog.java

ashiagr requested a review from theck13 February 6, 2020 09:22

ashiagr requested a review from malinajirka February 7, 2020 04:47

jkmassel modified the milestones: 14.2, 14.3 Feb 10, 2020

ashiagr added 2 commits February 11, 2020 13:45

Fix bottom sheet not disappearing after rotation

f963bc2

Merge branch 'develop' into issue/10908-navigation-bar-buttons-not-vi…

9fa9606

…sible-with-signup-sheet

malinajirka approved these changes Feb 12, 2020

View reviewed changes

theck13 approved these changes Feb 12, 2020

View reviewed changes

theck13 merged commit 34bab43 into develop Feb 12, 2020

theck13 deleted the issue/10908-navigation-bar-buttons-not-visible-with-signup-sheet branch February 12, 2020 13:14

This was referenced Feb 13, 2020

Merge from WPAndroid: Fix Navigation Bar Buttons Not Visible with Signup Sheet wordpress-mobile/WordPress-Login-Flow-Android#33

Merged

Update login lib with fix for signup sheet nav bar buttons visibility issue woocommerce/woocommerce-android#1938

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Navigation Bar Buttons Not Visible with Signup Sheet #11051

Fix Navigation Bar Buttons Not Visible with Signup Sheet #11051

ashiagr commented Jan 7, 2020

peril-wordpress-mobile bot commented Jan 7, 2020 •

edited

Loading

peril-wordpress-mobile bot commented Jan 7, 2020 •

edited

Loading

theck13 left a comment

ashiagr commented Jan 8, 2020 •

edited

Loading

ashiagr commented Jan 13, 2020

malinajirka left a comment •

edited

Loading

malinajirka Jan 13, 2020

ashiagr Jan 13, 2020 •

edited

Loading

ashiagr Jan 14, 2020

ashiagr commented Jan 14, 2020

theck13 commented Jan 14, 2020

malinajirka commented Jan 15, 2020 •

edited

Loading

ashiagr commented Jan 15, 2020 •

edited

Loading

osullivanchris left a comment

ashiagr commented Jan 28, 2020

theck13 commented Jan 28, 2020 •

edited

Loading

ashiagr commented Jan 29, 2020

osullivanchris left a comment

theck13 commented Jan 30, 2020

ashiagr commented Jan 31, 2020 •

edited

Loading

ashiagr commented Feb 6, 2020

theck13 commented Feb 6, 2020

malinajirka commented Feb 10, 2020

jkmassel commented Feb 10, 2020

ashiagr commented Feb 11, 2020 •

edited

Loading

malinajirka left a comment •

edited

Loading

malinajirka commented Feb 12, 2020

Fix Navigation Bar Buttons Not Visible with Signup Sheet #11051

Fix Navigation Bar Buttons Not Visible with Signup Sheet #11051

Conversation

ashiagr commented Jan 7, 2020

To test:

peril-wordpress-mobile bot commented Jan 7, 2020 • edited Loading

peril-wordpress-mobile bot commented Jan 7, 2020 • edited Loading

theck13 left a comment

Choose a reason for hiding this comment

ashiagr commented Jan 8, 2020 • edited Loading

ashiagr commented Jan 13, 2020

malinajirka left a comment • edited Loading

Choose a reason for hiding this comment

malinajirka Jan 13, 2020

Choose a reason for hiding this comment

ashiagr Jan 13, 2020 • edited Loading

Choose a reason for hiding this comment

ashiagr Jan 14, 2020

Choose a reason for hiding this comment

ashiagr commented Jan 14, 2020

theck13 commented Jan 14, 2020

malinajirka commented Jan 15, 2020 • edited Loading

ashiagr commented Jan 15, 2020 • edited Loading

osullivanchris left a comment

Choose a reason for hiding this comment

ashiagr commented Jan 28, 2020

theck13 commented Jan 28, 2020 • edited Loading

ashiagr commented Jan 29, 2020

osullivanchris left a comment

Choose a reason for hiding this comment

theck13 commented Jan 30, 2020

ashiagr commented Jan 31, 2020 • edited Loading

ashiagr commented Feb 6, 2020

theck13 commented Feb 6, 2020

malinajirka commented Feb 10, 2020

jkmassel commented Feb 10, 2020

ashiagr commented Feb 11, 2020 • edited Loading

malinajirka left a comment • edited Loading

Choose a reason for hiding this comment

malinajirka commented Feb 12, 2020

peril-wordpress-mobile bot commented Jan 7, 2020 •

edited

Loading

peril-wordpress-mobile bot commented Jan 7, 2020 •

edited

Loading

ashiagr commented Jan 8, 2020 •

edited

Loading

malinajirka left a comment •

edited

Loading

ashiagr Jan 13, 2020 •

edited

Loading

malinajirka commented Jan 15, 2020 •

edited

Loading

ashiagr commented Jan 15, 2020 •

edited

Loading

theck13 commented Jan 28, 2020 •

edited

Loading

ashiagr commented Jan 31, 2020 •

edited

Loading

ashiagr commented Feb 11, 2020 •

edited

Loading

malinajirka left a comment •

edited

Loading