41 bytes, text/html
143.68 KB, video/webm
1.27 KB, patch
|Details | Diff | Splinter Review|
16.13 KB, image/png
Created attachment 8704650 [details] testcase 1 - textarea displays cursor on wrong line.html >>> My Info: Win7_64, Nightly 46, 32bit, ID 20160105030211 STR: 1. Open attached "testcase 1" 2. Click at the end of the last line (you'll get "asdf[cursor]") 3. Press Shift+Up to select text "\nasdf" 4. Press (Shift+)Enter to paste "\n" instead of selected text 5. Type "x" Result: After Step 4 cursor is displayed at the end of the 1st line After Step 5 the typed letter ("x") appears in the beginning of the 2nd line Expectations (either A or B): A) After Step 4 cursor should be displayed at the start of the 2nd line B) After Step 5 the typed letter ("x") should appear in the end of the 1st line Regression range: > https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e9769c80ee593c9981cb00079801ae040ba55213&tochange=02f2f4c75007651c63bbc0791d9a58dea88f545f I detected this and bug 1237236 about 4 month ago and thought they were the same bug I will appreciate it if somebody will improve the summaries of this bug and bug 1237236
Not sure why this should block bug 756984. That bug was fixed and landed on Gecko 39. You're saying that this was regressed by bug 756984, right? In that bug we changed the selection behaviour when <br> elements are used. The selection is placed before the brFrame when positioning to the end of the line either by clicking to the right of the end of the line, using the "end" key or moving up using the "right" key from the start of the following line. Your example contains a textarea and there aren't any <br> elements. Bug 1237236 really looks very similar. Here and there you have textareas and you create a selection that includes the line break.
> You're saying that this was regressed by bug 756984 > Your example contains a textarea and there aren't any <br> elements. 1) Yes, I thought it was... 2) There's only anonymous <br> element (see screenshot below). If you're totally sure that 756984 couldn't cause this bug, then I'll probably set keyword "regressionwindow-wanted". > screenshot: https://dl.dropboxusercontent.com/s/jp8bur2z0rnz3y4/bug%201237286%20comments%201.png
In the DOM inspector the text content shows as "A\nasdf", so there really is no <br> in the textarea. I can only be totally sure if I back out the change from bug 756984. Sometimes a regression window doesn't help, since the bug that caused the problem cannot be backed out or sometimes a change exposes another bug. Without looking at it in detail, I don't know. Once again, the other bug 1237236 looks very similar, so I'd start there. By the looks of it the caret position doesn't accurately reflect the selection, so a subsequent insertion goes somewhere unexpected.
> there really is no <br> in the textarea It's not just screenshot, there IS <br> in textarea. You can inspect anonymous nodes by setting pref devtools.inspector.showAllAnonymousContent -> true and then restarting the browser (I only left this comment to fix the wrong statement)
Marking this as dependant bug based on comment 4 (since I agree with everything except <br> thing)
Created attachment 8705175 [details] [diff] [review] Backing out part of bug 756984 to see what difference it makes. Backing out a bit of bug 756984 (https://hg.mozilla.org/mozilla-central/rev/bf414f68291c#l3.19) makes this problem go away. So it is a regression from bug 756984 after all.
Created attachment 8705976 [details] DOM of textarea (take 2) I stand corrected, there is a <br> after the visible text ;-) Looks like the DOM is this: A\nasdf<br> Before bug 756984 landed, going to the end of the line put the selection behind the <br> as indicated with "|": A\nasdf<br>| With bug 756984 landed, we now place the selection before the <br>, and this is desired and not a bug: A\nasdf|<br> As I said in comment #4: ... the bug that caused the problem cannot be backed out [and the] change exposes another bug This is what has happened here: The behaviour of the system changed ever so slightly and exposed another bug. Since the exposed other bug looks very similar to what was reported in bug 1237236, there is hope that both can be fixed together.
Just looking at this again. The testcase that shows the error is this: <textarea style="height:200px;"> A asdf If you use complete HTML, so <textarea style="height:200px;"> A asdf </textarea> the problem doesn't occur.
I have to correct what I said in comment #11. Not working: <textarea style="height:200px;"> A asdf</textarea> Working: <textarea style="height:200px;"> A asdf </textarea> So it's the extra newline that makes the difference.
I debugged this a little bit: At https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/nsPlaintextEditor.cpp#798 Collapse() is called with offset == 2, after inserting a \n a few lines above. That looks correct to me, since the caret should be placed after the A and the \n in the text area. I don't understand why visually it's not placed there. I traced through the two cases described in the preceding comment (comment #12) and can't see a difference in processing, yet the outcome is different. Since you had no trouble fixing the "sister" bug 1237236, maybe you can shed some light on this, too. Oh, another observation: The STR say, place the cursor behind the asdf, press shift <up>, then hit enter. So this is a "backward" selection. If you place the cursor behind the A and then select to the end of the asdf, which should result in the very same selection containing the \nasdf but this time selecting "forward", and then hit enter, there is no problem. <off topic> Frankly, I don't see why I was assigned to this bug without asking me first. It is true that bug 756984 - which was implemented with Ehsan's and Robert's guidance - changed the selection behaviour (we collapse at the end of a text node and not after a br when selecting at the end of a br-terminated line), but that doesn't make me responsible for all other bugs that change uncovered. I'm happy to help though ;-) </off topic>
I can't reproduce this in a Linux64 Nightly. I've just landed bug 1237236 on mozilla-inbound. It's probably worth waiting for those builds to check if it fixes this bug too.
(In reply to Jorg K (GMT+1) from comment #13) > <off topic> > Frankly, I don't see why I was assigned to this bug without asking me first. I totally agree. Assigning bugs to people without asking them first is rude. needinfo? is the mechanism to use in a situation like this (and the same goes for bug 1237236 for that matter).
I've applied your patch from bug 1237236 locally and it doesn't fix this problem as I had hoped. Otherwise I wouldn't have contacted you ;-) I'm surprised you can't reproduce this. At least on Windows it's easily reproducible with attachment 8704650 [details] following the STR from comment #0. I've just tried FF 45 on Linux (64bit) and it's reproducible there. I don't have a current FF dev build on Linux.
(In reply to Mats Palmgren (:mats) from comment #15) > I totally agree. Assigning bugs to people without asking them first is rude. > needinfo? is the mechanism to use in a situation like this (and the same > goes for bug 1237236 for that matter). I'm happy to help, but my analysis has taken me nowhere so far. (I'm really a Thunderbird developer. We sometimes visit M-C territory to fix bugs that affect us, mainly in Core::Editor.)
(In reply to Mats Palmgren (:mats) from comment #14) > I can't reproduce this in a Linux64 Nightly. (In reply to Jorg K (GMT+1) from comment #16) > I've just tried FF 45 on Linux (64bit) and it's reproducible there. I don't > have a current FF dev build on Linux. Now I do. I've just refreshed my build on Linux Mint (64bit) just after your bug 1237236 landed. I *can* reproduce the problem. Can you please take another look.
I believe you. I just don't have time to work on this.
Jet, can you help find an assignee?
(In reply to Jorg K (GMT+1) from comment #11) > Just looking at this again. The testcase that shows the error is this: > > <textarea style="height:200px;"> > A > asdf > > If you use complete HTML, so > > <textarea style="height:200px;"> > A > asdf > </textarea> > > the problem doesn't occur. > >Severity: normal → minor I agree with the "minor" severity noted here. With the workaround suggested, plus the regression risk, I think we can defer fixing this.
I'm still hoping that this might get fixed by bug 1258308.
(In reply to Jet Villegas (:jet) from comment #21) > With the workaround suggested, What workaround? I constantly get this issue while editing text. > plus the regression risk Please show a bug that doesn't have regression risk. Did bug 756984 NOT have that risk? Don't all bugs about implementing brand new (useless) stuff have regression risk? Seriously, if your reason is "nobody wants to ever fix this bug" / "this is desired pain for users", then ofc close it again. Other reasons are not convincing.
(In reply to arni2033 from comment #23) > (In reply to Jet Villegas (:jet) from comment #21) > > With the workaround suggested, > What workaround? I constantly get this issue while editing text. I'm referring to the </textarea> workaround noted in my last comment. > > plus the regression risk > Please show a bug that doesn't have regression risk. Did bug 756984 NOT have > that risk? <contenteditable> has higher than average regression risk. > Don't all bugs about implementing brand new (useless) stuff have regression > risk? > Seriously, if your reason is "nobody wants to ever fix this bug" / "this is > desired pain for users", then ofc close it again. Other reasons are not > convincing. I'm not disputing that this is a bug, and that nobody (perhaps you?) will ever fix it. What we want is to explicitly state that we are OK to continue to ship with this regression.
(In reply to Jet Villegas (:jet) from comment #24) > I'm referring to the </textarea> workaround noted in my last comment. That was a workaround for the situation when I load an HTML page I created myself and (for some reason) want to edit text on that page. Anyway there was no worded workaround for all cases. Of course I know one "workaround": to open Notepad, type desired text there and then paste it in <textarea> in Firefox. Please don't call such inconvenient things a "workaround" > we are OK to continue to ship with this regression Not that I was surprised, as I reported ~800 long-standing bugs I encounter every day (and still counting). But "Wontfix" is only for bugs that "will _never_ be fixed" (I've read the instruction)
(In reply to arni2033 from comment #25) > But "Wontfix" is only for bugs that "will _never_ be fixed" Yeah, I agree. Please keep filing bugs, it's very helpful to have these documented with simple testcases and a clear STR. Thanks!
This will be fixed with bug 1263288. I tested attachment 8740151 [details] [diff] [review] and it fixes this.