Fix Navigation Bar Buttons Not Visible with Signup Sheet

[ashiagr]

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:

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

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

PR submission checklist:

• [X] I have considered adding unit tests where possible.

• [X] I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

[peril-wordpress-mobile[bot]]

Messages

:book:

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! 1. `cd WordPress-Android` 2. `git checkout issue/10908-navigation-bar-buttons-not-visible-with-signup-sheet` 3. `git subtree push --prefix=libs/login/ https://github.com/wordpress-mobile/WordPress-Login-Flow-Android.git merge/WordPress-Android/11051` 4. Browse to https://github.com/wordpress-mobile/WordPress-Login-Flow-Android/pull/new/merge/WordPress-Android/11051 and open a new PR.

Generated by :no_entry_sign: dangerJS

[peril-wordpress-mobile[bot]]

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

[ashiagr]

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]

Updated screenshots after applying changes requested.

API	Portrait	Landscape
26
27
29

[ashiagr]

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]

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

[malinajirka]

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]

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);
        }
    }

[theck13]

I don't think we should settle for the full-width solution. Restricting the bottom sheet to a maximum width is by design and has accessibility advantages as well. I haven't seen another app using a full-width bottom sheet on a large screen (e.g. tablet in landscape orientation). There are a few alternatives to leaving the bottom sheet full-width.

1. Make bottom sheet full-width only for API 28 and large screen.
2. Make bottom sheet style of navigation bar dark only for API 28 and large screen.
3. Find out why the navigation bar behavior is different on API 28 only.

[ashiagr]

Thanks for great suggestions @theck13 !

1. Make bottom sheet full-width only for API 28 and large screen.
2. Find out why the navigation bar behavior is different on API 28 only.

One thing to note is white navigation bar buttons on light background issue is replicable on large screens/ tablets with version > v27 and not just v28. It means making large screen bottom sheet full-width for all API versions > v27.

Make style of navigation bar dark only for API 28 and large screen.

How about something like this? I changed nav bar background color slightly #e9eff3, so both the buttons and nav bar icons appear to be white on a light blue background. But they're not clearly visible. I'll need hex color value to which I should update nav bar background.

Also updated width to fix layout width issue.

API	Portrait	Landscape
27, 29
26

Two more screenshots with dark nav bar background in different shades of gray (for design review), might help in deciding the color.

Dark Grey	Light Grey

CC @osullivanchris

[osullivanchris]

Hey @ashiagr

There's a few things going on here so I might not have all the context but just one thing I don't think looks right is having the system navigation limited to the width of the sheet. Is that a limitation that we need to have, or a suggestion?

I actually think the images shown for API 26 and below look more like what I would expect. I was looking at the same issue here and you can see that I'm just leaving the system bar unaffected by the sheet.

Hope that makes sense. Let me know if there's more info I'm lacking so that I can better help

[theck13]

I don't think we can guarantee the width or location of the navigation bar button/icons. As an example, navigation bar buttons/icons on the Pixel C are not centered and are much wider than the width of the bottom sheet. Therefore, I don't think setting the navigationBarColor attribute to a dark color will help for all cases. See the screenshots below for illustration.

[ashiagr]

Thanks for taking a look @theck13 , @osullivanchris ! Managed to show full width navigation bar and restrict max width for tablets. Can you please review these screenshots?

Tablet

API	Portrait	Landscape
29
27
26

Phone

API	Portrait	Landscape
29
27
26

In case width of dialog seems odd for tablet, I will need

1. max-width values for different tablet sizes
2. resource qualifiers that we use to differentiate tablets and phones. I find there's always confusion around it and I couldn't find this information here.

[osullivanchris]

The buttons do looked a little squished on tablets in portrait and landscape orientation. We may want to add bottom_sheet_dialog_width to a few dimens.xml files. I defer to the designers on what values they would like for certain screen sizes and orientations.

Chatting to @SylvesterWilmott and he said we have a 512dp container that is used to limit the width of content which is used in various parts of the app, which is what we've done for Simplenote as well.

Will we use that as a starting point? We can add further definitions / breakpoints if it doesn't work in some context/usecase

[ashiagr]

Thanks @osullivanchris !

As a starting point, I took screenshots for the bottom sheet on some of the popular tablets: Portrait Width existing 300dp Landscape Width 512dp

Let me know if we need to change the width on any of these and/or want me to test any other tablet.

Device/ Resolution/ Density/ API	Portrait	Landscape
Pixel C/ 1280x900/ xhdpi/ 27
Nexus 10/ 1280x800/ xhdpi/ 27
Nexus 9/ 1024x768/ xhdpi/ 29
Samsung Galaxy Tab 10/ 800x1280/ mdpi/ 27
Nexus 7 '13/ 600x960/ xhdpi/ 27
Nexus 7 '12/ 600x960 / tvdpi/ 27

[theck13]

@theck13 I see bottom_sheet_dialog_width for sw600dp configuration qualifier defined in WordPress resources. Is it for a reason or can we move it to login library resources? I'll put corresponding sw600dp-land value accordingly.

Since WPBottomSheetDialogFragment is a base class in a library, we don't want to specify certain dimensions because we don't know what the inherited class and layout will be in client apps. The only value for bottom_sheet_dialog_width in the library is 0dp so the container will wrap to the layout's content. It's up to the client apps (e.g. WooCommerce and WordPress) to specify what the dimensions should be based on their desired layout. That's why the values of bottom_sheet_dialog_width in sw600dp and sw600dp-land should be in WordPress and not WordPressLoginFlow.

[ashiagr]

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]

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]

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

[theck13]

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]

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]

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

[theck13]

I haven't found a better solution, but I haven't been researching this either. Therefore, we're going to use this is Simplenote (https://github.com/Automattic/simplenote-android/pull/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]

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

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

1. 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. https://github.com/wordpress-mobile/WordPress-Android/issues/11242

2. 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]

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.

[ashiagr]

2. The bottom sheet doesn't disappear after rotation.

Thanks for catching the bug @malinajirka ! Fixed here f963bc2

[malinajirka]

@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!