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

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[(no longer active)]

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.

Comment 1

[(no longer active)]

Attachment: Patch (v1)

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.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think you should just forward to the inner frame. That is where any content we insert will be inserted.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You might actually want to call GetContentInsertionFrame() explicitly somewhere to make this happen.

Comment 4

[(no longer active)]

Attachment: Patch (v2)

You're right, this will probably fix all of the future instances of this bug as well!

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+      document.body.offsetWidth;

Why is this needed? It should not be needed, reftests should flush when taking a snapshot.

Comment 6

[(no longer active)]

(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...

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Use reftest-analyzer.xhtml to find the differences.

We should try to understand why the tests fail without the explicit layout flush.

Comment 8

[(no longer active)]

(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...

Comment 9

[(no longer active)]

Attachment: Patch (v2.1)

Patch without the flushes.

Comment 10

[(no longer active)]

Comment on attachment 492708 [details] [diff] [review]
Patch (v2.1)

Requesting approval, but I think that this should block.

Comment 11

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/b5aa5e3dc568