1632451 - remove <grid> usage from mail/extensions/openpgp/

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Magnus Melin [:mkmelin]]

We've inherited some <grid>s from enigmail. Need to get rid of them
https://searchfox.org/comm-central/search?q=%3Cgrid&case=false&regexp=false&path=mail%2Fext

Comment 1

[Khushil Mistry [:khushil324]]

Attachment: Bug-1632451_de-grid-openpgp-0.patch

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9142828 [details] [diff] [review]
Bug-1632451_de-grid-openpgp-0.patch

Review of attachment 9142828 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/extensions/openpgp/content/ui/enigmailKeySelection.xhtml
@@ +30,5 @@
>    <script type="application/x-javascript" src="chrome://openpgp/content/ui/enigmailKeySelection.js"/>
>  
>    <vbox class="enigmailCaptionbox" id="dialogHeadline" orient="vertical">
>       <html:h1 id="usersNotFoundCapt"><html:span>&enigmail.usersNotFound.label;</html:span></html:h1>
> +     <!-- This part is not used right now, javascript part is also commented.

I think instead just remove the below. Then in https://searchfox.org/comm-central/rev/32109474297be91179f5f8388e778ef9ab5d38ce/mail/extensions/openpgp/content/ui/enigmailKeySelection.js#203 add a premalink to how this used to look before you removed it.

::: mail/extensions/openpgp/content/ui/enigmailKeygen.xhtml
@@ +70,5 @@
>      <vbox> <!-- Advanced Tab -->
> +      <html:div class="grid-two-column">
> +        <html:div class="flex-items-center">
> +          <label value="&enigmail.keyGen.keyType.label;" control="keyType"/>
> +        </html:div>

I think you should replace this (all three rows) with a 

<html:label for="keyType">&enigmail.keyGen.keyType.label;</html:label>

@@ +81,5 @@
> +          </menulist>
> +        </html:div>
> +        <html:div class="flex-items-center">
> +          <label value="&enigmail.keyGen.keySize.label;" control="keySize"/>
> +        </html:div>

same here

::: mail/extensions/openpgp/content/ui/enigmailMsgBox.xhtml
@@ +47,5 @@
>            <description id="msgtext" context="ctxmenu" noinitialfocus="true" class="enigmailDialogBody"/>
>          </vbox>
>        </vbox>
> +    </html:div>
> +    <html:div class="grid-item-col2" id="checkboxContainer" hidden="hidden">

id first

::: mail/extensions/openpgp/content/ui/keyDetailsDlg.xhtml
@@ +41,5 @@
> +          <html:label for="userId">
> +            &enigmail.keyDetails.userId2.label;
> +          </html:label>
> +        </html:div>
> +        <html:div>

I think you don't need all the extra divs here. Doesn't it work just as well with 

<html:label>...</html:label> <html:input......>

@@ +61,5 @@
> +          </html:label>
> +        </html:div>
> +        <html:div>
> +          <html:input id="fingerprint" class="plain" style="white-space: pre;"
> +                      readonly="true" value="?" multiline="false" size="60"/>

readonly="readonly" 
multiline="multiline"

@@ +70,5 @@
> +          </html:label>
> +        </html:div>
> +        <html:div>
> +          <html:input id="keyCreated" class="plain" style="white-space: pre;"
> +                      readonly="true" value="?" multiline="false" size="60"/>

readonly="readonly" 
multiline="multiline"

@@ +79,5 @@
> +          </html:label>
> +        </html:div>
> +        <html:div>
> +          <html:input id="keyExpiry" class="plain" style="white-space: pre;"
> +                      readonly="true" value="?" multiline="false" size="60"/>

here too

Comment 3

[Tim Nguyen :ntim]

Comment on attachment 9142828 [details] [diff] [review]
Bug-1632451_de-grid-openpgp-0.patch

Review of attachment 9142828 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/extensions/openpgp/content/ui/keyDetailsDlg.xhtml
@@ +61,5 @@
> +          </html:label>
> +        </html:div>
> +        <html:div>
> +          <html:input id="fingerprint" class="plain" style="white-space: pre;"
> +                      readonly="true" value="?" multiline="false" size="60"/>

multiline="multiline" isn't really correct since the attribute value is false.

Instead, I think `multiline="false"` should just be removed as it's a remain of XBL removal, see bug 1513343.

Comment 4

[Magnus Melin [:mkmelin]]

Ah yes, of course, thx!

Comment 5

[Khushil Mistry [:khushil324]]

(In reply to Magnus Melin [:mkmelin] from comment #2)

I think you should replace this (all three rows) with a

<html:label for="keyType">&enigmail.keyGen.keyType.label;</html:label>

Here, associated elements are menulist(XUL). So If we make them <html:label>, we also need to convert menulist to <html:select> else it will create a problem for the screen-reader.

Comment 6

[Magnus Melin [:mkmelin]]

IIRC that should work now (the core bug for it was fixed).

Comment 7

[Khushil Mistry [:khushil324]]

Attachment: Bug-1632451_de-grid-openpgp-1.patch

<html:div class="flex-items-center">
  <html:label for="keyType">
    &enigmail.keyGen.keyType.label;
  </html:label>
</html:div>

We have <html:div> becuase vertical alignment problem of the label.

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9143189 [details] [diff] [review]
Bug-1632451_de-grid-openpgp-1.patch

Review of attachment 9143189 [details] [diff] [review]:
-----------------------------------------------------------------

The menulists need "width:max-content;" to look like before (which was nicer). 
Or maybe there's some easier way too.

Comment 9

[Khushil Mistry [:khushil324]]

Attachment: Bug-1632451_de-grid-openpgp-2.patch

Just added <html:div> wrapper around <menulist>.

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 9143317 [details] [diff] [review]
Bug-1632451_de-grid-openpgp-2.patch

Review of attachment 9143317 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/extensions/openpgp/content/ui/keyDetailsDlg.xhtml
@@ +35,5 @@
>    </broadcasterset>
>  
>    <hbox >
> +    <vbox flex="1">
> +      <html:div class="grid-two-column">

Sorry I didn't notice earlier, but this is really tabular data, so better to use <html:table>. 
Also, with the patch the right column is more to the right than it should be.

Comment 11

[Khushil Mistry [:khushil324]]

Attachment: Bug-1632451_de-grid-openpgp-3.patch

Comment 12

[Magnus Melin [:mkmelin]]

Comment on attachment 9143437 [details] [diff] [review]
Bug-1632451_de-grid-openpgp-3.patch

Review of attachment 9143437 [details] [diff] [review]:
-----------------------------------------------------------------

While were here, can you add these to the dialogs so the inputs get context menus.
<script src="chrome://global/content/globalOverlay.js"/>
<script src="chrome://global/content/editMenuOverlay.js"/>

::: mail/extensions/openpgp/content/ui/keyDetailsDlg.xhtml
@@ +45,5 @@
> +      <html:tr>
> +        <html:th>&enigmail.keyDetails.keyType.label;</html:th>
> +        <html:td>
> +          <html:input id="keyType" class="plain" style="white-space: pre;"
> +                      readonly="readonly" value="?" multiline="false" size="60"/>

please remove the multiline="false" here and the other places, per previous comments

Comment 13

[Khushil Mistry [:khushil324]]

Attachment: Bug-1632451_de-grid-openpgp-4.patch

Comment 14

[Geoff Lankow (:darktrojan)]

This doesn't apply, please rebase it. (Mostly or wholly due to https://hg.mozilla.org/comm-central/rev/446dd720023b51fa656102a0b227b4a19bc8f784.)

Comment 15

[Khushil Mistry [:khushil324]]

Attachment: Bug-1632451_de-grid-openpgp-5.patch

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/964c4361f287
remove <grid> usage from mail/extensions/openpgp/. r=mkmelin