Closed Bug 1564270 Opened 3 months ago Closed 3 months ago

After inserting background image, dialogue needs to be OK'ed twice

Categories

(Thunderbird :: Message Compose Window, defect)

defect
Not set

Tracking

(thunderbird68+ fixed, thunderbird69 fixed, thunderbird70 fixed)

RESOLVED FIXED
Thunderbird 70.0
Tracking Status
thunderbird68 + fixed
thunderbird69 --- fixed
thunderbird70 --- fixed

People

(Reporter: jorgk, Assigned: darktrojan)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

STR:
Start a new message.
Format > Page Color and Background.
Select a image from disk.
Hit OK. Image fills the background.
Hit OK again, panel dismissed.

Looks like some fallout from the accept dialogue changes.

Flags: needinfo?(geoff)
Attached patch 1564270-page-background-1.diff (obsolete) — Splinter Review

What a strange piece of code.

Assignee: nobody → geoff
Status: NEW → ASSIGNED
Flags: needinfo?(geoff)
Attachment #9076688 - Flags: review?(jorgk)
Comment on attachment 9076688 [details] [diff] [review]
1564270-page-background-1.diff

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

Thanks for looking into this quickly, just a  question.

::: editor/ui/dialogs/content/EdColorProps.js
@@ +401,5 @@
>        });
>        File.createFromNsIFile(nsFile).then(file => {
>          reader.readAsDataURL(file);
>        });
> +      if (event) {

Is this test necessary? Is it ever called with null/undefined?

@@ +419,2 @@
>    }
> +  if (event) {

And here?
Target Milestone: --- → Thunderbird 70.0
Version: 63 → Trunk
Comment on attachment 9076688 [details] [diff] [review]
1564270-page-background-1.diff

beta->69
release->68 (for the beta continuation I'll do for a week or so)
Attachment #9076688 - Flags: approval-comm-release+
Attachment #9076688 - Flags: approval-comm-beta+
Comment on attachment 9076688 [details] [diff] [review]
1564270-page-background-1.diff

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

::: editor/ui/dialogs/content/EdColorProps.js
@@ +397,2 @@
>          gDialog.BackgroundImageInput.value = reader.result;
>          if (onAccept()) {

Oops, is this recursively calling itself? And not passing event. Should be pass event and lose all the guards you're adding?

We can go that way if you like, it matters not…

Attachment #9076688 - Attachment is obsolete: true
Attachment #9076688 - Flags: review?(jorgk)
Attachment #9076961 - Flags: review?(jorgk)
Comment on attachment 9076961 [details] [diff] [review]
1564270-page-background-2.diff

If you don't mind, I prefer this.
Attachment #9076961 - Flags: review?(jorgk) → review+
Attachment #9076961 - Flags: approval-comm-release+
Attachment #9076961 - Flags: approval-comm-beta+

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/5e8c4b40ca6a
Fix UI for setting a message's background image. r=jorgk DONTBUILD

Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Attachment #9076961 - Flags: approval-comm-release+
Attachment #9076688 - Flags: approval-comm-release+
You need to log in before you can comment on or make changes to this bug.