Closed Bug 1636971 Opened 4 years ago Closed 4 years ago

Clicking on inserted image in compose window doubles it

Categories

(Thunderbird :: Message Compose Window, defect, P1)

Product:
Thunderbird
Component:
Message Compose Window
Type:
defect

Tracking

(thunderbird78+ fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 79.0
Tracking Flags:
Tracking Status
thunderbird78 + fixed

People

(Reporter: jorgk-bmo, Assigned: rnons)

References

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

1636971.patch
2.44 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
Bug 1636971 - Load EditorOverride.css in HTMLEditor->Init to fix image styles in editor. r=emilio
47 bytes, text/x-phabricator-request
Details | Review

Today's Daily TB 78 on Windows.

Start new composition, type some text, capture a bit of your desktop onto the clipboard, insert image into compose window. So far, so good.

Click onto the image. You will see two images. Click anywhere outside the first or second image, the second image disappears.

Looking with ThunderHTMLedit, the second image isn't really inserted, is seems like some display artefact.

I can see it. I wonder if it's not the zoom-to-full-size/shrink to fit feature somehow behaving badly. Clicking once more it will shrink back to original.

Time to get a regression range, no? Alice, could you please do us the favour?

Flags: needinfo?(alice0775)

Thank you, Alice, much appreciated. Looks a bit like bug 1449522 or bug 1449522 and friends. Emilio, do you see this in Daily? Any idea?

Flags: needinfo?(emilio)

One more thing. What happens at https://www-archive.mozilla.org/editor/midasdemo/ in a FF Daily when you paste a picture there. Same effect?

EDIT: Tried FF Daily of 11 May 2020. No such effect visible there.

See bug 1636971. Cursor should turn to an arrow when moving over image in composition window, and clicking on image should Iproduce a bounding box with resizing handles, so that image can be visually (manually) resized. As of v 77.0b2, cursor no longer becomes an arrow; it remains as text-editing tool. Clicking on, or dragging cursor across image SOMETIMES selects the image -- and when it does, the duplicate image appears.

It's no longer possible to select the image by clicking on it, or to generate a bounding box.

The problem also occurs whether the image has been pasted into composition window OR using "insert image" menu command or new "insert image" button on toolbar.

Above comment should say: "See bug 1637836."

It's also now impossible to select table cells in the compose window -- e.g., when there's a table in a forwarded message (even in the copied header). Is this related to the selection problem in bug 1637836 -- and if so, are these selection problems related to the display-and-selection problems initially noted in this bug?

Assignee: nobody → remotenonsense
Attached patch 1636971.patch — — Splinter Review

I agree with comment 6, the EditorOverride.css is reverted when loading about:blank. I didn't find how to fix it, this patch just loads EditorOverride.css again in InitEditor.

Attachment #9155187 - Flags: review?(mkmelin+mozilla)
Status: NEW → ASSIGNED

Before bug 1449522, EditorOverride.css was loaded in HTMLEditor->Init. And in https://searchfox.org/mozilla-central/source/editor/composer/nsEditingSession.cpp, there is a function call path like this

OnStateChange ---> EndDocumentLoad ---> SetupEditorOnWindow ---> HTMLEditor->Init

I guess OnStateChange will be called when new URI is loaded, so after loading about:blank, HTMLEditor->Init will run again to load EditorOverride.css, that's how it worked before.

Seems to me if this can be fixed from m-c, c-c doesn't need any change. Emilio, do I understand correctly?

Flags: needinfo?(emilio)

Yeah, that sounds right. Probably this chunk of code needs to be moved to a point where the page has loaded or such.

Flags: needinfo?(emilio)
Comment on attachment 9155187 [details] [diff] [review]
1636971.patch

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

Can you look into the mozilla-central fix?
I guess adding an onload event listener in MozEditor.connectedCallback , and applying the stylesheet in the event listener instead?
Attachment #9155187 - Flags: review?(mkmelin+mozilla)

I tried

      // same error for  
      // window.addEventListener("load", () => {
      document.addEventListener("DOMContentLoaded", () => {
        let winUtils = this.contentWindow.windowUtils;
        winUtils.loadSheetUsingURIString(
          "resource://gre/res/EditorOverride.css",
          winUtils.AGENT_SHEET
        );
      });

but get

JavaScript error: chrome://global/content/elements/editor.js, line 66: NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIDOMWindowUtils.loadSheetUsingURIString]

I also tried

      this.contentWindow.addEventListener("load", () => {
      this.contentDocument.addEventListener("DOMContentLoaded", () => {

But they are not triggered at all.

Should I move it back to HtmlEditor.cpp?

Attached patch 1636971.patch (obsolete) — — Splinter Review

Changed to load EditorOverride.css in HTMLEditor->Init

Attachment #9155187 - Attachment is obsolete: true
Attachment #9155847 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9155847 [details] [diff] [review]
1636971.patch

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

I can confirm this works, but I'm not a reviewer for this code. Since this is mozilla-central code, please send the patch through phabricator and have someone like masayuki, m_kato or emilio review it.
Attachment #9155847 - Flags: review?(mkmelin+mozilla) → feedback+
Attachment #9155847 - Attachment is obsolete: true
See Also: → 1637836
Attachment #9155880 - Attachment is obsolete: true
Pushed by ncsoregi@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/55b66cc1f8cd
Load EditorOverride.css in HTMLEditor->Init to fix image styles in editor. r=emilio

https://hg.mozilla.org/mozilla-central/rev/55b66cc1f8cd

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 79.0

Comment on attachment 9155924 [details]
Bug 1636971 - Load EditorOverride.css in HTMLEditor->Init to fix image styles in editor. r=emilio

Beta/Release Uplift Approval Request

  • User impact if declined: Magnus told me TB78 needs this fix to be uplifted to mozilla-beta.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): No new code added, basically reverted some changes.
  • String changes made/needed:
Attachment #9155924 - Flags: approval-mozilla-beta?

The user impact is strange doubling of images (etc.) in the Thunderbird compose window. As far as I know there is no impact on Firefox.

What's the impact/risk for firefox here?

Flags: needinfo?(remotenonsense)

I think no impact/risk for Firefox.

Flags: needinfo?(remotenonsense)

Comment on attachment 9155924 [details]
Bug 1636971 - Load EditorOverride.css in HTMLEditor->Init to fix image styles in editor. r=emilio

I'm afraid that's not a particularly satisfying answer for a late beta uplift.

Attachment #9155924 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment on attachment 9155187 [details] [diff] [review]
1636971.patch

I guess we could also do the easy thing and land this (comm-central workaround) for tb 78 only. It essentially does the same thing as the real fix just in a conceptually wrong place.
Attachment #9155187 - Attachment is obsolete: false
Attachment #9155187 - Flags: review+
Attachment #9155187 - Flags: approval-comm-beta?

If you do that, you need to need to back it out from comm-esr78 and land the M-C fix on our branch on mozilla-esr78 instead which is the messier way to do it.

Emilio, can you provide an uplift request that will satisfy Julien, see comment #27.

Flags: needinfo?(emilio)

(In reply to Jorg K (CEST = GMT+2) from comment #29)

If you do that, you need to need to back it out from comm-esr78 and land the M-C fix on our branch on mozilla-esr78 instead which is the messier way to do it.

Not sure what you mean. Landing attachment 9155187 [details] [diff] [review] on comm-central-beta (only) is all that is needed AFAIK.

So you don't want the "right" solution to go to ESR?

In one way it doesn't really matter, IMHO.

Julien, this code is conditional on a flag that is not used by Firefox, is that a more satisfying answer?

Flags: needinfo?(emilio) → needinfo?(jcristau)

It gets closer, certainly. "I think" and "As far as I know" with no details aren't particularly reassuring, so any extra explanation helps.

Flags: needinfo?(jcristau)
Attachment #9155924 - Flags: approval-mozilla-beta- → approval-mozilla-beta+
Comment on attachment 9155187 [details] [diff] [review]
1636971.patch

Thanks, Julien. We don't need the CC version then.
Attachment #9155187 - Flags: approval-comm-beta?

Comment on attachment 9155924 [details]
Bug 1636971 - Load EditorOverride.css in HTMLEditor->Init to fix image styles in editor. r=emilio

Clearing the uplift flag to take this off pending-uplift queries.

Attachment #9155924 - Flags: approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: