910729 - Pressing the Apostrophe in a content editable opens up the quick find dialog

Status: VERIFIED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[filip_wieladek]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Ubuntu Chromium/28.0.1500.71 Chrome/28.0.1500.71 Safari/537.36

Steps to reproduce:

Use the following html:

<a  href="http://the.earth.li/~sgtatham/putty/latest/x86/putty.exe">link here </a>
<div contenteditable="true">First Click here and type ' and then click the link download the file and try again</div>

(the test case is http://jsfiddle.net/H4wNh/6/)

1) Press on the link to download the file.
2) Give focus to the content editable
3) Type ' (Apostrophe) 


Actual results:

The quick find opens. It does not open before the link is clicked


Expected results:

The apostrophe should be typed in.

Comment 1

[filip_wieladek]

I am changing the importance to major as this is a major usability problem for us. Once a link with a download is clicked, our rich text editor is not useable in firefox. Firefox is the only browser that behaves this way

Comment 2

[Alice0775 White]

This bug exists since Firefox3.0

Comment 3

[Tom S [:evilpie]]

Attachment: fix

Was working on this anyway, so I just fixed it.

Comment 4

[Mike de Boer [:mikedeboer]]

Comment on attachment 806210 [details] [diff] [review]
fix

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

::: toolkit/content/widgets/findbar.xml
@@ +721,5 @@
>                  elt instanceof HTMLEmbedElement)
>                return false;
> +
> +            if (elt.isContentEditable)
> +              return false;

What's the rationale behind this addition? (Or: how on earth did you find this solution? ;) )

Comment 5

[Tom S [:evilpie]]

So what this and the surrounding code does is, that it checks if the users is in some kind of input field. Because in that case the user actually wants to type the apostrophe, instead of opening quickfind. My new check basically extends this to an other case where somebody is actually entering text, namely editing a contenteditable element. I however just realized that there might be a problem that I have to look at. I think what could happen is that somebody focuses a node instead a node, which is contenteditable. In this case they still probably want to enter text, but I assume that the "isContentEditable" flag would be false.

Comment 6

[filip_wieladek]

It is important to note that the behaviour is correct initially and breaks only after having pressed on the link. So there might be a state issue somewhere additionally.

Comment 7

[Mike de Boer [:mikedeboer]]

Tom, any update on this?

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/32025f35568f

Comment 9

[Tom S [:evilpie]]

Oh I am sorry, this had the wrong bug number. The right would be bug 917406.

Comment 10

[Mike de Boer [:mikedeboer]]

Tom, the right thing to do (usually) is to back out the changeset with the wrong commit message and re-commit it with the corrected message. Mistakes like this will stay in the tree for eternity...

Comment 11

[Mike de Boer [:mikedeboer]]

Also, I'm still waiting for your feedback on your original concern (mentioned in comment 5): 
> I think what could happen is that somebody focuses a node instead a node, which is contenteditable. In this case they still probably want to enter text, but I assume that the "isContentEditable" flag would be false.

Comment 12

[Mike de Boer [:mikedeboer]]

Trying to use quote again:

> I think what could happen is that somebody focuses a node instead a node, which is contenteditable. In this case they still probably want to enter text, but I assume that the "isContentEditable" flag would be false.

As it looks horribly wrong in comment 11.

Comment 13

[Tom S [:evilpie]]

Okay sorry, I will try to look at this now.

Comment 14

[filip_wieladek]

THis is a major usability issue in our web application. Is there any hope that this bug is fixed? contenteditable=true is useless for us until the bug is fixed.

Would there be any work around for this issue?

Comment 15

[filip_wieladek]

Changing the severity to blocker.

Not only does the quick find toolbar appear, but other editing functionality is gone:

 * Paste (CTRL + V) does not work anymore
 * Cursor is gone
 * Quick find toolbar is opened

Our product is currently unusable with firefox due to this issue. There does also not seem to be any workaround for this issue. Toggling contenteditable from true to false does not fix the issue. 

The only workaround known to us is to refresh the page.

Comment 16

[filip_wieladek]

The product and component are most likely incorrect in this case as it seems to be a general issue with contenteditable=true and not just with the quick find

Comment 17

[filip_wieladek]

@philip.chee could you please file this against the right Product/Component?

This does not only affect the quick find bar but all editing actions (including Copy / Paste)


Could you also explain as to how this severity is only normal? Do you consider losing all modern editing functionality as "normal"?

Comment 18

[Philip Chee]

I think the best thing for you to do is to hop on to IRC:  irc://moznet/developers
and talk directly to Tom Schuster [:evilpie] or Mike de Boer [:mikedeboer]

