1426869 - td.contenteditable=true focus breaks text selection

Status: RESOLVED FIXED

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

Description

[Duncan]

Attachment: contenteditable.html

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.84 Safari/537.36

Steps to reproduce:

Focused table cell with contenteditable="true". Then I tried to use the mouse to select text in other table cells.





Actual results:

It appeared like the browser was selecting table cells themselves as opposed to text. Could not select text.

I produced with:
Firefox Developer Edition 58.0b12 (Windows 10 64-bit)
Firefox ESR Edition 52.5.2 (Windows 10 32-bit)

Firefox ESR is our target.



Expected results:

Should have been able to select text as I could before focusing the contenteditable="true" table cell.

Comment 1

[Duncan]

Note: Once the <td contenteditable="true"></td> is focused. The browser no longer selects text properly, even if I do the following:
$('[contenteditable]').remove()

It be broken!

Comment 2

[Alice0775 White]

This is a regression since Firefox47.

Comment 3

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ee4201100b15c95ca409faee977f45d39f7e52a1&tochange=71b0b49c6d47bdb2fec86bb750fad2e855fceb9a

Regressed by: 417d17932ff0	Jet Villegas — Bug 974309: Fixes the IsEditable() logic for table cells. r=ehsan

@Jet Villegas

Your patch causes the regression, can you look into this?

Comment 4

[Jet Villegas (inactive)]

Attachment: Verify that the selection type is for contenteditable before searching for contenteditable table cells.

Please see this diff from the regressing patch:
https://hg.mozilla.org/integration/mozilla-inbound/rev/417d17932ff0#l4.30

Comment 5

[Jet Villegas (inactive)]

Attachment: Drop any cached editable table cell before looking for a new one.

A better way to do this. We never drop an editable table cell once one is found, even when subsequent selections should do so.

Comment 6

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

Comment on attachment 8941097 [details] [diff] [review]
Drop any cached editable table cell before looking for a new one.

Looks good to me. On the other hand, although out of scope of this bug, I don't understand why mCellParent is a member of this class since it's not referred by other methods except CC. So, looks like that it's enough to be a local variable.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3118b4fae13
"td.contenteditable=true focus breaks text selection" [r=masayuki]

Comment 8

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/b3118b4fae13

Comment 9

[Julien Cristau [:jcristau] (back April 22)]

Should we uplift this to beta?

Comment 10

[Jet Villegas (inactive)]

(In reply to Julien Cristau [:jcristau] from comment #9)
> Should we uplift this to beta?

Let's wait to check for fallout. Thx!

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Any fallout? :)

Comment 12

[Jet Villegas (inactive)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #11)
> Any fallout? :)

We haven't sen any. Let's uplift.

Comment 13

[Jet Villegas (inactive)]

Comment on attachment 8941097 [details] [diff] [review]
Drop any cached editable table cell before looking for a new one.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 974309
[User impact if declined]: Editable table cells can prevent correct text selection.
[Is this code covered by automated tests?]: Yes, see tests in bug 974309
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Fix has had time to bake in Nightly. 
[String changes made/needed]: None

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8941097 [details] [diff] [review]
Drop any cached editable table cell before looking for a new one.

Simple fix for a text selection regression. Taking for 59b13.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b04d72c7cb7f

Comment 16

[Cornel Ionce [:noni] [Hubs QA]]

(In reply to Jet Villegas (:jet) from comment #13)
> [Is this code covered by automated tests?]: Yes, see tests in bug 974309
> [Needs manual test from QE? If yes, steps to reproduce]: No

Marking as qe-verify- due to the existing automated coverage.