1580715 - remove grid usage from comm/mail/components/compose/content/dialogs/EdDictionary.xul

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, task)

Comment 1

[Khushil Mistry [:khushil324]]

Attachment: Bug-1580715_remove-grid-EdDictionary.patch

Comment 2

[Khushil Mistry [:khushil324]]

Navigate: Mail Compose Window > Spelling > Edit.

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 9092354 [details] [diff] [review]
Bug-1580715_remove-grid-EdDictionary.patch

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

The Add button is now thinner than it used to be. 
Maybe the reason why the Remove and Replace buttons are not aligned like they use to (now higher up than they used to)

The "Replace" functionality is really odd. I think we can remove that. In the odd case people go into this dialog, they can add the new and delete the old. That the currently selected word is replaced with the one in "new word", well that's unexpected.

::: mail/components/compose/content/dialogs/EdDictionary.xul
@@ +30,5 @@
> +                   flex="1"
> +                   height="150px"/>
> +    </vbox>
> +    <vbox flex="1">
> +      <label value=""/>

I'm not sure why this is there, but it's a hack at best. Can we remove these?

@@ +38,5 @@
> +      <vbox flex="1">
> +        <button  id="ReplaceWord" oncommand="ReplaceWord()" label="&ReplaceButton.label;"
> +                 accesskey="&ReplaceButton.accessKey;"/>
> +        <button  id="RemoveWord" oncommand="RemoveWord()" label="&RemoveButton.label;"
> +                 accesskey="&RemoveButton.accessKey;"/>

please fix the double spaces

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9092354 [details] [diff] [review]
Bug-1580715_remove-grid-EdDictionary.patch

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

The Add button is now thinner than it used to be. 
Maybe the reason why the Remove and Replace buttons are not aligned like they use to (now higher up than they used to)

The "Replace" functionality is really odd. I think we can remove that. In the odd case people go into this dialog, they can add the new and delete the old. That the currently selected word is replaced with the one in "new word", well that's unexpected.

::: mail/components/compose/content/dialogs/EdDictionary.xul
@@ +30,5 @@
> +                   flex="1"
> +                   height="150px"/>
> +    </vbox>
> +    <vbox flex="1">
> +      <label value=""/>

I'm not sure why this is there, but it's a hack at best. Can we remove these?

@@ +38,5 @@
> +      <vbox flex="1">
> +        <button  id="ReplaceWord" oncommand="ReplaceWord()" label="&ReplaceButton.label;"
> +                 accesskey="&ReplaceButton.accessKey;"/>
> +        <button  id="RemoveWord" oncommand="RemoveWord()" label="&RemoveButton.label;"
> +                 accesskey="&RemoveButton.accessKey;"/>

please fix the double spaces

Comment 5

[Khushil Mistry [:khushil324]]

Attachment: Bug-1580715_remove-grid-EdDictionary.patch

For Linux, on the trunk, the add button is little thicker and looking little odd. It was thicker than all the other buttons. Now, all the buttons are of the same thickness.

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9092739 [details] [diff] [review]
Bug-1580715_remove-grid-EdDictionary.patch

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

Better, but top of the Remove button is still not aligned with the top of the word box (a few pixels off)

Comment 7

[Khushil Mistry [:khushil324]]

Attachment: Bug-1580715_remove-grid-EdDictionary.patch

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9092742 [details] [diff] [review]
Bug-1580715_remove-grid-EdDictionary.patch

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

Sill not completely right, will attach a screenshot. 
But, looking at the code more, it seems to me this isn't a very good way to convert it. Using flexbox would seem like a more sane structure. 

