1563092 - Use HTML input instead of XUL textbox in EdImageProps and edImage.inc.xul

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Alessandro Castellani [:aleca]]

• edImage.inc.xul: https://searchfox.org/comm-central/rev/94d2c771f935e9402ca3d19a43fafeeaa8f0ac4f/editor/ui/dialogs/content/edImage.inc.xul#15
• EdImageProps.xul
• EdInputImage.xul

Comment 1

[Jorg K (CEST = GMT+2)]

EdInputImage.xul isn't used in TB any more.

Comment 2

[Khushil Mistry [:khushil324]]

Attachment: Bug-1563092_convert-textbox-to-input-EdImageProps.patch

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 9097187 [details] [diff] [review]
Bug-1563092_convert-textbox-to-input-EdImageProps.patch

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

Under appearance, there is now sever misalignment. 
Under Dimensions too (but that's trunk also). I guess from de-grid. 

I think e.g. https://searchfox.org/comm-central/rev/266e9cc242cd0de076e85eb4aa0b8392fcb2ca01/mail/components/compose/content/dialogs/edImage.inc.xul#109-136 is wrong. It's just placing boxes and hoping the content will be at the right place. Looks like it should really be a table.

::: mail/components/compose/content/dialogs/edImage.inc.xul
@@ +125,4 @@
>              </hbox>
>            </vbox>
>            <vbox>
> +            <html:input id="widthInput" type="text"

let's use number

@@ +128,5 @@
> +            <html:input id="widthInput" type="text"
> +                        class="narrow input-inline"
> +                        onchange="constrainProportions(this.id,'heightInput')"
> +                        aria-labelledby="widthLabel"/>
> +            <html:input id="heightInput" type="text"

and here

Comment 4

[Khushil Mistry [:khushil324]]

Attachment: Bug-1563092_convert-textbox-to-input-EdImageProps.patch

Comment 5

[Khushil Mistry [:khushil324]]

Attachment: Bug-1563092_convert-textbox-to-input-EdImageProps.patch

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9097579 [details] [diff] [review]
Bug-1563092_convert-textbox-to-input-EdImageProps.patch

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

Dimensions is now ok. But Appearance still looks wrong

::: mail/components/compose/content/dialogs/edImage.inc.xul
@@ +143,5 @@
> +                            aria-labelledby="heightLabel"/>
> +              </html:td>
> +              <html:td>
> +                <menulist id="heightUnitsMenulist"
> +                          oncommand="doDimensionEnabling();" />

please remove the space before />

Comment 7

[Khushil Mistry [:khushil324]]

Attachment: Bug-1563092_convert-textbox-to-input-EdImageProps.patch

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9097583 [details] [diff] [review]
Bug-1563092_convert-textbox-to-input-EdImageProps.patch

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

Please add a min="0" to the number inputs

::: mail/components/compose/content/dialogs/edImage.inc.xul
@@ +168,5 @@
> +              </html:th>
> +              <html:td>
> +                <html:input id="imageleftrightInput" type="number"
> +                            class="narrow input-inline"
> +                            aria-labelledby="leftrightLabel"/>

apparently these are setting hspace and vspace attributes, which should not really be used, see https://developer.mozilla.org/en-US/docs/Web/API/HTMLImageElement/hspace

Can you file a followup bug to use style + margin for this instead

@@ +172,5 @@
> +                            aria-labelledby="leftrightLabel"/>
> +              </html:td>
> +              <html:td>
> +                <label id="leftrighttypeLabel"
> +                       value="&pixelsPopup.value;"/>

no need for label, just &pixelsPopup.value;

@@ +190,5 @@
> +                            aria-labelledby="topbottomLabel"/>
> +              </html:td>
> +              <html:td>
> +                <label id="topbottomtypeLabel"
> +                       value="&pixelsPopup.value;"/>

just &pixelsPopup.value;

Comment 9

[Khushil Mistry [:khushil324]]

Attachment: Bug-1563092_convert-textbox-to-input-EdImageProps.patch

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: number-box.png

Your number boxes don't look good on Windows. You want me to land like this or fix it?

Comment 11

[Jorg K (CEST = GMT+2)]

Richard, can you please take a look.

Comment 12

[Jorg K (CEST = GMT+2)]

They look equally bad on the width and height of the horizontal ruler landed recently. I guess we need to make all the boxes a little taller in a separate bug. I'll land it later then.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a05c073aca21
replace <textbox> in EdImageProps.xul and edImage.inc.xul. r=mkmelin

Comment 14

[Jorg K (CEST = GMT+2)]

Backout:
https://hg.mozilla.org/comm-central/rev/7cd8521f4423f9c5c86aa03ab58ef73960737352

I had to back this out for various MozMill failures on Mac and Windows:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=d85a8ba1bbd28de53fe3e1a96042c3536369c5ad

With the backout, those tests pass again:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4c531516fa221d8de177df12406fe805bac54fec

Comment 15

[Khushil Mistry [:khushil324]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=878bd11330bc5765e75a8dffba581f5cabbdaeeb

Comment 16

[Khushil Mistry [:khushil324]]

Attachment: Bug-1563092_convert-textbox-to-input-EdImageProps.patch

I have changed all the onchange to oninput. We can use onchange only on html:select.

Comment 17

[Jorg K (CEST = GMT+2)]

Comment on attachment 9098603 [details] [diff] [review]
Bug-1563092_convert-textbox-to-input-EdImageProps.patch

Thanks for that. It was very strange that it failed on Mac/Windows but not on Linux. I'm a bit strapped for time right now, so I won't try it. Your try will tell us whether it works now. Mac is looking good so far.

Comment 18

[Magnus Melin [:mkmelin]]

Well onchange and oninput are different. For onchange it only updates after you leave the field (blur), but oninput will trigger for each char you trigger. Some changes might be needed so the tests do not fail, but updating the preview as you type isn't very productive...

Comment 19

[Magnus Melin [:mkmelin]]

Comment on attachment 9098603 [details] [diff] [review]
Bug-1563092_convert-textbox-to-input-EdImageProps.patch

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

Isn't worse than before, but we should revisit using onchange instead for where it should be

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6f766cbe59fd
replace <textbox> in EdImageProps.xul and edImage.inc.xul. r=mkmelin

Comment 21

[Jorg K (CEST = GMT+2)]

We'd better remove all the textboxes before M-C remove the binding. We can worry about the fine print in another bug. Also see comment #10.

Comment 22

[Khushil Mistry [:khushil324]]

This was creating problem: https://searchfox.org/comm-central/source/mail/test/mozmill/composition/test-image-insertion-dialog.js#40
So if we are using onchange everywhere, we need to change this: https://searchfox.org/comm-central/source/mail/test/mozmill/shared-modules/KeyboardHelpers.jsm#20
I will file the follow-up bug to convert all the html:input to use onchange and change input_value function to reflect the values.

Comment 23

[Magnus Melin [:mkmelin]]

You'd only have to blur() after typing.
But perhaps the code is right, that people may paste the url, and expect to get a preview even before clicking elsewhere.

Comment 24

[Khushil Mistry [:khushil324]]

So do we want to change the oninput to onchange here and elsewhere?

Comment 25

[Magnus Melin [:mkmelin]]

Let's just leave it as is.

Comment 26

[Khushil Mistry [:khushil324]]

Okay Cool.