The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox -esr10

Status

()

Core
Editor
RESOLVED FIXED
6 years ago
6 months ago

People

(Reporter: mbrubeck, Assigned: masayuki)

Tracking

({reproducible, testcase})

Trunk
mozilla11
x86
All
reproducible, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox5 affected, firefox6 affected, firefox7 affected, firefox8 affected, firefox-esr1015+ fixed)

Details

(Whiteboard: [inbound][qa-])

Attachments

(5 attachments, 10 obsolete attachments)

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

Description

6 years ago
Created attachment 548972 [details]
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.
(Reporter)

Updated

6 years ago
status-firefox5: --- → affected
status-firefox6: --- → affected
status-firefox7: --- → affected
status-firefox8: --- → affected

Comment 1

6 years ago
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.
Blocks: 237964
OS: Linux → All
(Reporter)

Updated

6 years ago
OS: All → Linux
Summary: contenteditable breaks middle-click to open links → contenteditable breaks middle-click to open links when middlemouse.paste=true
Created attachment 549232 [details] [diff] [review]
Patch (v1)
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #549232 - Flags: review?(roc)
Attachment #549232 - Flags: review?(roc) → review+

Updated

6 years ago
Duplicate of this bug: 675551
Status: ASSIGNED → NEW
OS: Linux → All
http://hg.mozilla.org/mozilla-central/rev/83b6f03bb285
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
And backed out

http://hg.mozilla.org/mozilla-central/rev/0220740e0bea
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 6

6 years ago
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.

Comment 10

6 years ago
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
(Assignee)

Comment 11

6 years ago
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
(Assignee)

Comment 13

6 years ago
(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.
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.
(Assignee)

Comment 15

6 years ago
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.  :-)
(Assignee)

Comment 17

6 years ago
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
http://tbpl.mozilla.org/?tree=Try&rev=9f0409e91543

Comment 19

6 years ago
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

Comment 21

6 years ago
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
(Assignee)

Comment 22

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

Yes.
(Assignee)

Comment 23

6 years ago
Hmm, I changed my mind, I'll separate the patch which can fix only this bug. We should fix this bug first.
(Assignee)

Updated

6 years ago
Assignee: ehsan → masayuki
(Assignee)

Comment 24

6 years ago
Ehsan, would you post the latest your tests for this bug? I'll add the tests in my mq.
(Assignee)

Comment 25

6 years ago
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...
(Assignee)

Comment 26

6 years ago
Created attachment 569664 [details]
Result of test_bug597331.html
(Assignee)

Comment 27

6 years ago
Created attachment 569665 [details]
Reference of test_bug597331.html
(Assignee)

Comment 28

6 years ago
Created attachment 569666 [details]
Result of test_bug600570.html
(Assignee)

Comment 29

6 years ago
Created attachment 569667 [details]
Reference of test_bug600570.html
(Assignee)

Comment 30

6 years ago
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?
(Assignee)

Comment 31

6 years ago
I see the cause of the new failures, I'll post all patches soon.
(Assignee)

Comment 32

6 years ago
Created attachment 570147 [details] [diff] [review]
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.
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)
(Assignee)

Comment 33

6 years ago
Created attachment 570149 [details] [diff] [review]
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.
Attachment #570149 - Flags: review?(ehsan)
(Assignee)

Comment 34

6 years ago
Created attachment 570151 [details] [diff] [review]
Patch part.2 -w
(Assignee)

Comment 35

6 years ago
Created attachment 570152 [details] [diff] [review]
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.
(Assignee)

Comment 36

6 years ago
Created attachment 570155 [details] [diff] [review]
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.
Attachment #570155 - Flags: review?(ehsan)
(Assignee)

Comment 37

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

Updated

6 years ago
Blocks: 685395
No longer depends on: 685395
(Assignee)

Comment 38

6 years ago
(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
(Assignee)

Comment 39

5 years ago
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+
(Assignee)

Comment 44

5 years ago
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
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
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago5 years ago
Resolution: --- → FIXED
Depends on: 732792

Comment 46

5 years ago
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
tracking-firefox-esr10: --- → ?

Updated

5 years ago
tracking-firefox-esr10: ? → 15+

Comment 47

5 years ago
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!
(Assignee)

Comment 49

5 years ago
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...
(Assignee)

Comment 50

5 years ago
Created attachment 648228 [details] [diff] [review]
part.1 Shouldn't accept click event if the event target isn't in focused editor
Attachment #570147 - Attachment is obsolete: true
(Assignee)

Comment 51

5 years ago
Created attachment 648229 [details] [diff] [review]
part.2 Fix new test_bug597331.html failure and test_bug600570.html failur on Windows
Attachment #549232 - Attachment is obsolete: true
Attachment #570149 - Attachment is obsolete: true
(Assignee)

Comment 52

5 years ago
Created attachment 648230 [details] [diff] [review]
part.3 Add tests for middle-clicking on anchor element when the page has contenteditable element
Attachment #570151 - Attachment is obsolete: true
Attachment #570152 - Attachment is obsolete: true
(Assignee)

Comment 53

5 years ago
Created attachment 648231 [details] [diff] [review]
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/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
(Assignee)

Comment 54

5 years ago
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

5 years ago
(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
(Assignee)

Comment 56

5 years ago
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

5 years ago
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

5 years ago
As there patches work fine with ESR 10 - can this fix be applied to the next ESR release?
(Assignee)

Comment 59

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

I'll request the approval.
(Assignee)

Comment 60

5 years ago
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?
(Assignee)

Comment 61

5 years ago
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?
(Assignee)

Comment 62

5 years ago
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?
(Assignee)

Comment 63

5 years ago
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+
status-firefox-esr10: --- → affected
Attachment #648230 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Attachment #648231 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
(Assignee)

Comment 65

5 years ago
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
status-firefox-esr10: affected → fixed

Comment 66

5 years ago
(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-]
(Assignee)

Updated

6 months ago
Depends on: 972110
You need to log in before you can comment on or make changes to this bug.