Closed Bug 1563092 Opened 1 year ago Closed 5 months ago

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

Categories

(Thunderbird :: General, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 71.0

People

(Reporter: aleca, Assigned: khushil324, Mentored)

References

Details

Attachments

(2 files, 5 obsolete files)

Summary: Use HTML input instead of XUL textbox in edImage.inc.xul → Use HTML input instead of XUL textbox in editor/ui/dialogs/content/**image**

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

Summary: Use HTML input instead of XUL textbox in editor/ui/dialogs/content/**image** → Use HTML input instead of XUL textbox in mail/components/compose/content/dialogs/**image**
Assignee: nobody → khushil324
Summary: Use HTML input instead of XUL textbox in mail/components/compose/content/dialogs/**image** → Use HTML input instead of XUL textbox in EdImageProps and edImage.inc.xul
Attachment #9097187 - Flags: review?(mkmelin+mozilla)
Status: NEW → ASSIGNED
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
Attachment #9097187 - Flags: review?(mkmelin+mozilla) → review-
Attachment #9097187 - Attachment is obsolete: true
Attachment #9097577 - Flags: review?(mkmelin+mozilla)
Attachment #9097577 - Attachment is obsolete: true
Attachment #9097577 - Flags: review?(mkmelin+mozilla)
Attachment #9097579 - Flags: review?(mkmelin+mozilla)
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 />
Attachment #9097579 - Flags: review?(mkmelin+mozilla) → review-
Attachment #9097579 - Attachment is obsolete: true
Attachment #9097583 - Flags: review?(mkmelin+mozilla)
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;
Attachment #9097583 - Flags: review?(mkmelin+mozilla) → review+
Attachment #9097583 - Attachment is obsolete: true
Attachment #9098205 - Flags: review+
Keywords: checkin-needed
Attached image number-box.png

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

Richard, can you please take a look.

Keywords: checkin-needed

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.

Keywords: checkin-needed

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/a05c073aca21
replace <textbox> in EdImageProps.xul and edImage.inc.xul. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 71.0
Status: RESOLVED → REOPENED
Flags: needinfo?(khushil324)
Resolution: FIXED → ---

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

Attachment #9098205 - Attachment is obsolete: true
Attachment #9098603 - Flags: review?(mkmelin+mozilla)
Attachment #9098603 - Flags: feedback?(jorgk)
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.
Attachment #9098603 - Flags: feedback?(jorgk)

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 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
Attachment #9098603 - Flags: review?(mkmelin+mozilla) → review+

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

Status: REOPENED → RESOLVED
Closed: 5 months ago5 months ago
Resolution: --- → FIXED

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.

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.

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.

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

Let's just leave it as is.

Okay Cool.

You need to log in before you can comment on or make changes to this bug.