Closed Bug 1501663 Opened 6 years ago Closed 6 years ago

When composing an email, double-clicking on italic text brings up the Advanced Property Editor or Link Properties

Categories

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

Product:
Core
Component:
DOM: Editor
Version:
60 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla65
Tracking Flags:
Tracking Status
firefox65 --- fixed

People

(Reporter: andy, Assigned: masayuki)

References

Details

Attachments

(9 files)

1501663-debug.patch
3.17 KB, patch
Details | Diff | Splinter Review
Bug 1501663 - part 1: HTMLEditor::GetSelectedElement() should return nullptr when first found element is not what the caller is looking for
47 bytes, text/x-phabricator-request
Details | Review
Bug 1501663 - part 2: HTMLEditor::GetSelectedElement() should return nullptr if 2 or more elements are in selected range
47 bytes, text/x-phabricator-request
Details | Review
Bug 1501663 - part 3: HTMLEditor::GetElementOrParentByTagNameInternal() shouldn't return <a> element when it's looking for nsGkAtoms::href or nsGkAtoms::anchor but found <a> element does not match with <a href="..."> or <a name="...">
47 bytes, text/x-phabricator-request
Details | Review
Bug 1501663 - part 4: Clean up first-half of HTMLEditor::GetSelectedElement()
47 bytes, text/x-phabricator-request
Details | Review
Bug 1501663 - part 5: Rewrite the loop in HTMLEditor::GetSelectedElement() with |for|
47 bytes, text/x-phabricator-request
Details | Review
Bug 1501663 - part 6: Get rid of |foundElmentInRange|
47 bytes, text/x-phabricator-request
Details | Review
Bug 1501663 - part 7: HTMLEditor::GetSelectedElement() shouldn't return element node when Selection starts before the element node
47 bytes, text/x-phabricator-request
Details | Review
Patch for ESR60 (only for comm-central)
46.49 KB, patch
m_kato
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/70.0.3538.67 Safari/537.36 Steps to reproduce: Edit email. Write some text. Double-click on one word so that it's selected. Make it italic. Cancel the selection by clicking elsewhere. Now double-click on the italicised word again. Rather than just selecting the word, Thunderbird opens the "link properties" dialog box, as if you've clicked on a hyperlink. The same result occurs when double-clicking on bold or underlined text. Thunderbird 60.2.1 running on Windows 10. This has only happened since the last update. Actual results: See above. Link dialog box appears inappropriately when double-clicking on a word with italic/bold/underline formatting. Expected results: The link properties dialog box should not appear. The text is not a link and I've no intention of making it into one. The normal reason for clicking on a word is to edit it (either remove a style, apply a different one, or delete/replace the word).
Hmm, for me, double clicking opens the "Advanced Property Editor" instead of just selecting the word. I've seen this frequently in TB 60.x but haven't worked out exactly how to make it happen reliably. Masayuki-san: What has changed with double-clicking on words, something triggers the "Advanced Property Editor" instead of just selecting the word. It's a bit annoying at times.
Flags: needinfo?(masayuki)
Me too. Advanced Property Editor opens on my environment. Looks like that this is expected behavior: https://searchfox.org/comm-central/rev/e5f6d61e3151a45a9c5fc8c43133d0172080bf18/editor/ui/composer/content/editor.js#1587,1612 https://searchfox.org/comm-central/rev/e5f6d61e3151a45a9c5fc8c43133d0172080bf18/editor/ui/composer/content/ComposerCommands.js#2867,2883,2949-2950 On the other hand, it retrieves an element with nsIHTMLEditor.getSelectedElement(): https://searchfox.org/comm-central/rev/e5f6d61e3151a45a9c5fc8c43133d0172080bf18/editor/ui/composer/content/editor.js#1646,1654,1656-1657,1660-1663 I touched this API at bug 1482019. So, it perhaps changed the behavior. Thunderbird calls it with empty string. I'm not sure what is expected.
Flags: needinfo?(masayuki)
Only attachment 8999610 [details] changes the behavior. Previously, it may return a text node even though its name is "getSelectedElement". Now, it returns null if text node is selected. If this returns nullptr, the behavior of GetObjectForProperties() must be really different because it looks for specific parent element: https://searchfox.org/comm-central/rev/e5f6d61e3151a45a9c5fc8c43133d0172080bf18/editor/ui/composer/content/editor.js#1646,1654,1656,1663,1668-1669,1671,1693-1696,1698 If Tb does not modify Selection behavior, clicking a italic word should select text nodes as far as I've tested with: https://d-toybox.com/studio/lib/input_event_viewer.html Then, getSelectedElement() must return null.
Well, so, Selection must be like this: foo [ <i>bar</i> ] buz So, <i> won't be returned by getSelectedElement(). So, the dialog should depend on parent elements. I don't know what DOM tree is created in the editor. Can I see the DOM tree with add-on or something in composing window?
OK, I finally pinned down a reproducible case. Alice, can you please find the regression for us. STR Start a new composition, make sure you're entering text in "Variable Width", so no font/bold/italics. Type |This is text|. Double click the words. Expected and actual result: The word is selected. Now select the entire text and make it "Fixed Width". Internally this wraps the text into <tt>. Double click the words again. When you double-click the last word, "text", the Advanced Property Editor opens. That is undesired. Note: Instead of using "Fixed Width", you can also use italics with the same undesired effect. In TB 52 the Advanced Property Editor does not open, but it opens in TB 60. So some behaviour changed and the regression pre-dates bug 1482019. (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #4) > Can I see the DOM tree with add-on or something in composing window? You can used "Tools > Developer Tools > Developer Toolbox" and select the messengercompose.xul document.
Flags: needinfo?(alice0775)
Sorry, I'm hijacking this bug. I can't get link properties to open, only the annoying Advanced Property Editor.
Status: UNCONFIRMED → NEW
Component: Untriaged → Message Compose Window
Ever confirmed: true
Summary: When composing an email, double-clicking on italic text brings up the "link properties" dialog box → When composing an email, double-clicking on italic text brings up the Advanced Property Editor
question 1. "Start a new composition", html or plain text? 2. "Double click the words", where/which? 3. what does "Variable Width" , "Fixed Width" mean?
Flags: needinfo?(alice0775)
ni,Jorg K, see comment#7
Flags: needinfo?(jorgk)
(In reply to Alice0775 White from comment #8) > ni,Jorg K, > see comment#7 never mind, i can reproduce.
Flags: needinfo?(jorgk)
Thanks for getting to it so quickly. HTML, "Variable/Fixed Width" is in the font selector.
If it's any help... Some people have noted that they get Advanced Property Editor rather than Link Properties. After a bit more fiddling it seems that: - if the italicised word is just one word in a a line of multiple words, double-clicking on it gives you Link Properties. - if the italicised word is on a line by itself, double-clicking on it gives you Advanced Property Editor. - if there are multiple adjacent italicised words, the bug only applies to the *last* of those words. here's how the HTML looks for an email demonstrating all three conditions described above: --- <html> <head> <meta http-equiv="content-type" content="text/html; charset=UTF-8"> </head> <body text="#000000" bgcolor="#FFFFFF"> <p> Here is a line with one <i>italicised</i> word. Double-clicking on it will open the Link Properties dialog.</p> <p>Double-clicking on the following word will open the Advanced Properties Editor:<br> </p> <p><i>italics</i></p> <p>This is a line with<i> three italicised words</i> in a row. Double-clicking on the last one will open the Link Properties dialog. Double-clicking on either of the first two will simply select them.<br> </p> </body> </html> --- Incidentally I have tried disabling all add-ons and have confirmed that the bug still occurs.
STR A: start a new composition with html type "this is text" in e-mail body double click on "text" and make it "fixed with" double click on "text" again, if not reproduce, double click on "text" again --- Advanced Property Editor pops up STR B: start a new composition with html type "this is text" in e-mail body double click on "is" and make it "fixed with" double click on "text" --- link properties pops up
Many thanks Alice, as always. So this look like something went wrong in M-C bug 1418076 or bug 1432977 and our related C-C activities in bug 1433542 and bug 1433193. Masayuki-san already did a lot of research in comment #2, I'll just have to see what element is returned here: https://searchfox.org/comm-central/rev/e5f6d61e3151a45a9c5fc8c43133d0172080bf18/editor/ui/composer/content/editor.js#1608 in EditorDblClick().
Summary: When composing an email, double-clicking on italic text brings up the Advanced Property Editor → When composing an email, double-clicking on italic text brings up the Advanced Property Editor or Link Properties
(In reply to Alice0775 White from comment #13) > STR B: > start a new composition with html > type "this is text" in e-mail body > double click on "is" and make it "fixed with" > double click on "text" > --- link properties pops up You mean: double-click on "is", right? That does it for me.
(In reply to Jorg K (GMT+2) from comment #15) > (In reply to Alice0775 White from comment #13) > > STR B: > > start a new composition with html > > type "this is text" in e-mail body > > double click on "is" and make it "fixed with" > > double click on "text" > > --- link properties pops up > You mean: double-click on "is", right? That does it for me. yes. but i am on daily x64 windows10.
Here's a debug patch. STR A: start a new composition with html type "this is text" in e-mail body double click on "text" and make it "fixed with" Debug: === element (1) #text === element (2) #text === element (5) TT === element (5) TT === element (3) TT STR B: start a new composition with html type "this is text" in e-mail body double click on "is" and make it "fixed with" double click on "is" Debug: === element (1) #text === element (2) #text === element (4) TT I don't understand who this code try { element = GetCurrentEditor().getSelectedElement("href"); } catch (e) {} if (element) dump(`=== element (4) ${element.nodeName}\n`); if (element) goDoCommand("cmd_link"); brings back the TT. Also try { element = editor.getSelectedElement(""); } catch (e) {} if (element) dump(`=== element (5) ${element.nodeName}\n`); brings back the TT. Something is work in getSelectedElement().
Flags: needinfo?(masayuki)
Sorry, one line missing: STR A: start a new composition with html type "this is text" in e-mail body double click on "text" and make it "fixed with" double click on "text" again.
Grrr, more typos: Something isn't working in getSelectedElement().
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #3) > If Tb does not modify Selection behavior, clicking a italic word should > select text nodes as far as I've tested with: > https://d-toybox.com/studio/lib/input_event_viewer.html > Then, getSelectedElement() must return null. Yes, my debug also shows that #text is initially returned, but then getSelectedElement() returns <TT> if we use "fixed witdh", or it would return <I> if we use italics. That's the bug.
The change in behaviour of getSelectedElement() does appear to be a regression of 8d4336d56112 Boris Zbarsky — Bug 1432944 part 4. Work with Element, not nsIDOMElement, inside HTMLEditor::GetSelectedElement. r=m_kato 84d651460f98 Boris Zbarsky — Bug 1432944 part 3. Take an early return from HTMLEditor::GetSelectedElement when we can. r=m_kato 15f093275a0d Boris Zbarsky — Bug 1432944 part 2. Make nsIHTMLEditor.getSelectedElement return nsISupports. r=m_kato 93c1d149d757 Boris Zbarsky — Bug 1432944 part 1. Stop returning NS_EDITOR_ELEMENT_NOT_FOUND from nsIHTMLEditor::GetSelectedElement. r=m_kato as per comment #11 so maybe Boris or Makoto-san have an insight here. The undesired double-click action is a little annoying and it's mostly my fault that this didn't get reported earlier :-(
Flags: needinfo?(m_kato)
Flags: needinfo?(bzbarsky)
Hello, I don't know if it was there on the 1st 60 release...but above all if so (but even if on the latest version only...) in my opinion a fix about it is certainly a priority. Honestly, just if (example) I write two words on a line, the second word is bold and I just want to double click for copy/paste that word, the Advanced Property Editor pops up... now, I have a table full of bold words on rows...I can't double click without the famous "Advanced Property Editor" running :-D nooooooo...that's a mess...please thank you very much
(In reply to 7stars from comment #22) > Hello, I don't know if it was there on the 1st 60 release...but above all if > so (but even if on the latest version only...) in my opinion a fix about it > is certainly a priority. > Honestly, just if (example) I write two words on a line, the second word is > bold and I just want to double click for copy/paste that word, the Advanced > Property Editor pops up... > > now, I have a table full of bold words on rows...I can't double click > without the famous "Advanced Property Editor" running :-D nooooooo...that's > a mess...please > > thank you very much sorry, forgot...at most, it could be as option (only) into the settings or that can be opened by a button on a toolbar, but nothing more than that.
I got it. https://hg.mozilla.org/mozilla-central/rev/93c1d149d757 drops necessary if block since selectedElement may have different element here.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Component: Message Compose Window → Editor
Flags: needinfo?(masayuki)
Flags: needinfo?(m_kato)
Flags: needinfo?(bzbarsky)
OS: Unspecified → All
Product: Thunderbird → Core
Hardware: Unspecified → All
Version: 60 → 60 Branch
Hmm, there could be another regression. Even after I fix the regression, the result is still odd when the selected word is at end of a block.
Thank you for picking this up, Masayuki-san!
(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #27) > Thank you for picking this up, Masayuki-san! No problem. We should've had unit test for each editor API... (I've already added for some of them, but I don't know expected behavior of some of them... So, creating unit test by initial developer is really important, of course, not your fault.)
Hmm, okay, I understand what's the API... Despite of its name, it does NOT return element node even if a element is selected from end of previous text node and/or to start of next text node. The reason is, it's designed for retrieving target element node to modify with UI like dialog. So, it's intended to select replaced elements like <img>. Only when user click such element, Gecko sets selection container to its parent element and startOffset to its index and endOffset is its index + 1. On the other hand, when user selects with dragging mouse or double click, selection is set at least start point is into a text node. Therefore, mail composer does not expect that double clicking a word won't cause itself opening "Advanced Property Dialog". Note that even before the fix of bug 1432944, the API has a lot of bugs in edge cases. I'll fix all of them tomorrow since such odd edge case behavior makes other developers confused since it may be unclear which behavior is expected when comparing contradictory results.
This takes back old behavior, 59 or earlier version. |selectedElement| may be set to an element which is not what the caller is looking for. Therefore, if the first element node in the selected range is not what the caller is looking for, it should return nullptr. This regression is caused by this changeset: https://hg.mozilla.org/mozilla-central/rev/93c1d149d757 (bug 1432944) Additionally, this patch modifies the automated test for conforming to original idea of the API. This API must not be intended to retrieve usual inline elements like <b>, <i>, etc.
This is also regression of bug 1432944, but hidden by the optimization of bug 1482019. In bug 1482019, we removing the code clearing |found| flag when the loop meets second element in selection range. https://searchfox.org/mozilla-central/diff/0dd29e18302c5e6b07b88aac7164889d752acda7/editor/libeditor/HTMLEditor.cpp#2751-2753 because the flag won't be referred. However, before the fix of bug 1432944, it's referred and the cleared flag makes the method return nullptr. Therefore, this patch makes it return nullptr when 2 or more elements are in selected range.
HTMLEditor::GetElementOrParentByTagNameInternal() may return <a> element which does not match for nsGkAtoms::href or nsGkAtoms::anchor (in the former case, it should return only an <a> element which has "href" attribute and its value is not empty. in the latter case, it should return only <a> element which has "name" attribute and its value is not empty). This patch makes it won't check found element's name when nsGkAtoms::href or nsGkAtoms::anchor is specified.
HTMLEditor::GetSelectedElement() should use RangeBoundary to reduce auto variables which are in large scope. So, first optimization block can be optimized with RangeBoundary. Then, the second block, for handling nsGkAtoms::href, does not need to retrieve AnchorRef() nor FocusRef() since this is exactly same as StartRef() and EndRef() of the first range when there is only one selected range. So, the last |if| block in this block is not necessary since this has already been handled by the first block.
HTMLEditor::GetSelectedElement() uses |while| and calls nsIContentIterator::Next() at the last line. Therefore, it cannot use early-continue style. Because nsIContentIterator::Next() is always called, it should be rewrite with |for|.
The |for| loop in HTMLEditor::GetSelectedElement() does not allow to be after an element node because if another element is found, returns nullptr, and if another non-element node is found, lastElementInRange which is result of the method is cleared. So, we checking lastElementInRange at first of the for loop (i.e., immediately after calling nsIContentIterator::Next()), we can get rid of use ugly bool flag.
This fixes odd case of this API. GetSelectedElement() ignores non-element nodes before an element node. This must be intended to allow Selection to start from in an element node. However, current code allows to select starting from previous text node. This patch changes this behavior to stop looking for element node if non-element node appears before an element node.
Summary of the patches: 1 and 2: Fixing regression of bug 1432944 3: Fixing long standing bug. 4, 5 and 6: Refactoring. 7: Fixing odd behavior (risky). ESR 60 does not need those fixes since Firefox does not use this API. Instead, comm-central for ESR 60 should take minimized patches which includes only 1 and 2 or 1, 2 and 3. I'll write the patche after review. Then, I have a question. Does comm-central for beta 64 needs this fix? (I don't know the release schedule of Thunderbird, but I guess, necessary for Beta users?)
Flags: needinfo?(jorgk)
Thanks for getting this fixed. Wow, such a lot of work went into it. As for your question: This bug has been around for some time now, so I think it's OK to fix it in TB 65 and TB 60 ESR. I'll just take parts 1-3 for the latter. Thanks again!
Flags: needinfo?(jorgk)
Priority: -- → P2
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/eb94ff3abeca part 1: HTMLEditor::GetSelectedElement() should return nullptr when first found element is not what the caller is looking for r=m_kato https://hg.mozilla.org/integration/autoland/rev/d0cbbc6d2371 part 2: HTMLEditor::GetSelectedElement() should return nullptr if 2 or more elements are in selected range r=m_kato https://hg.mozilla.org/integration/autoland/rev/a7c1286bce7f part 3: HTMLEditor::GetElementOrParentByTagNameInternal() shouldn't return <a> element when it's looking for nsGkAtoms::href or nsGkAtoms::anchor but found <a> element does not match with <a href="..."> or <a name="..."> r=m_kato https://hg.mozilla.org/integration/autoland/rev/7572f410e0c9 part 4: Clean up first-half of HTMLEditor::GetSelectedElement() r=m_kato https://hg.mozilla.org/integration/autoland/rev/a537e68e8002 part 5: Rewrite the loop in HTMLEditor::GetSelectedElement() with |for| r=m_kato https://hg.mozilla.org/integration/autoland/rev/7a45d67850c4 part 6: Get rid of |foundElmentInRange| r=m_kato https://hg.mozilla.org/integration/autoland/rev/cb6ee8469bfb part 7: HTMLEditor::GetSelectedElement() shouldn't return element node when Selection starts before the element node r=m_kato
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #39) > ESR 60 does not need those fixes since Firefox does not use this API. > Instead, comm-central for ESR 60 should take minimized patches which > includes only 1 and 2 or 1, 2 and 3. I'll write the patches after review. I'm looking forward to those ;-)
Bug 1501663 - Make HTMLEditor::GetSelectedElement() return nullptr when it finds different element or 2 or more elements in selected range This back-ports: - https://hg.mozilla.org/integration/autoland/diff/eb94ff3abeca/editor/libeditor/HTMLEditor.cpp - https://hg.mozilla.org/integration/autoland/diff/d0cbbc6d2371/editor/libeditor/HTMLEditor.cpp - https://hg.mozilla.org/integration/autoland/diff/a7c1286bce7f/editor/libeditor/HTMLEditor.cpp - and the automated tests This makes: - HTMLEditor::GetSelectedElement() return nullptr without error when it finds different element from what the caller is looking for. - HTMLEditor::GetSelectedElement() return nullptr without error when it finds second element node in the selected range. - HTMLEditor::GetElementOrParentByTagName() return only <a> element whose href attribute value is not empty when the caller is looking for "href". - HTMLEditor::GetElementOrParentByTagName() return only <a> element whose name attribute value is not empty when the caller is looking for "anchor" or "namedanchor".
Attachment #9022826 - Flags: review?(m_kato)
Attachment #9022826 - Flags: review?(m_kato) → review+
jorgk: take attachment 9022826 [details] [diff] [review]. I wrote the patch with esr60 for Firefox. So, it might not be available with esr60 for comm-central. Let me know if so.
Flags: needinfo?(jorgk)
Thanks a lot, I will build a version with your patch included and post the changeset here.
Flags: needinfo?(jorgk)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: