1706259 - Bring adjustments to the protocol handler modal dialogue per UX review

Status: VERIFIED FIXED

(Firefox :: File Handling, defect, P2)

Description

[Romain Testard [:RT]]

This bug should address the following UX gaps per UX review:

• Make text size match other modal text size
• Increase space between elements (16px)
• Don’t focus element by default ('Choose a different application' and the 'Allow' checkbox on the 'Allow this site to open the ... link' modal get default focussed)
• Adjust checkbox text color to #5B5B66;
• Checkbox needs to be top align, not center align.

Comment 1

[:Gijs (he/him)]

(In reply to Romain Testard [:RT] from comment #0)

This bug should address the following UX gaps per UX review:

• Make text size match other modal text size

As far as I can tell this is already the case. The font size is the same as in e.g. the quit warning for me. Can you clarify what you're comparing with, on what OS, etc. ?

• Increase space between elements (16px)

We can do this here.

• Don’t focus element by default ('Choose a different application' and the 'Allow' checkbox on the 'Allow this site to open the ... link' modal get default focussed)

We need to focus something inside the dialog for accessibility reasons. We can try to avoid showing the focus outline, but then something is focused and the user can't tell that this is the case.

• Adjust checkbox text color to #5B5B66;
• Checkbox needs to be top align, not center align.

This is bug 1705882.

Comment 2

[emanuela [ux]]

(In reply to :Gijs (he/him) from comment #1)

(In reply to Romain Testard [:RT] from comment #0)

This bug should address the following UX gaps per UX review:

• Make text size match other modal text size

As far as I can tell this is already the case. The font size is the same as in e.g. the quit warning for me. Can you clarify what you're comparing with, on what OS, etc. ?

Then good, ignore this :)

• Increase space between elements (16px)

We can do this here.

Perfect. Please note 16px is also the padding. https://cln.sh/CrhRyH

• Don’t focus element by default ('Choose a different application' and the 'Allow' checkbox on the 'Allow this site to open the ... link' modal get default focussed)

We need to focus something inside the dialog for accessibility reasons. We can try to avoid showing the focus outline, but then something is focused and the user can't tell that this is the case.

Proposal. Focus the title without showing the focus ring. If the user stars using keyboard navigation to move in the modal, show the focus ring.

• Adjust checkbox text color to #5B5B66;
• Checkbox needs to be top align, not center align.

This is bug 1705882.

Awesome, great there is already a bug for that.

Comment 3

[:Gijs (he/him)]

(In reply to emanuela [ux] from comment #2)

Proposal. Focus the title without showing the focus ring. If the user stars using keyboard navigation to move in the modal, show the focus ring.

I'll try this in bug 1704882 which was already on file, though it's not entirely straightforward, as the title element isn't the same everywhere, and the code that is doing the focusing is generic and doesn't know about the dialogs' content - it just picks the first focusable element, and text isn't normally focusable.

Comment 4

[:Gijs (he/him)]

Attachment: Bug 1706259 - fix styling in the protocol handler dialogs to be more like the design, r?mconley

This starts using the same spacing as commonDialog for these dialogs
with proton enabled. It also updates the checkbox spacing, alignment
and colour (bug 1705882 will fix the correct colour choice here.)

The spacing is still not ideal in the second dialog (after you click
'Choose Application') but there isn't much we can do about that unless
we get rid of the confirmation text about where to change the preference,
or we start creating ways for the dialog to tell SubDialog to resize
it when items are added/removed, which is not straightforward, so I
decided not to tackle that here.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2df231be7500
fix styling in the protocol handler dialogs to be more like the design, r=mconley

Comment 6

[Alexandru Michis [:malexandru]]

Backed out changeset 2df231be7500 (Bug 1706259) for causing bc failures in browser_parsable_css.js and browser_protocol_ask_dialog_permission.js
Backout link: https://hg.mozilla.org/integration/autoland/rev/5b126bae321ab4055d1cfc2def88d54216b40032
Failures in browser_protocol_ask_dialog_permission.js.
Failures in browser_parsable_css.js.

Failure logs: browser_protocol_ask_dialog_permission.js, browser_parsable_css.js.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6e9e48e3290c
fix styling in the protocol handler dialogs to be more like the design, r=mconley

Comment 8

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/6e9e48e3290c

Comment 9

[:Gijs (he/him)]

Comment on attachment 9217135 [details]
Bug 1706259 - fix styling in the protocol handler dialogs to be more like the design, r?mconley

Beta/Release Uplift Approval Request

• User impact if declined: Required for MR1 / Proton
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Open an external protocol handler dialog (e.g. via zoom or msteams website links)
• List of other uplifts needed: n/a
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): CSS+markup+test only change
• String changes made/needed: Nope

Comment 10

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9217135 [details]
Bug 1706259 - fix styling in the protocol handler dialogs to be more like the design, r?mconley

Approved for 89 beta 4, thanks.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/dc6559cce1fb

Comment 12

[Rares Doghi, Desktop QA]

Hi Gijs, Can you please take a look at my screenshot? I think the Checkbox label Text is a pixel lower than it should be, at least compared to Figma as well as other modals. The rest of the modal looks good.

Please also note that the same issue occur with the "Please choose an Application to open Zoom links" modal.

Comment 13

[Rares Doghi, Desktop QA]

Attachment: checkboxTEXT.png

Comment 14

[Rares Doghi, Desktop QA]

Attachment: ChooseAPP.png

Comment 15

[:Gijs (he/him)]

(In reply to Rares Doghi from comment #12)

Hi Gijs, Can you please take a look at my screenshot? I think the Checkbox label Text is a pixel lower than it should be, at least compared to Figma as well as other modals. The rest of the modal looks good.

Please also note that the same issue occur with the "Please choose an Application to open Zoom links" modal.

This is funny/sad because I used the same CSS as https://hg.mozilla.org/mozilla-central/rev/f48063c60dec#l1.33 which you're using as the "good" comparison, so I don't understand off-hand why the result is not the same. :-(

Anyway, please file a bug with more details for product to prioritize. Please also clarify on which OS(es) you're seeing this, as I can't tell from the screenshot.

Comment 16

[Rares Doghi, Desktop QA]

I have logged Bug 1707538 for the remaining issue I will confirm this issue as Verified as fixed in Beta 89.0b4 as well as our latest nightly build 90.0a1 (2021-04-25). I will update the flags accordingly.