Closed Bug 1529915 Opened 2 years ago Closed 1 year ago

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

Categories

(MailNews Core :: Composition, defect)

defect
Not set
normal

Tracking

(thunderbird68 fixed, thunderbird69 fixed)

RESOLVED FIXED
Thunderbird 69.0
Tracking Status
thunderbird68 --- fixed
thunderbird69 --- fixed

People

(Reporter: BenB, Assigned: benjamin)

Details

Attachments

(1 file, 3 obsolete files)

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.

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.

Assignee: nobody → benjamin

Looking at it now.

Attached patch bug-1529915-fix.patch (obsolete) — Splinter Review

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.

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 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)
Attachment #9068447 - Flags: review-

(double click, not click)

Attached patch bug-1529915-fix.patch (obsolete) — Splinter Review

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.

Attachment #9068447 - Attachment is obsolete: true
Attachment #9068739 - Flags: review?(mkmelin+mozilla)
Attachment #9068739 - Flags: feedback+
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.
Attachment #9068739 - Flags: review?(mkmelin+mozilla)
Attachment #9068739 - Flags: review+
Attachment #9068739 - Flags: feedback+

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.

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.

Attached patch bug-1529915-fix.patch (obsolete) — Splinter Review

Updated the commit message added:

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

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

Attachment #9068739 - Attachment is obsolete: true

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

Attachment #9069009 - Flags: review+
Attachment #9069009 - Flags: approval-comm-beta+
Attachment #9068967 - Attachment is obsolete: true

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

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.

Flags: needinfo?(alice0775)
Summary: [regression] Double-click on word in bulleted list blocks composer by opening a modal dialog → Double-click on word in list/table launches modal property panel

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.

Keywords: checkin-needed
OS: Linux → All
Hardware: x86_64 → All

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]?

Flags: needinfo?(masayuki)

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

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().

Flags: needinfo?(masayuki)
Flags: needinfo?(jorgk)
Flags: needinfo?(benjamin)

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().

Flags: needinfo?(jorgk)

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.

(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

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.

Flags: needinfo?(benjamin)

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.

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

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

Keywords: checkin-needed

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

Status: NEW → RESOLVED
Closed: 1 year ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 69.0
You need to log in before you can comment on or make changes to this bug.