1961593 - [DoH on Android] DoH settings page toolbar is inconsistent

Status: RESOLVED FIXED

(Firefox for Android :: Settings, defect)

Description

[Michel Le Bihan]

Steps to reproduce

1. Go to settings -> about Firefox
2. Tap the logo 5 times
3. Go back to settings and in the hidden settings category that now appeard, enable DoH settings
4. Go back again to settings and enter the DNS over HTTPS screen

Expected behavior

The DoH settings page animation and spacing should be consistent with other settings screens

Actual behavior

When entering the DoH settings screen, the top bar is replaced in the animation. When entering other settings screens, the top bar is kept and only its title is changed.

The spacing between the top bar and the screen content is much smaller than in other settings screens.

Device information

• Firefox version: 139
• Android device model: Sony Xperia 5 V
• Android OS version: 15

Any additional information?

Comment 1

[Michel Le Bihan]

Is there a reason why the DoH settings screen has it's own toolbar instead of reusing and updating the existing one with:

override fun onResume() {
    super.onResume()
    showToolbar(getString(R.string.preference_doh_title))
}

like it's done in all other setting fragments?

Comment 2

[Sean Kim [:sekim]]

(In reply to Michel Le Bihan from comment #1)

Is there a reason why the DoH settings screen has it's own toolbar instead of reusing and updating the existing one with:

override fun onResume() {
    super.onResume()
    showToolbar(getString(R.string.preference_doh_title))
}

like it's done in all other setting fragments?

Great catch, it should be updated using the existing one.

Comment 3

[Sean Kim [:sekim]]

Attachment: Bug 1961593 - Reuse and update the existing toolbar in DoH on Fenix r=sfamisa

Comment 4

[Sean Kim [:sekim]]

Oops my bad, I didn't check the ASSIGNED. I will abandon my revision, please feel free to reference my code.

Comment 5

[Michel Le Bihan]

That's OK. I haven't done it yet. Please keep it.

Comment 6

[Michel Le Bihan]

Why is the spacing on the left side of the radio buttons so large? In the HTTPS only mode screen it's much smaller, however it's the same as in the advanced tracking protection screen.

Comment 7

[Sean Kim [:sekim]]

(In reply to Michel Le Bihan from comment #6)

Why is the spacing on the left side of the radio buttons so large? In the HTTPS only mode screen it's much smaller, however it's the same as in the advanced tracking protection screen.

Seems like we aren't using Jetpack Compose for HTTPS Only (hence it isn't really one-to-one). I will double check if there are discrepancies in the paddings. Could you kindly file a new bug for this issue?

Comment 8

[Michel Le Bihan]

Could you kindly file a new bug for this issue?

You mean that it's incorrect in the HTTPS only screen?

Comment 9

[Sean Kim [:sekim]]

(In reply to Michel Le Bihan from comment #8)

Could you kindly file a new bug for this issue?

You mean that it's incorrect in the HTTPS only screen?

I think it should match the HTTPS only screen (DoH is probably incorrect).

Comment 10

[Sean Kim [:sekim]]

This is where we set the radio buttons.

We import org.mozilla.fenix.compose.button.RadioButton, so I don't think it is necessarily incorrect. Segun, would this be an issue?

Comment 11

[Segun Famisa [:segun]]

(In reply to Michel Le Bihan from comment #6)

Why is the spacing on the left side of the radio buttons so large? In the HTTPS only mode screen it's much smaller, however it's the same as in the advanced tracking protection screen.

The spacing on the DoH screen is exactly as designed. And as you rightly noticed, the Enhanced Tracking Protection screen has similar spacing as the DoH screen.

(In reply to Sean Kim from comment #9)

(In reply to Michel Le Bihan from comment #8)

Could you kindly file a new bug for this issue?

You mean that it's incorrect in the HTTPS only screen?

I think it should match the HTTPS only screen (DoH is probably incorrect).

I can't comment on why the HTTPS-Only screen is different, but as far as I know, the spacing is not a bug or incorrect, but rather, by design.

I can try to find out why it's different.

Comment 12

[Michel Le Bihan]

I reported https://bugzilla.mozilla.org/show_bug.cgi?id=1962221.

Another thing that I noticed is that the spacing between the globe and the Exceptions text is much larger tin the Advanced tracking protection screen than it is in DoH settings. However, the former one seems way too large.

Comment 13

[Sean Kim [:sekim]]

Unassigned myself, please feel free to take the patches and land them after review!

Comment 14

[BugBot [:suhaib / :marco]]

The severity field is not set for this bug.
:royang, could you have a look please?

For more information, please visit BugBot documentation.

Comment 15

[Valentin Gosu [:valentin]]

Attachment: Bug 1961593 - Make the DohSettingsScreend and ExceptionsListScreen scrollable r=sfamisa

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a9c1b731e886
Reuse and update the existing toolbar in DoH on Fenix r=sfamisa,android-reviewers,android-l10n-reviewers,delphine
https://hg.mozilla.org/integration/autoland/rev/5af57149327b
Make the DohSettingsScreend and ExceptionsListScreen scrollable r=sfamisa,android-reviewers

Comment 17

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/a9c1b731e886
https://hg.mozilla.org/mozilla-central/rev/5af57149327b