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)
Core
DOM: Editor
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a1
| Tracking | Status | |
|---|---|---|
| status1.9.2 | --- | beta1-fixed |
People
(Reporter: sylvain.pasche, Assigned: MatsPalmgren_bugz)
References
(Depends on 1 open bug, )
Details
(Keywords: regression)
Attachments
(3 files, 1 obsolete file)
|
(deleted),
text/html
|
Details | |
|
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
|
(deleted),
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> </span>b<br><br> </body>
|
||
Updated•15 years ago
|
Flags: blocking1.9.2?
Flags: blocking1.9.2? → blocking1.9.2+
|
||
Comment 1•15 years ago
|
||
Peterv, if this is a regression from bug 308736, perhaps you could take a look?
|
||
Updated•15 years ago
|
Priority: -- → P1
Mats, maybe you could look at this?
Assignee: nobody → matspal
|
Assignee | |
Comment 3•15 years ago
|
||
I'll take a look, not sure if I'll be able to fix it before beta1 though.
|
|
Assignee | |
Comment 4•15 years ago
|
||
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 5•15 years ago
|
||
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)
Attachment #403306 -
Flags: review?(roc) → review+
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.
|
|
Assignee | |
Comment 7•15 years ago
|
||
(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"...
|
|
Assignee | |
Comment 8•15 years ago
|
||
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?
|
|
Assignee | |
Comment 9•15 years ago
|
||
(spawned off the regression test discussion to bug 519361)
|
|
Assignee | |
Comment 10•15 years ago
|
||
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+
|
|
Assignee | |
Comment 12•15 years ago
|
||
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
|
|
Assignee | |
Updated•15 years ago
|
Attachment #403306 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 13•15 years ago
|
||
status1.9.2:
--- → beta1-fixed
|
|
Assignee | |
Comment 14•15 years ago
|
||
|
|
Assignee | |
Comment 15•15 years ago
|
||
Pushed the reftest:
http://hg.mozilla.org/mozilla-central/rev/e1933cd572c6
Flags: in-testsuite? → in-testsuite+
|
||
Updated•9 years ago
|
Keywords: testcase-wanted
You need to log in
before you can comment on or make changes to this bug.
Description
•