1529915 - Double-click on word in list/table launches modal property panel

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Ben Bucksch (:BenB)]

Reproduction:

1. Write new email
2. Create a bulleted list
3. Write some text in each list item
4. Double-click on one of the words to select the word
5. Try to edit

Actual result:

• The word gets selected (as expected)
• The composer window is blocked. You cannot edit anything.
• What actually happened is that composer opened the "List properties", i.e. the settings for the bulleted list, like the bullet style. The window is a) modal, blocking input and b) appears on a completely different part of the screen (for me, extreme left, which is completely out of view), and c) is unwanted.

I have no idea why a double-click on a word in a list item should open the list properties.

• This didn't happen before.
• It makes no sense. I use bullet lists very often, and I've almost never needed the list properties. This shouldn't have a shortcut gesture.
• The text of a list item has nothing to do with the list properties. Why would a double-click on the text open this dialog?
• Most importantly, double-clicking on a word to select it has a very common standard operation for text editing that is a convention across many applications and even across platforms, and this "shortcot" broke that.

Expected result:

• The word gets selected
• The dialog does not open.
• I can continue to edit the text

Importance:

• Double-click on a word is a very fundamental feature of the editor, and lists are common in emails. This breaks that combination.
• It's a regression. I've never seen this in old versions of Thunderbird.

Comment 1

[Eva McD]

I have encountered the same exact issue today.
Win 10
Thunderbird Beta 67.0b1 (32-bit)
See: https://support.mozilla.org/en-US/questions/1255445

Additional information:
*Either bulleted or numbered lists respond with the same unexpected behavior.
*Double click while mouse pointer is fully to the right or to the left of the bulleted or numbered list line will also bring up the "List Properties" dialog box. This action could be considered expected.
*The "List Properties" dialog box will close when "Cancel" is clicked, but reappears when a double click on a word in the list occurs again.
*The dialog box does NOT appear on a single click in the list.

Workaround:
*Click, hold and drag to highlight a word in a bulleted or numbered list. This action does not call up the "List Properties" dialog box.

Comment 2

[Magnus Melin [:mkmelin]]

Benjamin, can you investigate this. I think it's likely around here: https://searchfox.org/comm-central/rev/2ddfe0d5bd910e7e7bdbdebb5b826b4863dfd880/editor/ui/composer/content/ComposerCommands.js#2607
... which ends up at https://searchfox.org/comm-central/rev/2ddfe0d5bd910e7e7bdbdebb5b826b4863dfd880/editor/ui/composer/content/ComposerCommands.js#2522

The problem may be that it get's the wrong object (shouldn't get the li when it's just the text), and in that case, problem in this function: https://searchfox.org/comm-central/rev/2ddfe0d5bd910e7e7bdbdebb5b826b4863dfd880/editor/ui/composer/content/editor.js#1489

Comment 3

[Benjamin Flanagin]

Looking at it now.

Comment 4

[Benjamin Flanagin]

Attachment: bug-1529915-fix.patch

The GetObjectForProperties function was ignoring the "#text" nodeName and continuing through the loop till it found ul.

The patch adds a condition to break out of the loop when the "#text" nodeName is found.

Comment 5

[Ben Bucksch (:BenB)]

Sounds good.

Did you check the history of the file why it regressed and which bug that regression cause was intended to fix? Just to make sure we're not regressing another bug again.

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9068447 [details] [diff] [review]
bug-1529915-fix.patch

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

Please remove all the unneeded change, and the whitespaces. If you're using mq then put this in the .hgrc [hooks] section: post-qrefresh.whitespace = hg qdiff | (! egrep --color -C 2 -n '^\+.*\s$')
But the linter would also tell you...

Also make sure it does work to bring up the properties - compare it to how 60 works. If you click on the item, but next to it not on the text, the list properties should come up.

