Closed Bug 613433 Opened 10 years ago Closed 10 years ago
Invalid caret position on facebook's status textbox (styled with min-height and overflow-x or overflow-y)
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.
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.
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.
You're right, this will probably fix all of the future instances of this bug as well!
+ 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...
Patch without the flushes.
Attachment #492708 - Flags: review?(roc) → review+
Comment on attachment 492708 [details] [diff] [review] Patch (v2.1) Requesting approval, but I think that this should block.
blocking2.0: ? → final+
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
You need to log in before you can comment on or make changes to this bug.