Magnus Melin [:mkmelin] Reporter
	Comment 2 • 4 years ago

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

	Tim Nguyen :ntim
	Comment 3 • 4 years ago

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.

	Magnus Melin [:mkmelin] Reporter
	Comment 8 • 4 years ago

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.

	Magnus Melin [:mkmelin] Reporter
	Comment 10 • 4 years ago

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.

	Magnus Melin [:mkmelin] Reporter
	Comment 12 • 4 years ago

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