674770 - contenteditable breaks middle-click to open links when middlemouse.paste=true

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Matt Brubeck (:mbrubeck)]

Attachment: test case

Steps to reproduce:
1. Open a page with both a contenteditable element and a link.
2. Middle-click the link.

Expected results: Link opens in a new tab.
Actual results: Nothing happens.

I first noticed this on Planet Mozilla today, where the inclusion of http://kazhack.org/?post/2011/07/27/Editor-Progress%3A-make-P%2C-not-BR! caused middle-click to break on the whole page.  I can reproduce the problem in versions of Firefox 5 through 8 on Ubuntu 11.04, using the attached minimal test case.

Comment 1

[Alice0775 White]

Confirmed.WORKAROUND: middlemouse.paste to false.

I can also reproduce on Windows build if I set middlemouse.paste to true,

Regression window:
Works:
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9a6pre) Gecko/20070627 Minefield/3.0a6pre
Fails:
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9a6pre) Gecko/20070628 Minefield/3.0a6pre
Pushlog:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-06-27+00%3A00%3A00&maxdate=2007-06-28+03%3A00%3A00&cvsroot=%2Fcvsroot

Suspected:
Fix for bug 237964 (Allow editable areas in browser (contentEditable)). r/sr=sicking.

Comment 2

[(no longer active)]

Attachment: Patch (v1)

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/83b6f03bb285

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

And backed out

http://hg.mozilla.org/mozilla-central/rev/0220740e0bea

Comment 6

[Alexander]

When will this bug get fixed? It still affects v7.

Comment 7

[(no longer active)]

The patch for this bug has not landed yet...

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Well, it was landed, it got backed out for test failures though.

Comment 9

[(no longer active)]

Yes.  I've pushed it to try again to see what the test failures were.

Comment 10

[Mozilla RelEng Bot]

Try run for e63afd4b940f is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e63afd4b940f
Results (out of 157 total builds):
    exception: 1
    success: 131
    warnings: 12
    failure: 13
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-e63afd4b940f

Comment 11

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

Ehsan, what's the status of this bug? I'd like to change nsEditorEventListener::MouseClick() for other bugs. If you could, I'd like you to fix this bug first.

Comment 12

[(no longer active)]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #11)
> Ehsan, what's the status of this bug? I'd like to change
> nsEditorEventListener::MouseClick() for other bugs. If you could, I'd like
> you to fix this bug first.

Sorry, totally forgot about it.  Just pushed it to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc4f8b8d18b3

Out of curiosity, what changes are you planning to make to MouseClick?

Comment 13

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

(In reply to Ehsan Akhgari [:ehsan] from comment #12)
> Out of curiosity, what changes are you planning to make to MouseClick?

I think that we should update software keyboard open state when editor is clicked. If so, this bug's change may help better behavior of that.

But I'm still not sure whether the final patch changes the method because I'm still working on to make the new IME APIs which will be used for controlling the software keyboard state. See bug 685395.

Comment 14

[(no longer active)]

Backed out again due to test failure on Windows:

https://tbpl.mozilla.org/php/getParsedLog.php?id=6812430&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/78085156d5da

I'll look at it again, maybe on Friday.

Comment 15

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

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=6e1bb2b92011

I tried to write a patch for bug 685395. The patch also fixes this bug and I don't see the same failure of test_bug597331.html. But I don't understand the actual cause...

Comment 16

[(no longer active)]

We need to take this patch for this bug, by checking IsModifiableNode.  The reftest failure is some sort of a focus issue, which I think is happening because we open a new tab in the test for this bug.  I've fought with that once before, and I don't think it's particularly tricky, I just need access to a Windows machine in order to reproduce and fix it.  :-)

Comment 17

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

Oh, OK. I see.

But your patch looks like preventing middle-paste on non-editable root element which has editable <body> (i.e., <html><body contenteditable></body></html>). Is that your intent? We are assuming that the root element is an editable element in such case. And my patch keeps the behavior.

I think that we shouldn't change IsModifiableNode() behavior anyway. That is used by very many if statement, so, changing it is very risky. I think that we should use nsEditor::IsAccepatbleInputEvent() for click event too, like my patch.
https://hg.mozilla.org/try/rev/f3983ee9bb72

Comment 18

[(no longer active)]

http://tbpl.mozilla.org/?tree=Try&rev=9f0409e91543

Comment 19

[Mozilla RelEng Bot]

Try run for 9f0409e91543 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=9f0409e91543
Results (out of 3 total builds):
    success: 2
    warnings: 1
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-9f0409e91543

Comment 20

[(no longer active)]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #17)
> Oh, OK. I see.
> 
> But your patch looks like preventing middle-paste on non-editable root
> element which has editable <body> (i.e., <html><body
> contenteditable></body></html>). Is that your intent? We are assuming that
> the root element is an editable element in such case. And my patch keeps the
> behavior.
> 
> I think that we shouldn't change IsModifiableNode() behavior anyway. That is
> used by very many if statement, so, changing it is very risky. I think that
> we should use nsEditor::IsAccepatbleInputEvent() for click event too, like
> my patch.
> https://hg.mozilla.org/try/rev/f3983ee9bb72

OK, yes, I think you're right.

So your patch should fix this bug as well, right?

Comment 21

[Mozilla RelEng Bot]

Try run for 31ef02f4ca59 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=31ef02f4ca59
Results (out of 3 total builds):
    success: 2
    warnings: 1
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-31ef02f4ca59

Comment 22

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

(In reply to Ehsan Akhgari [:ehsan] from comment #20)
> So your patch should fix this bug as well, right?

Yes.

Comment 23

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

Hmm, I changed my mind, I'll separate the patch which can fix only this bug. We should fix this bug first.

Comment 24

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

Ehsan, would you post the latest your tests for this bug? I'll add the tests in my mq.

Comment 25

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

Hmm, I meet the same test failure without your new tests. I guess that there is another bug for the new failure. I'm looking for it...

Comment 26

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

Attachment: Result of test_bug597331.html

Comment 27

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

Attachment: Reference of test_bug597331.html

Comment 28

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

Attachment: Result of test_bug600570.html

Comment 29

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

Attachment: Reference of test_bug600570.html

Comment 30

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

The cause of the new failures is that the textareas don't have "focused" look-and-feel at their reference. I have no idea for the reason. Do you have any idea, Ehsan?

Comment 31

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

I see the cause of the new failures, I'll post all patches soon.

Comment 32

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

Attachment: Patch part.1 Shouldn't accept click event if the event target isn't in focused editor

This checks whether the mouse event target is in focused editor or not.

Note that even if focus is moved to another element by focus event handler or mouse event handler, the editor still has focus when the IsAcceptableInputEvent() is called because focus will be actually moved by delayed focus event.

I'm not sure whether this is expected behavior or not. But at least, editor should not accept the mouse event for editing when the editor doesn't have focus.

Comment 33

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

Attachment: Patch part.2 Fix new test_bug597331.html failure and test_bug600570.html failur on Windows

I think that the new failures are caused by delayed focus events. Therefore, the reference screenshots are captured before the textarea style is changed to :focus style. I'm not sure why my patch (and also your patch) causes changing the behavior.

Comment 34

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

Attachment: Patch part.2 -w

Comment 35

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

Attachment: Patch part.3 Add tests for middle-clicking on anchor element when the page has contenteditable element

This is your test (r=roc) but I changed the file name for next patch.

Comment 36

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

Attachment: Patch part.4 Add new tests for pasting by middle click

For avoiding some test failures, this patch uses executeSoon a lot.

1. If focus() is called by script which is kicked by another DOM event, focus event may be delayed.
2. Pasting sometimes fails by synthesized mouse event, I'm not sure the reason. By delayed paste event?

And we should file a bug after this bug.

Middle click event is prevented and stopped by editor's event handler *before* web application handles it.

See XXX comment in nsEditorEventListener in the part.1 patch.

Comment 37

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

And on Mac, middle click paste doesn't work. I think that we should use global clipboard rather than primary selection on Mac.

Comment 38

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

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #37)
> And on Mac, middle click paste doesn't work. I think that we should use
> global clipboard rather than primary selection on Mac.

This is bug 392159.

Comment 39

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

Ehsan:

ping.

Comment 40

[(no longer active)]

Comment on attachment 570147 [details] [diff] [review]
Patch part.1 Shouldn't accept click event if the event target isn't in focused editor

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

Looks good, thanks!

(Sorry for my very long delay in getting back to you!)

Comment 41

[(no longer active)]

Comment on attachment 570149 [details] [diff] [review]
Patch part.2 Fix new test_bug597331.html failure and test_bug600570.html failur on Windows

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

I have no idea why the original failure happens, but they're indeed the side effect of something which is not important in this bug.  If these executeSoon calls let us proceed with this, then so be it!  :-)

Comment 42

[(no longer active)]

Comment on attachment 570149 [details] [diff] [review]
Patch part.2 Fix new test_bug597331.html failure and test_bug600570.html failur on Windows

(BTW, please remove the reference and result data URIs from the test log message before landing.  While they're useful for debugging, they will clutter the successful test run logs needlessly.  Thanks!)

Comment 43

[(no longer active)]

Comment on attachment 570155 [details] [diff] [review]
Patch part.4 Add new tests for pasting by middle click

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

::: editor/libeditor/html/tests/test_bug674770-2.html
@@ +47,5 @@
> +// NOTE: tests need to check the result *after* the content is actually
> +//       modified.  Sometimes, the modification is delayed. Therefore, there
> +//       are a lot of functions and SimpleTest.executeSoon()s.
> +
> +addLoadEvent(function() {

I think you need to use waitForFocus when you call synthesizeKey.  Otherwise the test might fail intermittently on Linux.

Comment 44

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/dad793c1b143
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a9cc49f9cf1
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ab2c7d4c91d
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5782f6da550

try:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=1c08af775d64

> (BTW, please remove the reference and result data URIs from the test log message before landing.  While they're useful for debugging, they will clutter the successful test run logs needlessly.  Thanks!)

The data URIs are undefined if the test succeed. Anyway, I changed the tests again. The landed patch outputs the data URIs only when it fails.

If we can still see the failure on inbound, I think we should disable the tests for now. I've looked similar failure on other bugs, so, I should fix the actual cause of the failure in another bug.

Comment 45

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/dad793c1b143
https://hg.mozilla.org/mozilla-central/rev/1a9cc49f9cf1
https://hg.mozilla.org/mozilla-central/rev/1ab2c7d4c91d
https://hg.mozilla.org/mozilla-central/rev/b5782f6da550

Comment 46

[James Pearson]

The fixes in patch https://hg.mozilla.org/mozilla-central/rev/dad793c1b143 also fix problems we have with version 10 ESR on Linux using middle-button paste with OWA (Outlook Web App - Exchange 2010 Webmail)

The problem is that the text pasted via a middle-button paste is then not editable - i.e. if I middle-button paste some text in to the body of a new message, I can then not move the cursor in to that text - either using the mouse or the keyboard cursor keys i.e. the pasted text is displayed - but Firefox is unaware of it ...

I get a similar issue middle-button pasting in to the OWA search boxes

I have rebuilt 10.0.6 ESR on Linux with the above patch and it does indeed fix the problem

As middle-button pasting is quite common in the Linux world, I would like to have this fix incorporated in to 10 ESR

Comment 47

[James Pearson]

Is this fix going to be added to the next ESR (10.0.7) release?

Comment 48

[(no longer active)]

Masayuki, would you mind preparing a rolled up version of the patch that applies cleanly on the ESR branch?  We're going to try to take this for the next ESR release.  Thanks!

Comment 49

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

The landed patches can apply to esr10 without any changes.

But I'll test them tomorrow. However, I'm not sure why this bug causes the behavior described in comment 46...

Comment 50

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

Attachment: part.1 Shouldn't accept click event if the event target isn't in focused editor

Comment 51

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

Attachment: part.2 Fix new test_bug597331.html failure and test_bug600570.html failur on Windows

Comment 52

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

Attachment: part.3 Add tests for middle-clicking on anchor element when the page has contenteditable element

Comment 53

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

Attachment: part.4 Add new tests for pasting by middle click

These patches works fine for me.

Test builds are:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-6712854b20d3/

Unfortunately, failed to build on some platforms on tryserver. I guess that tryserver is using buildconfig for mozilla-central.

Comment 54

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

The result of tests:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=6712854b20d3

I tested with some testcases and on Outlook.

1. login to Outlook.
2. middle click in "Search email" field.
3. try modifying the pasted text.

Then, I don't see any problem in it. But I couldn't set focus (and also paste by middle click) to the editor of mail body at new mail composing. I'm not sure whether it's valuable to land these patches to ESR for Outlook.

Comment 55

[James Pearson]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #54)
> 
> Then, I don't see any problem in it. But I couldn't set focus (and also
> paste by middle click) to the editor of mail body at new mail composing. I'm
> not sure whether it's valuable to land these patches to ESR for Outlook.

Is the above before or after applying the patch?

Which version of OWA/Exchange are you using? Are you using the 'full' or 'light' interface to OWA (the problem isn't there with the 'light' version)

We definitely have a problem using middle-button paste with OWA 2012 and ESR 10.0.6 - but when I rebuild ESR 10.0.6 with this patch, the problems I described in comment #46 are fixed

Comment 56

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

I tested on outlook.com only with the tryserver build. Is the OWA you mentioned different from it?

Anyway, I'll be offline since now.

Ehsan, I confirmed the patches work fine for me. If you think that they should be landed on ESR, please request the approvals yourself. Thanks.

Comment 57

[James Pearson]

No, we're using OWA (Outlook Web App) - which is the webmail client to Exchange 2010 - which is not outlook.com/hotmail webmail ...

Comment 58

[James Pearson]

As there patches work fine with ESR 10 - can this fix be applied to the next ESR release?

Comment 59

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

Sorry for the delay, I'm back from my short vacation.

I'll request the approval.

Comment 60

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

Comment on attachment 648228 [details] [diff] [review]
part.1 Shouldn't accept click event if the event target isn't in focused editor

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:

See comment 46 and comment 55. I've never tested (confirmed) on it since I don't have such environment.

User impact if declined:

Uses cannot use middle click paste on some web applications. The middle click paste is common operation for Linux users. So, they must feel bad.

Fix Landed on Version:

Fx11.

Risk to taking this patch (and alternatives if risky): 

This patch added only the check if the event is fired on an editable element in the HTML editor. And I wrote the patch while mozilla-central was for Fx10. So, I tested with Fx10 when I worked on this. And following patches tests middle mouse event handling on editor.

String or UUID changes made by this patch: 

None.

Comment 61

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

Comment on attachment 648229 [details] [diff] [review]
part.2 Fix new test_bug597331.html failure and test_bug600570.html failur on Windows

[Approval Request Comment]
This is necessary for keeping tinderbox green if we pick part.1 up.

Comment 62

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

Comment on attachment 648230 [details] [diff] [review]
part.3 Add tests for middle-clicking on anchor element when the page has contenteditable element

[Approval Request Comment]
new tests for part.1.

Comment 63

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

Comment on attachment 648231 [details] [diff] [review]
part.4 Add new tests for pasting by middle click

[Approval Request Comment]
this is also additional new tests for part.1.

Comment 64

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 648228 [details] [diff] [review]
part.1 Shouldn't accept click event if the event target isn't in focused editor

Let's land this on ESR and then put out a call for some testing prior to release to make sure this is working as expected.

Comment 65

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

https://hg.mozilla.org/releases/mozilla-esr10/rev/2a45f89808a5
https://hg.mozilla.org/releases/mozilla-esr10/rev/c8c9093bbb51
https://hg.mozilla.org/releases/mozilla-esr10/rev/012144090a5d
https://hg.mozilla.org/releases/mozilla-esr10/rev/d9acbde0420b

Comment 66

[James Pearson]

(In reply to Lukas Blakk [:lsblakk] from comment #64)
> 
> Let's land this on ESR and then put out a call for some testing prior to
> release to make sure this is working as expected.

My problem is now fixed in the latest (10th August) nightly ESR build - thanks

Comment 67

[yiding.jia]

I've noticed something similar to this problem again in Firefox 71.0. A middle clicking a link in a contenteditable element generates a paste event rather than opening the link in a new tab.

Simple repro:

<!DOCTYPE html>
<html>
  <body>
    <div contenteditable="true">
      <a href="http://example.com" style="cursor: pointer;">
        <span>Link</span>
      </a>
    </div>
  </body>
</html>

Comment 68

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Comment 69

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

(In reply to yiding.jia from comment #67)

I've noticed something similar to this problem again in Firefox 71.0. A middle clicking a link in a contenteditable element generates a paste event rather than opening the link in a new tab.

This bug was fixed 7 years ago so that even if you see same symptom, it may be different bug. Please file a new bug.