Closed Bug 674770 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: DOM: Editor, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla11
Tracking Status
firefox5 --- affected
firefox6 --- affected
firefox7 --- affected
firefox8 --- affected
firefox-esr10 15+ fixed

People

(Reporter: mbrubeck, Assigned: masayuki)

References

Details

(Keywords: reproducible, testcase, Whiteboard: [inbound][qa-])

Attachments

(5 files, 10 obsolete files)

238 bytes, text/html
Details
6.87 KB, patch
Details | Diff | Splinter Review
6.00 KB, patch
Details | Diff | Splinter Review
3.62 KB, patch
Details | Diff | Splinter Review
13.37 KB, patch
Details | Diff | Splinter Review
Attached file 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.
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.
OS: Linux → All
OS: All → Linux
Summary: contenteditable breaks middle-click to open links → contenteditable breaks middle-click to open links when middlemouse.paste=true
Attached patch Patch (v1) (obsolete) — Splinter Review
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #549232 - Flags: review?(roc)
Status: ASSIGNED → NEW
OS: Linux → All
http://hg.mozilla.org/mozilla-central/rev/83b6f03bb285
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
And backed out

http://hg.mozilla.org/mozilla-central/rev/0220740e0bea
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
When will this bug get fixed? It still affects v7.
The patch for this bug has not landed yet...
Well, it was landed, it got backed out for test failures though.
Yes.  I've pushed it to try again to see what the test failures were.
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/eakhgari@mozilla.com-e63afd4b940f
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.
(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?
Status: REOPENED → ASSIGNED
Target Milestone: --- → mozilla10
(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.
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...
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.  :-)
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
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/eakhgari@mozilla.com-9f0409e91543
(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?
Depends on: 685395
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/eakhgari@mozilla.com-31ef02f4ca59
(In reply to Ehsan Akhgari [:ehsan] from comment #20)
> So your patch should fix this bug as well, right?

Yes.
Hmm, I changed my mind, I'll separate the patch which can fix only this bug. We should fix this bug first.
Assignee: ehsan → masayuki
Ehsan, would you post the latest your tests for this bug? I'll add the tests in my mq.
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...
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?
I see the cause of the new failures, I'll post all patches soon.
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.
Attachment #569664 - Attachment is obsolete: true
Attachment #569665 - Attachment is obsolete: true
Attachment #569666 - Attachment is obsolete: true
Attachment #569667 - Attachment is obsolete: true
Attachment #570147 - Flags: review?(ehsan)
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.
Attachment #570149 - Flags: review?(ehsan)
This is your test (r=roc) but I changed the file name for next patch.
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.
Attachment #570155 - Flags: review?(ehsan)
And on Mac, middle click paste doesn't work. I think that we should use global clipboard rather than primary selection on Mac.
Blocks: 685395
No longer depends on: 685395
(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.
Blocks: 392159
Ehsan:

ping.
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!)
Attachment #570147 - Flags: review?(ehsan) → review+
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!  :-)
Attachment #570149 - Flags: review?(ehsan) → review+
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 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.
Attachment #570155 - Flags: review?(ehsan) → review+
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.
Whiteboard: [inbound]
Target Milestone: mozilla10 → mozilla11
Depends on: 732792
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
Is this fix going to be added to the next ESR (10.0.7) release?
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!
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...
These patches works fine for me.

Test builds are:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/masayuki@d-toybox.com-6712854b20d3/

Unfortunately, failed to build on some platforms on tryserver. I guess that tryserver is using buildconfig for mozilla-central.
Attachment #570155 - Attachment is obsolete: true
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.
(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
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.
No, we're using OWA (Outlook Web App) - which is the webmail client to Exchange 2010 - which is not outlook.com/hotmail webmail ...
As there patches work fine with ESR 10 - can this fix be applied to the next ESR release?
Sorry for the delay, I'm back from my short vacation.

I'll request the approval.
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.
Attachment #648228 - Flags: approval-mozilla-esr10?
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.
Attachment #648229 - Flags: approval-mozilla-esr10?
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.
Attachment #648230 - Flags: approval-mozilla-esr10?
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.
Attachment #648231 - Flags: approval-mozilla-esr10?
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.
Attachment #648228 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Attachment #648229 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Attachment #648230 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Attachment #648231 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
(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
Whiteboard: [inbound] → [inbound][qa-]

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>

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

Keywords: regression

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

Keywords: regression
You need to log in before you can comment on or make changes to this bug.