Comment 19

[Benjamin Kerensa [:bkerensa]]

Has this been happening since 27 and how many users of your application are impacted by this?

Comment 20

[filip_wieladek]

@bkerensa, yes, this happens even on the nightly build of firefox. See the description which comes with a test case (http://jsfiddle.net/H4wNh/6/)

We develop a tracking and planning tool and virtually every firefox user is affected by this as we are:

1) using divs with contenteditable=true (for comments + description)
2) providing attachment links which download files on the same page.

Once this issue comes up, the user is unable to paste into the div and the ' is causing the quick find toolbar to appear. The only work around for the user is to refresh the page.

As of today, we have not found any workaround which we could apply code wise to make sure firefox does not enter into the broken state

Comment 21

[Mike de Boer [:mikedeboer]]

Tom, do you have cycles available to look at this?

Comment 22

[Tom S [:evilpie]]

Comment on attachment 806210 [details] [diff] [review]
fix

So I just checked "isContentEditable" is true for every child of a contenteditable node. So this fix is correct. I also verified the fix with the last testcase. (Thanks a lot for that!)

Comment 23

[filip_wieladek]

@evilpie Are you sure this is the right fix? As I mentioned earlier, when firefox gets into this state, other things become disabled. 

E.g. CTRL + V does not result in a paste anymore. I think the fix should not really go into the toolbar code as it works in the beginning but gets broken after the link is pressed. This should most likely be a firefox common bug, then just the toolbar.

Why is the toolbar not getting this event when everything is fine? E.g. before the link is pressed

Comment 24

[Alice0775 White]

Filed Bug 991537

Comment 25

[Tom S [:evilpie]]

We should land this patch and find somebody else to look at the platform issue. I can only assume that fixing that is going to take longer.

Comment 26

[Mike de Boer [:mikedeboer]]

Comment on attachment 806210 [details] [diff] [review]
fix

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

::: toolkit/content/widgets/findbar.xml
@@ +721,5 @@
>                  elt instanceof HTMLEmbedElement)
>                return false;
> +
> +            if (elt.isContentEditable)
> +              return false;

Perhaps it's OK to just add this check to the if-clause on line 715? That one is also about text-input-type elements, just like this one.

Comment 27

[Tom S [:evilpie]]

(In reply to Mike de Boer [:mikedeboer] from comment #26)
> Comment on attachment 806210 [details] [diff] [review]
> fix
> 
> Review of attachment 806210 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/content/widgets/findbar.xml
> @@ +721,5 @@
> >                  elt instanceof HTMLEmbedElement)
> >                return false;
> > +
> > +            if (elt.isContentEditable)
> > +              return false;
> 
> Perhaps it's OK to just add this check to the if-clause on line 715? That
> one is also about text-input-type elements, just like this one.
I don't think that works, we need the element reference.

Comment 28

[Tom S [:evilpie]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/201d593499cd

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/201d593499cd

Comment 30

[Marco Mucci [:MarcoM]]

Hi Anthony, can you review to determine if further QA verification is required.  Thanks.

Comment 31

[u279076]

Tagging this [qa+] to verify this is now fixed and to confirm similar behaviours continue to work. Florin, please assign this to someone on your team for verification.

Tom, does this have automated test coverage? If not, does it need automated tests?

Comment 32

[Catalin Varga [QA][:VarCat]]

I verified the bug using the following environment:
FF 31
Build Id: 20140409030203
OS: Win 7 x64, Mac Os X 10.9.2, Ubuntu 13.04 x64

The Quickfind bar no longer opens when typing ' in the "contenteditable" field after pressing the    download link.
The other edit issues( CTLR+V not working after clicking the download link) are still there and Alice logged a different bug 991537 for them.
I also noticed a new issue that is reproducible on older builds, eg typing  ' when the focus is set outside of the contenteditable field is not opening the quickfind bar. I've logged bug 994063 for this issue.

Comment 33

[Tom S [:evilpie]]

Thanks for looking at this. We sadly don't have tests for this. I am not interesting in writing such a test, because it's 10x more complicated than this fix.

Comment 34

[u279076]

(In reply to Tom Schuster [:evilpie] from comment #33)
> Thanks for looking at this. We sadly don't have tests for this. I am not
> interesting in writing such a test, because it's 10x more complicated than
> this fix.

Is this something you want tests for? In other words, if I had someone in QA willing to work on the tests would you be willing to mentor them?

Comment 35

[u279076]

Tom, do you want to handle bug 991537 and bug 994063 in *this* bug, or should we defer that work to a future iteration? In my opinion those issues are likely out of scope for this bug and should be addressed following this iteration. What do you think?