Invalid caret position on facebook's status textbox (styled with min-height and overflow-x or overflow-y)

RESOLVED FIXED in mozilla2.0b8

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

({regression, testcase})

Trunk
mozilla2.0b8
x86
macOS
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 final+)

Details

()

Attachments

(1 attachment, 2 obsolete attachments)

I noticed this on facebook.com's status textbox <http://grab.by/7tAb>.  I played with the page a bit until I could get a reproduced test case (look at the URL field).  This happens when the contenteditable field has min-height and overflow-x or overflow-y.

This is probably a regression from the recent caret work, and should definitely block Firefox 4, as it happens on a major website.
Assignee: nobody → ehsan
blocking2.0: --- → ?
Posted patch Patch (v1) (obsolete) — Splinter Review
The problem here is that GetCaretBaseline for nsHTMLScrollFrame falls back to nsFrame::GetBaseline which takes mRect.height into account.  The fix is simple: provide the same implementation of nsBlockFrame::GetCaretBaseline for nsHTMLScrollFrame as well.
Attachment #492114 - Flags: review?(roc)
I think you should just forward to the inner frame. That is where any content we insert will be inserted.
You might actually want to call GetContentInsertionFrame() explicitly somewhere to make this happen.
Posted patch Patch (v2) (obsolete) — Splinter Review
You're right, this will probably fix all of the future instances of this bug as well!
Attachment #492114 - Attachment is obsolete: true
Attachment #492567 - Flags: review?(roc)
Attachment #492114 - Flags: review?(roc)
+      document.body.offsetWidth;

Why is this needed? It should not be needed, reftests should flush when taking a snapshot.
(In reply to comment #5)
> +      document.body.offsetWidth;
> 
> Why is this needed? It should not be needed, reftests should flush when taking
> a snapshot.

Well, these two reftests failed with my patch without this change, but there was no visible difference between the test and reference images.  It seemed to be maybe a very subtle pixel color difference which I wasn't able to point out with my eyes.  Adding the flush manually fixed the issue, so I thought maybe that might have an effect on the rendering...
Use reftest-analyzer.xhtml to find the differences.

We should try to understand why the tests fail without the explicit layout flush.
(In reply to comment #7)
> Use reftest-analyzer.xhtml to find the differences.
> 
> We should try to understand why the tests fail without the explicit layout
> flush.

I did test this in reftest-analyzer.  I tried to rerun the tests without flushing, and this time they passed.  I then re-ran the tests multiple times, and I couldn't reproduce the failure any more.  I submit the patch to the try server without the flush and see what happens there...
Posted patch Patch (v2.1)Splinter Review
Patch without the flushes.
Attachment #492567 - Attachment is obsolete: true
Attachment #492708 - Flags: review?(roc)
Attachment #492567 - Flags: review?(roc)
Comment on attachment 492708 [details] [diff] [review]
Patch (v2.1)

Requesting approval, but I think that this should block.
Attachment #492708 - Flags: approval2.0?
Attachment #492708 - Flags: approval2.0?
http://hg.mozilla.org/mozilla-central/rev/b5aa5e3dc568
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
You need to log in before you can comment on or make changes to this bug.