1337422 - Stub installer maintenance service checkbox should allow two lines of text

Status: VERIFIED FIXED

(Firefox :: Installer, defect, P2)

Description

[Molly Howell (she/her) (mostly inactive)]

This is a followup to bug 1334786, where support was added for the shortcut and ping checkbox labels in the stub installer to use two lines of text instead of just one. QA discovered that the third checkbox, for installing the maintenance service, also needed this fix applied.

Comment 1

[Molly Howell (she/her) (mostly inactive)]

Attachment: Bug 1337422 - Support multiple text lines for the last stub installer checkbox label.

Review commit: https://reviewboard.mozilla.org/r/110394/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/110394/

Comment 2

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Comment on attachment 8834454 [details]
Bug 1337422 - Support multiple text lines for the last stub installer checkbox label.

https://reviewboard.mozilla.org/r/110394/#review111750

Let me know what you think about the width calculation issue.

::: browser/installer/windows/nsis/stub.nsi:1065
(Diff revision 1)
>          IntOp $0 132 + $ControlTopAdjustment
> +        ; In some locales, this string may be too long to fit on one line.
> +        ; In that case, we'll need to give the control two lines worth of height.
> +        StrCpy $1 12 ; single line height
> +        ${GetTextExtent} "$(INSTALL_MAINT_SERVICE)" $FontNormal $R1 $R2
> +        ${If} $R1 > ${OPTIONS_ITEM_WIDTH_DU}

The docs for GetTextExtentPoint32 (inside GetTextExtent) say it returns "logical units", not the "dialog box units" that OPTIONS_ITEM_WIDTH_DU is in. The existing text is already close to the limit if you compare disgregarding units, if the line gets only a little longer this check trips but the line isn't nearly wrapping; I just had to make it "Install the Nightly background update service a b c d e f".

It may make sense to use something like this, MapDialogRect is the call NSIS uses to convert from dialog units when creating the control.

    ; Remove the the & for the access key
    ${WordReplace} "$(INSTALL_MAINT_SERVICE)" "&" "" "+" $2
    ${GetTextExtent} "$2" $FontNormal $R1 $R2

    StrCpy $2 "${OPTIONS_ITEM_WIDTH_DU}" -1
    IntOp $2 $2 - 14 ; subtract width of checkbox
    ; Convert dialog units to screen units
    System::Call "*(i r2,i,i,i) p .r3"
    System::Call "user32::MapDialogRect(p $HWNDPARENT, p r3)"
    System::Call "*$3(i .r12,i,i,i)" ; output to $R2
    System::Free $3

    ${If} $R1 > $R2

However I don't know how to properly account for the width of the checkbox and the spacing between the checkbox and the label text, the 14 is a fudge that doesn't seem to work exactly with different scaling.

Or it might make sense to just go with what you have, which will be safe as long as display units are > 1px, even if it produces extra spacing sometimes.

Comment 3

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8834454 [details]
Bug 1337422 - Support multiple text lines for the last stub installer checkbox label.

https://reviewboard.mozilla.org/r/110394/#review111750

> The docs for GetTextExtentPoint32 (inside GetTextExtent) say it returns "logical units", not the "dialog box units" that OPTIONS_ITEM_WIDTH_DU is in. The existing text is already close to the limit if you compare disgregarding units, if the line gets only a little longer this check trips but the line isn't nearly wrapping; I just had to make it "Install the Nightly background update service a b c d e f".
> 
> It may make sense to use something like this, MapDialogRect is the call NSIS uses to convert from dialog units when creating the control.
> 
>     ; Remove the the & for the access key
>     ${WordReplace} "$(INSTALL_MAINT_SERVICE)" "&" "" "+" $2
>     ${GetTextExtent} "$2" $FontNormal $R1 $R2
> 
>     StrCpy $2 "${OPTIONS_ITEM_WIDTH_DU}" -1
>     IntOp $2 $2 - 14 ; subtract width of checkbox
>     ; Convert dialog units to screen units
>     System::Call "*(i r2,i,i,i) p .r3"
>     System::Call "user32::MapDialogRect(p $HWNDPARENT, p r3)"
>     System::Call "*$3(i .r12,i,i,i)" ; output to $R2
>     System::Free $3
> 
>     ${If} $R1 > $R2
> 
> However I don't know how to properly account for the width of the checkbox and the spacing between the checkbox and the label text, the 14 is a fudge that doesn't seem to work exactly with different scaling.
> 
> Or it might make sense to just go with what you have, which will be safe as long as display units are > 1px, even if it produces extra spacing sometimes.

So there are two issues here. Both are legitiate and I will fix them.
1) I forgot to filter out the accelerator character from the strings that I'm calling GetTextExtent on. I know about this, there is code _in this very file, that I wrote_, that handles this, I just totally ignored the need for it.
2) The unit conversion does need to be done. The conversion factor is at least 1.5, so it's too large to ignore or just allow as padding.

The architecture selection drop-down ran in to the same issue with accounting for the width of the button, and I used the same kludge that you suggest there; I don't see why it wouldn't be fine here as well.

Comment 4

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8834454 [details]
Bug 1337422 - Support multiple text lines for the last stub installer checkbox label.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110394/diff/1-2/

Comment 5

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8834454 [details]
Bug 1337422 - Support multiple text lines for the last stub installer checkbox label.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110394/diff/2-3/

Comment 6

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Comment on attachment 8834454 [details]
Bug 1337422 - Support multiple text lines for the last stub installer checkbox label.

https://reviewboard.mozilla.org/r/110394/#review112186

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0d5d14b42818
Support multiple text lines for the last stub installer checkbox label. r=agashlin

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0d5d14b42818

Comment 9

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8834454 [details]
Bug 1337422 - Support multiple text lines for the last stub installer checkbox label.

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1334786

[User impact if declined]:
Bug 1340131

[Is this code covered by automated tests?]:
No

[Has the fix been verified in Nightly?]:
Yes

[Needs manual test from QE? If yes, steps to reproduce]:
Bug 1340131 was filed by QA, so they should verify that this fixes it. STR are in bug 1340131 comment 0.

[List of other uplifts needed for the feature/fix]:
None

[Is the change risky?]:
No

[Why is the change risky/not risky?]:
It's small and focused, and entirely cosmetic

[String changes made/needed]:
None

Comment 10

[Gerry Chang [:gchang]]

Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!

Comment 11

[Brindusa Tot, Desktop QA]

Camelia, could you please take a look at this on latest Nightly build?

Comment 12

[Molly Howell (she/her) (mostly inactive)]

Bug 1340131 already shows that Nightly was checked and the issue does not appear there.

Comment 13

[Gerry Chang [:gchang]]

Comment on attachment 8834454 [details]
Bug 1337422 - Support multiple text lines for the last stub installer checkbox label.

Fix a multiple lines issue in stub installer. Aurora53+.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e5daae0b3090

Comment 15

[Camelia Badau [:cbadau], Desktop Test Engineering]

Verified fixed on Windows 7 x64, Windows 10 x64, x86, Windows 8.1 x64, x86 using latest Aurora 54.0a2 (2017-03-10) and Firefox 53 Beta 1 - using fr and en-US builds.