Closed Bug 503531 Opened 11 years ago Closed 10 years ago

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

Categories

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

defect

Tracking

()

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

People

(Reporter: sylvain.pasche, Assigned: mats)

References

(Depends on 1 open bug, )

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

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. 2Splinter 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: 10 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Attachment #403306 - Attachment is obsolete: true
Attached patch reftest.diffSplinter 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.