Closed Bug 1557996 Opened 4 months ago Closed 4 months ago

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

Categories

(Core :: Editor, defect, P2)

60 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox68 --- fixed
firefox69 --- fixed

People

(Reporter: jorgk, Assigned: masayuki)

Details

Attachments

(1 file)

+++ 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.

Flags: needinfo?(alice0775)

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.

Flags: needinfo?(masayuki)

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.

Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Flags: needinfo?(masayuki)

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.

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.

Attachment #9070938 - Attachment description: Bug 1557996 - Make `HTMLEditor::GetSelectedElement()` not treat as an element selected when found node is followed by a <br> element → Bug 1557996 - Make `HTMLEditor::GetSelectedElement()` not treat an element as selected when it's followed by a <br> element
Pushed by masayuki@d-toybox.com:
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
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

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.
Attachment #9070938 - Flags: approval-mozilla-beta?

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

Attachment #9070938 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

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.

Flags: needinfo?(jorgk)

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.

Flags: needinfo?(jorgk)
You need to log in before you can comment on or make changes to this bug.