Also, the close button placement is a bit odd. (Usually, it's separate, not in connection with the list area.)

Comment 9

[Khushil Mistry [:khushil324]]

Attachment: Bug-1580715_remove-grid-EdDictionary.patch

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 9092860 [details] [diff] [review]
Bug-1580715_remove-grid-EdDictionary.patch

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

::: mail/components/compose/content/dialogs/EdDictionary.xul
@@ +42,5 @@
> +                   control="DictionaryList"
> +                   accesskey="&DictionaryList.accessKey;"/>
> +      </div>
> +      <div>
> +      </div>

please remove the empty divs. When you add those, you're doing something wrong.

Should be able to add a rule like below, and add that instead

.grid-item-span-row {
  grid-column: 1 / -1;
}

Comment 11

[Khushil Mistry [:khushil324]]

Attachment: Bug-1580715_remove-grid-EdDictionary.patch

Comment 12

[Khushil Mistry [:khushil324]]

Attachment: Bug-1580715_remove-grid-EdDictionary.patch

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 9093242 [details] [diff] [review]
Bug-1580715_remove-grid-EdDictionary.patch

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

Looks good. r=mkmelin

Unrelated to this bug, when opening this dialog I get

JavaScript error: chrome://messenger/content/messengercompose/EdSpellCheck.js, line 210: NS_ERROR_NOT_INITIALIZED: Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIEditorSpellCheck.GetNextMisspelledWord]

... and closing it I get
JavaScript error: chrome://messenger/content/messengercompose/EdDictionary.xul, line 1: ReferenceError: onClose is not defined

Could you please file bugs. For the latter, should be related to bug 1541789.  https://hg.mozilla.org/comm-central/log/tip/editor/ui/dialogs/content/EdDictionary.js

Comment 14

[Khushil Mistry [:khushil324]]

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

JavaScript error:
chrome://messenger/content/messengercompose/EdSpellCheck.js, line 210:
NS_ERROR_NOT_INITIALIZED: Component returned failure code: 0xc1f30001
(NS_ERROR_NOT_INITIALIZED) [nsIEditorSpellCheck.GetNextMisspelledWord]

This error is shown when editor part is not selected and you click on the Spelling button to check the spelling. I will open the new bug for this.

... and closing it I get
JavaScript error:
chrome://messenger/content/messengercompose/EdDictionary.xul, line 1:
ReferenceError: onClose is not defined

This is resolved with this patch as I have added onclose function in the dialog.

Comment 15

[Magnus Melin [:mkmelin]]

You added the onclose, but not the function?
https://hg.mozilla.org/comm-central/diff/76cba5d0233aded3bbb881c0e6440a0a9f2015ab/editor/ui/dialogs/content/EdDictionary.js

Comment 16

[Khushil Mistry [:khushil324]]

Attachment: Bug-1580715_remove-grid-EdDictionary.patch

Comment 17

[Magnus Melin [:mkmelin]]

The close button is now cancel

Comment 18

[Khushil Mistry [:khushil324]]

Then we need to integrate it with the grid-layout. Can we change the label of the cancel button?

Comment 19

[Magnus Melin [:mkmelin]]

I think you can set buttonlabelaccept="&CloseButton.label;" (and buttons="accept")

Comment 20

[Khushil Mistry [:khushil324]]

Attachment: Bug-1580715_remove-grid-EdDictionary.patch

Comment 21

[Khushil Mistry [:khushil324]]

Attachment: Screenshot 2019-09-17 at 7.05.36 PM.png

Comment 22

[Jorg K (CEST = GMT+2)]

can you please do the commit messages like so:
Bug 1580715 - remove grid usage from EdDictionary.xul. r=mkmelin
That's the standard suggested here:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Creating_a_patch

I know, it's just a small detail, but I prefer everyone to used the same punctuation rules.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0e468cd9a91b
remove grid usage from EdDictionary.xul, remove Replace button. r=mkmelin

Comment 24

[Khushil Mistry [:khushil324]]

(In reply to Jorg K (GMT+2) (reduced availability 14-19 of Sept.) from comment #22)

can you please do the commit messages like so:
Bug 1580715 - remove grid usage from EdDictionary.xul. r=mkmelin
That's the standard suggested here:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Creating_a_patch

I know, it's just a small detail, but I prefer everyone to used the same punctuation rules.

Sure, I will keep this in mind now onwards.