::: editor/ui/composer/content/editor.js
@@ +1527,5 @@
>    while (node) {
>      if (node.nodeName) {
>        var nodeName = node.nodeName.toLowerCase();
> +      // Done when we hit the body or #text 
> +      if (nodeName == "body" || nodName == "#text") break;

typo, so this doesn't work at all.

Also, put the break on the new line (and add braces)

Comment 7

[Magnus Melin [:mkmelin]]

(double click, not click)

Comment 8

[Benjamin Flanagin]

Attachment: bug-1529915-fix.patch

Patch excludes all but the needed code to fix the regression. Functionality has been restored to what is expected and what is exhibited in version 60.6.1.

Per request I've added braces to the edited code.

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9068739 [details] [diff] [review]
bug-1529915-fix.patch

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

Looks good to me, r=mkmelin

Please still fix the commit message 
 * author to include your email address
 * remove the [regression]. 
* add . r=mkmelin at the end

You can add the checkin-needed keyword and someone will check it in for you.

Comment 10

[Ben Bucksch (:BenB)]

Just FYI: Without the patch, if you double-click on a link, it opens the link properties dialog to change the link target. With your patch, that behavior will likely disappear.

Is that intended? I'm not saying that this must block the land of the fix, but such an effect - if I'm right - needs to be understood and documented here in the bug.

There are other property dialogs that this patch will probably disable, e.g. the one for tables, but I think that is actually a good change, for the same reasons as for lists. A double-click on a word in a table should just select a word and not open the table properties, so I think that's good.

Benjamin: It's important to not just fix the bug at hand, but also check for other unintended side-effects of the fix, based on the code and the commit history of that function. That's what my comment 5 aimed at.

Aside from that, I think your patch looks good.

Comment 11

[Benjamin Flanagin]

Thanks other Ben,

I admit I didn't even think about links or tables. Thankfully, and just by beginners luck, you still get the link dialog if you insert a link after the bullet. It also works for tables where in if you double click on the text it doesn't trigger the dialog box, but when you double click anywhere else within the table the table dialog box appears.

As for checking the history, I didn't check any files not pertaining to the editor, and didn't find anything that seemed to cause the issue in them. If I were to hazard a guess it has something to do with how mouse events are interpreted, and how they pass through from child to parent.

Comment 12

[Benjamin Flanagin]

Attachment: bug-1529915-fix.patch

Updated the commit message added:

• Author: author_name <email_address>
• . r=mkmelin

Removed [regression] and changed title to represent that the issue was fixed.

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: bug-1529915-fix.patch

Fixed patch for check-in: Fixed code indentation and HG header.

Comment 14

[Jorg K (CEST = GMT+2)]

(In reply to Ben Bucksch (:BenB) from comment #5)

Did you check the history of the file why it regressed and which bug that regression cause was intended to fix? Just to make sure we're not regressing another bug again.

I'm pretty sure it comes from bug 1501663. I think the fix is acceptable.

Comment 15

[Jorg K (CEST = GMT+2)]

Actually, what I just wrote is wrong. That bug got uplifted to TB 60 and yet there is no problem in TB 60. BenB, are you sure this is a problem in TB 60, I can't see it there.

So let's find the regression:
STR: Open a TB Write windows and add a bulleted list, numbered list or table. Type some test into some bullet points. Then double click some of the text. In TB 60 nothing happens. In TB 68 beta, the properties panel is launched.

Alice, can you please find the regression for us.

Comment 16

[Jorg K (CEST = GMT+2)]

Actually, let's not land this straight away. I suspect this is an editor bug and Masayuki-san might want to fix it elsewhere. We can still paper over it in the short term.

Comment 17

[Alice0775 White]

Regression window:
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=4d7274e18df2de8216378e52a605e00135c14fd7&enddate=2018-04-28%2004:21:16
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7b40283bf1c7a2a3e6a8a5d00156a2f506&tochange=8b2c1fc3d6c348f053fe33a478fa3b1ddb5eb8a6

I tried bisection, however, there are no build in https://tools.taskcluster.net/index/comm.v2.comm-central.pushdate.2018, because they are expired.

And also there are few common-central build in http://archive.mozilla.org/pub/thunderbird/nightly/2018/04/

Comment 18

[Jorg K (CEST = GMT+2)]

Thanks, Alice. So the regression window is almost the entire month of April 2018 :-(

Masayuki, can you please take a look. Should we paper over the issue as proposed in attachment 9069009 [details] [diff] [review]?

Comment 19

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

Well, even before the bug 1501663, I saw the dialog when I editing list items (I was thinking that it was intentional behavior).

According to the regression range, only this patch touches getSelectedElement(), I keep looking what happened...

Comment 20

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

Sorry for taking long time, but I'm still being confused.

EditorDblClick() checks explicit original target of dblclick event. Then, even when the target is a text node, it dispatches cmd_objectProperties command.

Then, GetObjectForProperties() is called by doCommand() for the command.

Finally, GetObjectForProperties() looks for a parent element which can be edited with dialog if nsIHTMLEditor.getSelectedElement() returns null.

So, if mail composer should ignore double click in text node, why does not EditorDblClick() exit if the target is not an element node? So, there may be some cases which need to climbing up the tree from text node. E.g., the command is executed from menu. So, perhaps, if you want to stop showing the dialog on dblclick in text node, you should do it in EditorDblClick().

Comment 21

[Jorg K (CEST = GMT+2)]

Masayuki-san, thanks for the comment. Somehow something changed in M-C, so the behaviour of the mail composer changed, that's why we're asking you. If you think that we should fix it in C-C code, we can do that, thanks for the hints.

The code you're quoting is pretty old, especially
https://searchfox.org/comm-central/rev/7db00200238867751687f2eee2e4f51d34e266dc/editor/ui/composer/content/editor.js#1437
which is referencing bug 193689 which has been fixed.

I'll try a patch in EditorDblClick().

Comment 22

[Jorg K (CEST = GMT+2)]

OK, looking at

  dump(`=== ${event.explicitOriginalTarget.nodeName.toLowerCase()}\n`);
  dump(`=== ${event.target.nodeName.toLowerCase()}\n`);

in EditorDblClick() I see p, td and li when double-clicking on text in a paragraph, table or list. So unless I'm missing something, there's no chance to ignore the text. If not using paragraphs, I get body.

Ben's patch in attachment 9069009 [details] [diff] [review] works, but I didn't land it to investigate the changed M-C behaviour first. Also note bug 1557996.

Comment 23

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

(In reply to Jorg K (GMT+2) from comment #21)

Masayuki-san, thanks for the comment. Somehow something changed in M-C, so the behaviour of the mail composer changed, that's why we're asking you. If you think that we should fix it in C-C code, we can do that, thanks for the hints.

Yeah, probably, nsIHTMLEditor.getSelectedElement() returned different element node, but I still don't find where is different for this case.
Old code: https://searchfox.org/mozilla-central/rev/8e992098fab3fb60cea33c99fb27324fd6900d28/editor/libeditor/HTMLEditor.cpp#2719-2780
Current: https://searchfox.org/mozilla-central/rev/ee806041c6f76cc33aa3c9869107ca87cb3de371/editor/libeditor/HTMLEditor.cpp#2758-2815

Comment 24

[Jorg K (CEST = GMT+2)]

I hope you don't want me to look through the M-C editor code ;-) - In which bug did that change at some stage during April 2018?

Just to double check, I tried
http://archive.mozilla.org/pub/thunderbird/nightly/2018/03/2018-03-18-03-02-01-comm-central/
http://archive.mozilla.org/pub/thunderbird/nightly/2018/04/2018-04-28-04-21-16-comm-central/

In the former, a double-click on a text in a table selected the text, in the latter, it launches the advanced properties editor. So bug 1501663 which landed in Nov. 2018 is not related.

Comment 25

[Jorg K (CEST = GMT+2)]

TB 68 beta:
https://hg.mozilla.org/releases/comm-beta/rev/78c68901684d5db5a9ada045cb86cb9a62aa98b7

I decided to take the hacky patch for the beta so we fix the immediate issue for the users.

Comment 26

[Jorg K (CEST = GMT+2)]

Actually, the fix also doesn't work when you put <tt>text</tt> into a table cell. Then the advanced property editor for tt is launched. Well, maybe that's fixed in bug 1557996 since the table cell also contains a <br>.

Comment 27

[Jorg K (CEST = GMT+2)]

Let's take this now before we forget. Looks like a better solution is not in sight.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e5d177f7e623
Don't launch property dialog when double-clicking on text in list or table. r=mkmelin