Closed Bug 503531 Opened 15 years ago Closed 15 years ago

Pressing TAB before a character in Google Docs moves the caret to a wrong position

Categories

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

Product:
Core
Component:
DOM: Editor
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.3a1
Tracking Flags:
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: sylvain.pasche, Assigned: MatsPalmgren_bugz)

References

(Depends on 1 open bug,
URL
)

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

Testcase that the patch breaks
577 bytes, text/html
Details
Patch rev. 2
3.03 KB, patch
roc
: review+
Details | Diff | Splinter Review
reftest.diff
2.49 KB, patch
Details | Diff | Splinter Review
This is a regression from bug 308736. To reproduce, open a Google Docs document, place the caret somewhere not on the end of a line and press TAB. The caret will correctly move horizontally to the right, but it is offset vertically and gets to the line below. If you move the caret after that, it goes to the right place. I wanted to create a reduced testcase, but I didn't manage to reproduce the Google Docs behavior on an empty designMode document. When I press TAB, the focus moves to the next focusable element instead of inserting some whitespace. I tried to attach an event listener for TAB and call the inserthtml command with the same markup that gets inserted in Google Docs, but that doesn't reproduce the issue. On Google docs, if you type "ab" in an empty document and place the caret between these two letters, the initial markup is (stripping irrelevant attributes): <body> ab<br><br> </body>" after you press TAB, there is: <body> a<span>&nbsp;&nbsp; &nbsp;</span>b<br><br> </body>
Flags: blocking1.9.2?
Flags: blocking1.9.2? → blocking1.9.2+
Peterv, if this is a regression from bug 308736, perhaps you could take a look?
Mats, maybe you could look at this?
Assignee: nobody → matspal
I'll take a look, not sure if I'll be able to fix it before beta1 though.
Attached patch Patch rev. 1 (obsolete) — — Splinter Review
This fixes it without regressing bug 308736. In addition to Google Docs, I also tested it with Midas and it works fine locally (on Linux). It's cooking on TryServer at the moment. Not sure how to write a reliable reftest... is there a way to make the caret draw without blinking? I found nsILookAndFeel::eMetric_CaretBlinkTime but it seems tied to native UI settings rather than a pref.
Attachment #403306 - Flags: review?(peterv)
Comment on attachment 403306 [details] [diff] [review] Patch rev. 1 Could you describe what the problem was? The fix is fine by me, but given that I don't completely understand the change I'm going to reassign this to roc. I don't know how to write caret testcases either, I never managed to make one for bug 308736.
Attachment #403306 - Flags: review?(peterv) → review?(roc)
One option for testing that would be perhaps generally useful would be to add an API window.mozGetCaretBoundingClientRect() which returns the viewport-relative bounding box of the caret. An alternative which would work but is less convenient would be to add "caret blink" as an invalidation reason in nsPaintRequest. Then if the caret is visible you can wait for it to blink and get the invalidation rectangle which tells you where the caret is.
(In reply to comment #5) > Could you describe what the problem was? I have to admit I don't know precisely, it just made sense to me that if we're synthesizing the caret height we need to adjust the caret y-pos to the baseline for all zero-height inlines, not just <br>. I doubt this code does the right thing for all edge cases though, in fact I found one which this patch "regresses"...
The caret should be placed below BLOCK, this was fixed by bug 308736. The patch in this bug makes us place the caret slightly above where it should be, as we do in 3.5.1 and earlier. I still think we should take the patch since it relatively safe - it relaxes the condition added by bug 308736 so we'd just return to previous behaviour. It does fix the more serious regression (Google Docs) and doesn't regress bug 308736. Given the short time to beta1 I think that's what we should do for now. What do you think?
Depends on: 519361
(spawned off the regression test discussion to bug 519361)
Attached patch Patch rev. 2 — — Splinter Review
This patch does not cause the regression mentioned in comment 8. It works with all the tests I could find. There's TryServer (1.9.2) builds here: https://build.mozilla.org/tryserver-builds/mpalmgren@mozilla.com-503531-2/ Let me know which patch you want for 1.9.2 and I'll land that.
Attachment #403514 - Flags: review?(roc)
Comment on attachment 403514 [details] [diff] [review] Patch rev. 2 Let's use this one.
Attachment #403514 - Flags: review?(roc) → review+
http://hg.mozilla.org/mozilla-central/rev/1e2e64a82a17
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Attachment #403306 - Attachment is obsolete: true
Attached patch reftest.diff — — Splinter Review
Pushed the reftest: http://hg.mozilla.org/mozilla-central/rev/e1933cd572c6
Flags: in-testsuite? → in-testsuite+
Depends on: 607548
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: