1557996 - When composing an email, double-clicking on fixed width text <tt> brings up the Advanced Property Editor

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P2)

Description

[Jorg K (CEST = GMT+2)]

+++ This bug was initially created as a clone of Bug #1501663 +++

STR:
Start a new HTML e-mail.
Switch the font to "fixed width".
Type a word, like "text".
Double-click on that word.

The advanced property editor for <tt> is launched. That is undesired.

Note: When you type multiple words, only double-clicking on the last work has that effect.

I believe that this was fixed in bug 1501663 but has regressed since. It's broken in TB 68.

Alice, can you please find the regression for us.

Comment 1

[Alice0775 White]

Regression window:
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=caa15de76ecfa1d6718704bd1275fae392078a2c&tochange=d4624bdffc31b8dc5e8ff68ca622b4f4b515831c
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9a3b6d64a64b328ed0de3d6503b99f20d1c94cfb&tochange=9746e0a0a81cc089ff65e30ae902864846cd1b94

Comment 2

[Jorg K (CEST = GMT+2)]

Thanks, Alice. I don't see any editor changes in the regression range. Masayuki-san, can you please take a look. Can this be reproduced in FF somehow? The puzzling thing is that something similar got fixed in bug 1501663, but no test caught the new regression.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

From point of view of editor module, nsIHTMLEditor.getSelectedElement() must work as expected with this test.
https://searchfox.org/mozilla-central/rev/ee806041c6f76cc33aa3c9869107ca87cb3de371/editor/libeditor/tests/test_nsIHTMLEditor_getSelectedElement.html#44-58

However, I find this case:

1. Load https://d-toybox.com/studio/lib/input_event_viewer.html
2. Paste "<p>
   <tt>abc</tt><br>
   </p>" into the <textarea> following <div contenteditable> and click the radio button.
3. Double click "abc" in the editor below toolbars.

Then, selected range is shown as {#text-0}~{P-2}. Perhaps, <tt> element node is returned from nsIHTMLEditor.getSelectedElement(). I think that this is inconsistency behavior. I'll write a patch for it. Then, it'll return null for this case. But I have no idea which change in the range caused this. Looks like that this is long standing inconsistent behavior.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1557996 - Make `HTMLEditor::GetSelectedElement()` not treat an element as selected when it's followed by a <br> element

Currently, HTMLEditor::GetSelectedElement() is not used in mozilla-central
and mainly used for handling double clicks in the editor with its complicated
path. In most cases, users don't want double clicks to cause showing
property dialog in mail composer. Therefore, we must be able to stricter in
the complicated path.

This patch adds new check whether the selected range ends immediately before
a <br> element. If it's end at a <br> element, we shouldn't treat found
element as selected.

Note that when <a href="..."> element is double-clicked, the element itself
is selected like <img> element. So, we don't need to worry about the case
which is that users probably want to update a link with double-clicking since
such case is handled by the first optimized path in the method.

Comment 5

[Jorg K (CEST = GMT+2)]

Thanks, Masayuki-san, the patch works for me, indeed, it's text before the <br> which caused the problem.

That said, it makes me feel uneasy coding another special case that wasn't necessary before without understanding what happened in the regression range. Clearly there were a lot of changes of DOM handling, and these look specifically suspicious:
8d4336d56112f3abf27e6d7ef9843512f6b9a518 Boris Zbarsky — Bug 1432944 part 4. Work with Element, not nsIDOMElement, inside HTMLEditor::GetSelectedElement. r=m_kato
84d651460f98f1e1a973393a2adf1d4cdbc4fa7b Boris Zbarsky — Bug 1432944 part 3. Take an early return from HTMLEditor::GetSelectedElement when we can. r=m_kato
15f093275a0df22eca0c4692c1fbc02fa783d593 Boris Zbarsky — Bug 1432944 part 2. Make nsIHTMLEditor.getSelectedElement return nsISupports. r=m_kato
93c1d149d757d49da158cc64f6eb21fb3406da18 Boris Zbarsky — Bug 1432944 part 1. Stop returning NS_EDITOR_ELEMENT_NOT_FOUND from nsIHTMLEditor::GetSelectedElement. r=m_kato

Note We have that in a try/catch:
https://searchfox.org/comm-central/rev/94dde9f62467a62fdcab1ec8c5acafa14df5151d/editor/ui/composer/content/editor.js#1494
so some failure is expected.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/424ecd7d1403
Make `HTMLEditor::GetSelectedElement()` not treat an element as selected when it's followed by a <br> element r=m_kato

Comment 7

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/424ecd7d1403

Comment 8

[Jorg K (CEST = GMT+2)]

Comment on attachment 9070938 [details]
Bug 1557996 - Make HTMLEditor::GetSelectedElement() not treat an element as selected when it's followed by a <br> element

Beta/Release Uplift Approval Request

• User impact if declined: Some strange behaviour in the TB mail editor. Code apparently not used in FF, see comment #4.
• Is this code covered by automated tests?: Yes
• 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): Doesn't affect FF.
• String changes made/needed: None.

Comment 9

[Julien Cristau [:jcristau]]

Comment on attachment 9070938 [details]
Bug 1557996 - Make HTMLEditor::GetSelectedElement() not treat an element as selected when it's followed by a <br> element

fix for tb, approved for 68.0b10

Comment 10

[Oana Pop-Rus]

https://hg.mozilla.org/releases/mozilla-beta/rev/9d6ef6164ec1

Comment 11

[Daniel Bodea [:danibodea]]

Is this an issue for the Thunderbird product?

Can I have some detailed steps to reproduce, please? I need to reproduce it in order to validate the fix. Thank you.

Comment 12

[Jorg K (CEST = GMT+2)]

If you really want to check this:
Do this in Thundernird Daily versions of TB, said from today/yesterday and 1st June.

STR: Start a new message in paragraph mode (default), set the font to Fixed Width, type "two words". Double-click on "words". In the old version, the advanced property editor for <tt> (tele-type = fixed width) is launched, in the new version, nothing happens, as expected.