389321 - Caret invisible or at wrong place with empty contenteditable nodes

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

When elements are empty, the caret won't show up for contenteditable elements.

For the first element in the testcase, the caret won't show up when focusing it.
For the second element in the testcase, the caret shows up at the wrong place when focusing it. Also, when you type something, a 'ghost' caret remnant can remain when you type at the wrong moment (when the caret is visible).
For the second element, the caret won't show up when focusing and it seems impossible to type anything in it, at all.

The fourth element, seems to work fine (I just added it for completeness).

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Ok, there are a few problems here:

1. In an empty div with a border, the caret is drawn overlapping the left border (in LTR mode at least).
2. "<div> </div>" test case has a text frame inside it, but it's 0 height. GetOffsetTo() isn't pushing the offset down far enough, and the caret is being drawn vertically offset incorrectly.
3. <span> </span> fails to allow insertion of text, there's assertion is failing then.

Taking bug.

Comment 2

[Martijn Wargers (dead)]

I guess this might be related to bug 308736 too.

Comment 3

[Kevin Field]

I'm having this too in Firefox 3.0.7 on win32, really annoying for our web app.  I just found that the cursor is actually there, but *above* (and to the left of) the DIV having contenteditable set--if you put just one space character inside the DIV, you can see the cursor a line up from the first line, blinking on top of the other page content, with only 2px worth of cursor actually being in the DIV.  If you put <br> (as mentioned in bug 308736) the cursor shows up in the right spot.  Oddly enough, backspacing does not make the cursor disappear (although I guess it makes sense to some degree, since I can see in Firebug that a <br _moz_dirty=""/> element is still there despite any backspacing or delete-key-pressing one might do.)

(For me the <br> thing happened to be an acceptable workaround because we trim <br>s at the server anyway.)

Comment 5

[Peter Van der Beken [:peterv]]

(In reply to comment #1)
> 2. "<div> </div>" test case has a text frame inside it, but it's 0 height.
> GetOffsetTo() isn't pushing the offset down far enough, and the caret is being
> drawn vertically offset incorrectly.

This might have been fixed by bug 308736?

Comment 6

[Emil Ivanov]

Well actually the caret is there, its just over the border with the same color. Set the border color to different than the text color in the testcase and you will see it.
So the caret is at the wrong place after all, and its with 100% height, something like bug 542116

Comment 7

[luxigo]

Attachment: workaround / testcase..

Found a way to display the caret properly using CSS and <br> ...

But there's other bugs showing up in my testcase:

- Backspace delete characters in previous contentEditable elements until reaching an empty element

- Backspace dont delete the selected characters when using css but no <br>

Comment 8

[luxigo]

Positionning the caret before the first character of a field on the 3rd line and trying to delete all characters with <del>, results in selecting the last one.

Comment 9

[luxigo]

on the first line too (both are without ending <br>)

Comment 10

[bmccann]

This is one of a few contentEditable bugs that has made it difficult/impossible to implement Google Spreadsheet's formula highlighting in Firefox.  We'd like to turn the feature on in FF 3.7 assuming enough of the issues have been addressed in the browser.  Thanks!
http://googledocs.blogspot.com/2010/05/formula-highlighting-in-spreadsheets.html

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

Reassigning to Ehsan.

Comment 12

[(no longer active)]

So, here's the current situation for the first test case:

(In reply to comment #0)
> Created an attachment (id=273490) [details]
> testcase
> 
> When elements are empty, the caret won't show up for contenteditable elements.
> 
> For the first element in the testcase, the caret won't show up when focusing
> it.

The caret shows up in the first test case now 

> For the second element in the testcase, the caret shows up at the wrong place
> when focusing it. Also, when you type something, a 'ghost' caret remnant can
> remain when you type at the wrong moment (when the caret is visible).

For the second test case, the caret doesn't get drawn when you first focus the element, but if you type something in it and then delete it, we'll draw the caret correctly (which is probably no surprise, since the bug happens because of the empty space).

I didn't manage to reproduce the ghost caret problem though.

> For the second element, the caret won't show up when focusing and it seems
> impossible to type anything in it, at all.

The third element seems to be working fine right now (the caret shows up correctly and typing also works.

> The fourth element, seems to work fine (I just added it for completeness).

It's still working!  So I guess the only problem to look into in this test case is the second element.

(In reply to comment #7)
> Created an attachment (id=443835) [details]
> workaround / testcase..
> 
> Found a way to display the caret properly using CSS and <br> ...

The span element in the first row and 2nd column has zero width and zero height.  The span element in the third row and 2nd column has non-zero width and zero height.  So I guess we need to fix the caret positioning for elements with 0 height.

> But there's other bugs showing up in my testcase:
> 
> - Backspace delete characters in previous contentEditable elements until
> reaching an empty element

That should probably be its own bug.  I filed bug 577367 for it.  I also found bug 577365 when I was inspecting this.

> - Backspace dont delete the selected characters when using css but no <br>

I don't quite get what you meant here.

(In reply to comment #8)
> Positionning the caret before the first character of a field on the 3rd line
> and trying to delete all characters with <del>, results in selecting the last
> one.

That might also be something similar to bug 577365. (see bug 577365 comment 1).

Comment 13

[(no longer active)]

Attachment: Part 1: Don't choke on textnodes which don't have a primary frame

The first problem to solve here is that the caret positioning code does not correctly handle the case where we have a collapsible whitespace text node for which we don't create a frame (therefore the primary frame is null).  This patch makes us just use the parent frame, just like the case where there is no text node.

It fixes the second element problem in the first test case.

Comment 14

[(no longer active)]

Attachment: Part 2: Fix a caret which doesn't get erased completely

I came across a problem which is actually a regression from bug 581585.  We fail to erase the caret completely when entering a character into one of the span elements and then deleting it, because we're using a cached frame when we're not supposed to.  This patch fixes that problem and adds a test.

Comment 15

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

   // Recompute the frame that we're supposed to draw in if the cached frame
   // is stale (dead).

This comment is no longer correct, because we no longer care about the cached frame here.

I think instead of adding a boolean parameter (boolean parameters aren't nice) it would be better to have a method that clears the cached frame, and just call it before we call GetCaretFrameForNodeOffset.

Comment 16

[(no longer active)]

Attachment: Part 2: Fix a caret which doesn't get erased completely

You're right, this way it's much simpler!

Comment 17

[(no longer active)]

Mats, in bug 503531, you added this line:

<http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCaret.cpp#384>

I may be completely insane, but shouldn't that condition be reverse?  (i.e., shouldn't we drop the ! operator)?  The comment says that we're placing the caret on the baseline unless there's a frame on this line with non-zero height.  But the way that the code works, we only place the caret on the baseline if there is a frame with non-zero height on the line.

Also, reading bug 503531, I don't really get why this condition was added.  Would you care to elaborate, please?

Comment 18

[(no longer active)]

Attachment: Part 3: Fix the caret location with 0-height inline frames without textframes

OK, this is the dirtiest patch that I've ever submitted for review, but I don't really know any better way to fix things.  I basically don't understand the check added in bug 503531, but it shouldn't affect non-textframe inline frames, as far as I can tell.

I'm open to suggestions, but this patch seems not to regress any caret positioning tests that we have, and fixes the span in the first row and 2nd column on the second test case.

Comment 19

[(no longer active)]

Attachment: Part 3: Fix the caret location with 0-height inline frames without textframes

Fixed a typo

Comment 20

[(no longer active)]

Attachment: Part 4: Add another exception for floating frames with 0 height

I'm liking this bug less and less.  I guess we need to extend the exception to cover floats as well, in order to take care of the third row and second column in the second test case.  :-(

Comment 21

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

I think we should take a step back here and think more about how we want the caret to be sized and positioned.

The caret is supposed to represent what's going to happen if text is inserted. So why don't we *always* use the font ascent for the height of the caret, and position the bottom of the caret at the frame's baseline by calling nsIFrame::GetBaseline? You would need to actually implement nsIFrame::GetBaseline for all the frames that set aMetrics.ascent instead of using ASK_FOR_BASELINE, but that's only text frames, bullets, inline frames and BR frames.

Comment 22

[(no longer active)]

Well, I had a similar discussion with Shaver yesterday.  Here's my take on the issue:

Carets are used for three purposes: figuring out where data (text, images, etc) insertion goes, and as an aid for navigation and selection.  As far as the latter two purposes are concerned, people don't (shouldn't?) care about the height of the caret, they should only care about where on the screen it is, so as long as it's visible compared to its surrounding content, it will serve its purpose.  I _suspect_ that the caret size being set to the height of the frame it's attached to is coming from these purposes.  Let's consider the case where you have three large images on the same line next to each other, and a CSS rule is imposing a 14px high font size.  If we choose to use the ascent (possibly plus the descent) of the font metrics for the frame for the size of the caret, the caret will practically be invisible when navigating between the images, which would mean that it will fail to serve the latter two purposes.

As far as data insertion goes, a fundamental problem is that we don't always have a very good idea on how big the content to be inserted will be.  This is obviously true for things like images, but let's just consider the simple case of text.  If we have a paragraph which has a :first-letter style set to a bigger font size than the rest of the text on the paragraph, we should _probably_ use that font size when placing the caret on an empty block frame, because we expect the text to be bigger for the first letter.  But that wouldn't probably be nice, because the caret height will exceed the block frame height (and the caret will potentially run into preceding/following content.  There are many more ways to construct similar cases, especially with script modifying our DOM taken into consideration, but this example demonstrates the problem.

These are all general issues on the difficulty associated with figuring out a sensible height for the caret.  Your suggestions seems good to me, although I need to experiment with it in order to say with more certainty whether it solves the problem at hand.  I'll experiment with it and post some patches later today, hopefully!

Comment 23

[(no longer active)]

Attachment: WIP

This is a first start at implementing the suggestion in comment 21.  There is one problem: the position of the frames with 0 height inside their parents are not consistent, so even with this approach I think we would still need to hack around that, and if that's the case, I'm not sure what the advantage of doing this would be...

I'll upload a test case which shows where we position an inline frame based on its contents and whether or not it's followed by a text frame for example.

Comment 24

[(no longer active)]

Attachment: Test case for comment 23

Comment 25

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

(In reply to comment #22)
> Carets are used for three purposes: figuring out where data (text, images, etc)
> insertion goes, and as an aid for navigation and selection.  As far as the
> latter two purposes are concerned, people don't (shouldn't?) care about the
> height of the caret, they should only care about where on the screen it is, so
> as long as it's visible compared to its surrounding content, it will serve its
> purpose.  I _suspect_ that the caret size being set to the height of the frame
> it's attached to is coming from these purposes.

I suspect that when this code was originally written people hadn't thought about it in a principled way at all :-)

> Let's consider the case where
> you have three large images on the same line next to each other, and a CSS rule
> is imposing a 14px high font size.  If we choose to use the ascent (possibly
> plus the descent) of the font metrics for the frame for the size of the caret,
> the caret will practically be invisible when navigating between the images,
> which would mean that it will fail to serve the latter two purposes.

That is true, but even a very tall 1px wide caret easily gets lost when it's adjacent to an image.

> As far as data insertion goes, a fundamental problem is that we don't always
> have a very good idea on how big the content to be inserted will be.  This is
> obviously true for things like images, but let's just consider the simple case
> of text.  If we have a paragraph which has a :first-letter style set to a
> bigger font size than the rest of the text on the paragraph, we should
> _probably_ use that font size when placing the caret on an empty block frame,
> because we expect the text to be bigger for the first letter.  But that
> wouldn't probably be nice, because the caret height will exceed the block frame
> height (and the caret will potentially run into preceding/following content. 
> There are many more ways to construct similar cases, especially with script
> modifying our DOM taken into consideration, but this example demonstrates the
> problem.

Yeah, it's a problem for any caret sizing approach and there's not much we can do about it.

> These are all general issues on the difficulty associated with figuring out a
> sensible height for the caret.  Your suggestions seems good to me, although I
> need to experiment with it in order to say with more certainty whether it
> solves the problem at hand.  I'll experiment with it and post some patches
> later today, hopefully!

Great!

Comment 26

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

We don't want to add mHeight to nsTextFrame. mAscent should give you the baseline. In fact it seems to me you don't need to add members to any of the other frames either; in each case, the calculations performed during reflow can be done by GetBaseline (and probably shared with some code refactoring).

Comment 27

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

Attachment: tweaked testcase

I like this one slightly better since the font sizes are consistent

Comment 28

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

First of all, I don't know why the empty spans have zero height. I don't know if the spec requires that. If it doesn't, that's probably just a layout bug that we should fix. Note that Webkit makes the empty spans the height of the line box, or at least draws the overflow as if it did.

Even apart from that, I see that the span y is at the top of the line-box. If the span ascent was the font-ascent, wouldn't the caret be in the right place? The fact that the bottom of the caret would be outside the frame doesn't matter.

Comment 29

[Mats Palmgren (inactive)]

I started writing this comment before the last few comments so 
it may be obsolete already, anyway:

(In reply to comment #17)
> I may be completely insane, but shouldn't that condition be reverse? 

The code is correct, the comment about the exception is wrong and
should be changed to match the code.

Just to clarify, we *always* want to place the caret "on the baseline",
it's just that in certain cases for zero height frames the frame
position is different from what it is when the frame has some height,
but we want the caret position to be the same on the screen.
That's why we need to adjust it here.

In some cases it might be that the frame position is actually wrong.
Take the 1st row, 2nd column in attachment 467483 [details] for example.
The 'x' here is not on the same baseline as the 1st column.
If you type a space before the 'x' (inside the span), the 'x' drops
to be on the same baseline.  In this case, I think the caret
is placed correctly given the layout, but maybe the layout is wrong?

For the 3rd row, we need to adjust this differently than the
current code does, I tried
      else if (aFrame->GetParent()->GetStyleContext()->GetPseudo() ==
               nsCSSAnonBoxes::cellContent)
        framePos.y -= height / 2;
which seems to work as expected.

Comment 30

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

It's probably worth figuring what sets the span's height to zero. I had a quick look but couldn't find it. Check the nsHTMLReflowMetrics returned by nsInlineFrame::Reflow, if it's not zero there then it might be cleared in nsLineLayout::VerticalAlignFrames and related code.

Comment 31

[Mats Palmgren (inactive)]

There are quirks at play here, at least in the table testcases.
Add <!DOCTYPE html> and the caret looks fine to me.

Comment 32

[(no longer active)]

(In reply to comment #25)
> (In reply to comment #22)
> > Carets are used for three purposes: figuring out where data (text, images, etc)
> > insertion goes, and as an aid for navigation and selection.  As far as the
> > latter two purposes are concerned, people don't (shouldn't?) care about the
> > height of the caret, they should only care about where on the screen it is, so
> > as long as it's visible compared to its surrounding content, it will serve its
> > purpose.  I _suspect_ that the caret size being set to the height of the frame
> > it's attached to is coming from these purposes.
> 
> I suspect that when this code was originally written people hadn't thought
> about it in a principled way at all :-)

That's certainly possible!

> > Let's consider the case where
> > you have three large images on the same line next to each other, and a CSS rule
> > is imposing a 14px high font size.  If we choose to use the ascent (possibly
> > plus the descent) of the font metrics for the frame for the size of the caret,
> > the caret will practically be invisible when navigating between the images,
> > which would mean that it will fail to serve the latter two purposes.
> 
> That is true, but even a very tall 1px wide caret easily gets lost when it's
> adjacent to an image.

True, depending on the surroundings.  We could be doing things to make the caret easier to spot generally, but I think most of them don't necessarily boil down to sizing or positioning the caret differently.

> > As far as data insertion goes, a fundamental problem is that we don't always
> > have a very good idea on how big the content to be inserted will be.  This is
> > obviously true for things like images, but let's just consider the simple case
> > of text.  If we have a paragraph which has a :first-letter style set to a
> > bigger font size than the rest of the text on the paragraph, we should
> > _probably_ use that font size when placing the caret on an empty block frame,
> > because we expect the text to be bigger for the first letter.  But that
> > wouldn't probably be nice, because the caret height will exceed the block frame
> > height (and the caret will potentially run into preceding/following content. 
> > There are many more ways to construct similar cases, especially with script
> > modifying our DOM taken into consideration, but this example demonstrates the
> > problem.
> 
> Yeah, it's a problem for any caret sizing approach and there's not much we can
> do about it.

True.

Comment 33

[(no longer active)]

(In reply to comment #26)
> We don't want to add mHeight to nsTextFrame. mAscent should give you the
> baseline. In fact it seems to me you don't need to add members to any of the
> other frames either; in each case, the calculations performed during reflow can
> be done by GetBaseline (and probably shared with some code refactoring).

Well, that's kind of like what I tried at the beginning, but I think we actually want the caret height to be equal to ascent + descent, not ascent.  The caret looks weird if height == ascent.  And making the caret height ascent+descent means that you can't really position it on the baseline.

Comment 34

[(no longer active)]

(In reply to comment #27)
> Created attachment 467549 [details]
> tweaked testcase
> 
> I like this one slightly better since the font sizes are consistent

Yes, but it doesn't demonstrate the problem, because it's rendered in standards mode I guess.

Comment 35

[(no longer active)]

(In reply to comment #28)
> First of all, I don't know why the empty spans have zero height. I don't know
> if the spec requires that. If it doesn't, that's probably just a layout bug
> that we should fix. Note that Webkit makes the empty spans the height of the
> line box, or at least draws the overflow as if it did.

Yes.  I noticed this as well.  I tried Opera as well, but they don't render the outlines at all, and I didn't know how I could actually measure the height of the "frame" that they generate.

> Even apart from that, I see that the span y is at the top of the line-box. If
> the span ascent was the font-ascent, wouldn't the caret be in the right place?

Maybe.  We need to decide what the correct height of the caret should be first, I think.

> The fact that the bottom of the caret would be outside the frame doesn't
> matter.

Why?

Comment 36

[(no longer active)]

(In reply to comment #29)
> (In reply to comment #17)
> > I may be completely insane, but shouldn't that condition be reverse? 
> 
> The code is correct, the comment about the exception is wrong and
> should be changed to match the code.

That's what I gathered after inspecting the code, but thanks for confirming it.

> Just to clarify, we *always* want to place the caret "on the baseline",

Is that actually true?  That's not what we currently do for text frames, for example, at least, those which actually contain text.

> it's just that in certain cases for zero height frames the frame
> position is different from what it is when the frame has some height,
> but we want the caret position to be the same on the screen.
> That's why we need to adjust it here.

Yes, but those adjustments don't place the caret on the baseline.  They place the caret below the baseline by "font-descent" units.  But I agree that those adjustments make the caret appear on the correct position on the screen anyway.

> In some cases it might be that the frame position is actually wrong.
> Take the 1st row, 2nd column in attachment 467483 [details] for example.
> The 'x' here is not on the same baseline as the 1st column.
> If you type a space before the 'x' (inside the span), the 'x' drops
> to be on the same baseline.  In this case, I think the caret
> is placed correctly given the layout, but maybe the layout is wrong?

Hmm, that's a very good point.  It appears to me that the layout is wrong, but I'm not the best person to judge that for sure.

> For the 3rd row, we need to adjust this differently than the
> current code does, I tried
>       else if (aFrame->GetParent()->GetStyleContext()->GetPseudo() ==
>                nsCSSAnonBoxes::cellContent)
>         framePos.y -= height / 2;
> which seems to work as expected.

Interesting...  Although I don't really get the rationale of this adjustment.

Comment 37

[(no longer active)]

(In reply to comment #30)
> It's probably worth figuring what sets the span's height to zero. I had a quick
> look but couldn't find it. Check the nsHTMLReflowMetrics returned by
> nsInlineFrame::Reflow, if it's not zero there then it might be cleared in
> nsLineLayout::VerticalAlignFrames and related code.

I'll do that tomorrow (or maybe tonight, depending on how the rest of the evening goes!).

But here's a general question: when debugging reflows, I always get lost in the dozens of recursive calls that we make.  One approach that I recently adopted was to open the test case in Layout Debugger, dump the frame tree, take note of the frame address, add a conditional breakpoint on the Reflow method which checks |this| against that address, so that I can be sure that I'm looking at the right frame, and then cause a reflow.  Is there a better way of delving into our reflow code?

Comment 38

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

(In reply to comment #35)
> (In reply to comment #28)
> > The fact that the bottom of the caret would be outside the frame doesn't
> > matter.
> 
> Why?

Because we can draw the caret outside the frame just fine.

(In reply to comment #33)
> (In reply to comment #26)
> > We don't want to add mHeight to nsTextFrame. mAscent should give you the
> > baseline. In fact it seems to me you don't need to add members to any of the
> > other frames either; in each case, the calculations performed during reflow
> > can be done by GetBaseline (and probably shared with some code refactoring).
> 
> Well, that's kind of like what I tried at the beginning, but I think we
> actually want the caret height to be equal to ascent + descent, not ascent. 
> The caret looks weird if height == ascent.

You're absolutely right, sorry.

> And making the caret height
> ascent+descent means that you can't really position it on the baseline.

OK, what I really mean is that the caret's baseline should be positioned at the frame baseline. So the caret y would be at frame y + frame baseline - font ascent, and the caret height would be font ascent + font descent.

And I'm becoming more convinced that where the frame position or baseline for empty frames makes no sense, that's a layout bug we should fix.

(In reply to comment #37)
> One approach that I recently adopted was to open the test case in Layout
> Debugger, dump the frame tree, take note of
> the frame address, add a conditional breakpoint on the Reflow method which
> checks |this| against that address, so that I can be sure that I'm looking at
> the right frame, and then cause a reflow.  Is there a better way of delving
> into our reflow code?

That's pretty much what I do, except that I don't use the Layout Debugger. Instead I get a pointer to a frame in the document (e.g. by setting a breakpoint in nsCanvasFrame::BuildDisplayList), then call nsFrame::DumpFrameTree(<frameptr>) from the debugger.

There is logging code you can turn on but for this sort of thing it's probably not that useful.

Comment 39

[(no longer active)]

Attachment: WIP 2

Another WIP based on comment 38.  It is pretty broken at this stage, I'm just attaching it so that roc can take a look.  More to come.

Comment 40

[(no longer active)]

Attachment: WIP 3

OK, this is better.  This patch mostly positions the caret correctly, except for two cases:

1. For block frames, the frame's baseline is actually equal to the frame height.  This causes the caret to appear by a Y offset equal to font descent below where it should.  While I _could_ special case block frames, is there any more elegant way to handle that?

2. The caret is not positioned correctly for the weird frames (the layout bugs you mentioned before).  I'm going to investigate each of them and try to fix them in their own patches.

Comment 41

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

(In reply to comment #40)
> 1. For block frames, the frame's baseline is actually equal to the frame
> height.  This causes the caret to appear by a Y offset equal to font descent
> below where it should.  While I _could_ special case block frames, is there any
> more elegant way to handle that?

Got a testcase for this?

Comment 42

[(no longer active)]

Attachment: caret on a block frame (screenshot)

(In reply to comment #41)
> (In reply to comment #40)
> > 1. For block frames, the frame's baseline is actually equal to the frame
> > height.  This causes the caret to appear by a Y offset equal to font descent
> > below where it should.  While I _could_ special case block frames, is there any
> > more elegant way to handle that?
> 
> Got a testcase for this?

Yep:

data:text/html,<div contenteditable style="height: 30px; outline:1px solid red">

Here is how the caret looks like.

Comment 43

[(no longer active)]

And also, we currently render the caret with its height equal to the block frame height, so my patch regresses things like the test case for bug 539323 <http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/539323-1.html>.

Comment 44

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

(In reply to comment #42)
> data:text/html,<div contenteditable style="height: 30px; outline:1px solid
> red">

Good testcase. So the issue here is that nsIFrame::GetBaseline returns a baseline that's really for "outside" the frame, that aligns the frame itself with content outside it. That's not what we want for blocks.

I guess we need a new method, say GetCaretBaseline, that returns GetBaseline but can be overridden for blocks and possibly other frame types. When the caret is in a block like this, I guess we should return the baseline of the first line, or if there is no first line, the baseline the first line would have if there was one.

(In reply to comment #43)
> And also, we currently render the caret with its height equal to the block
> frame height, so my patch regresses things like the test case for bug 539323
> <http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/539323-1.html>.

That's OK, because that's a bad caret height --- it's dumb to show a huge caret and then after inserting one character, the caret suddenly shrinks to the right size.

Comment 45

[(no longer active)]

(In reply to comment #44)
> I guess we need a new method, say GetCaretBaseline, that returns GetBaseline
> but can be overridden for blocks and possibly other frame types. When the caret
> is in a block like this, I guess we should return the baseline of the first
> line, or if there is no first line, the baseline the first line would have if
> there was one.

Makes sense.  Especially since I'm fairly certain that we'd need a custom implementation for GetCaretBaseline for some other frame types that I just haven't thought of testing yet.

Given such a method, do you want me to move the newly implemented GetBaseline methods over to it, or does it make sense to leave the GetBaseline implementation there for the rest of the layout code to use as well?

> (In reply to comment #43)
> > And also, we currently render the caret with its height equal to the block
> > frame height, so my patch regresses things like the test case for bug 539323
> > <http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/539323-1.html>.
> 
> That's OK, because that's a bad caret height --- it's dumb to show a huge caret
> and then after inserting one character, the caret suddenly shrinks to the right
> size.

OK, I'll adjust the test cases then.

Comment 46

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

(In reply to comment #45)
> Makes sense.  Especially since I'm fairly certain that we'd need a custom
> implementation for GetCaretBaseline for some other frame types that I just
> haven't thought of testing yet.
> 
> Given such a method, do you want me to move the newly implemented GetBaseline
> methods over to it, or does it make sense to leave the GetBaseline
> implementation there for the rest of the layout code to use as well?

Leave them there for now. Once you're done, if there are GetBaseline methods that are not called by GetCaretBaseline or anywhere else, we should probably remove them before landing.

Comment 47

[(no longer active)]

Attachment: Part 3: Use a centralized algorithm for caret positioning

Comment 48

[(no longer active)]

This new patch implements GetCaretBaseline...

Comment 49

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

I don't think you need to add mBaseline to nsBRFrame. You can recalculate it based on what Reflow() does (plus maybe a frame state bit to indicate whether we took the path to call nsLayoutUtils::GetCenteredFontBaseline). Note that 'logicalHeight' will be available as frame->GetRect().mHeight. Although it seems to me that we should always be giving the BR frame nonzero height and ascent.

Similar story for nsBulletFrame. The ascent can be reverse engineered from the frame size and bullet type, possibly with some frame state bits to let you know which path was taken.

Similar for nsInlineFrame. The ascent is always the font ascent plus top border+padding. (It's only zero in the failure case.)

You do not want to be calling GetClientRect anywhere. Call GetUsedBorderPadding or GetContentRect instead.

nsTextFrame can't have border/padding so you can simplify nsTextFrame::GetBaseline.

Comment 50

[(no longer active)]

Attachment: WIP: compute the correct height and ascent for empty text frames

I took a stab at making sure that empty text frames get the correct height and ascend, but this patch doesn't seem to have any effect on where the caret for an empty text frame appears (I'm using attachment 403378 [details] as a test case).  roc: do you happen to have any ideas why?

Comment 51

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

Not without debugging.

Comment 52

[(no longer active)]

Attachment: Part 3: Use a centralized algorithm for caret positioning

With comment 49 addressed.

Comment 53

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

nsBulletFrame could be simpler:
-- use a state bit to record whether we took the "ascent = GetRect().height;" (checking the image status might be out of sync with what we actually did during Reflow, the status could have changed)
-- remove all the case NS_STYLE_LIST_ cases after "default:", they're not needed

+  NS_ASSERTION(!NS_SUBTREE_DIRTY(this), "frame must not be empty");

Assertion text doesn't match the assertion. Can we actually be sure the subtree isn't dirty? I don't think so, we might be doing caret stuff while there's a pending reflow.

Comment 54

[(no longer active)]

Attachment: Part 3: Use a centralized algorithm for caret positioning

Comment 55

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

Comment on attachment 470847 [details] [diff] [review]
Part 3: Use a centralized algorithm for caret positioning

+        fm->GetMaxAscent(ascent);
+        bottomPadding = NSToCoordRound(float(ascent) / 8.0f);
+        ascent = NS_MAX(nsPresContext::CSSPixelsToAppUnits(MIN_BULLET_SIZE),
+                        NSToCoordRound(0.8f * (float(ascent) / 2.0f)));
+        ascent += bottomPadding;

Move this to a function that can be shared from Reflow? Maybe even the entire 'switch'?

Comment 56

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

I guess we need some way to test this too.

Comment 57

[(no longer active)]

I have modified attachment 403378 [details] to generate a non-empty text node in order to compare the frame trees between the cases where the 2nd line contains only empty text frames and when it's not.

Here's a frame tree of that test case in its original form <http://pastebin.mozilla.org/779870> and here's a frame tree of that test case modified to add a text frame containing "x" <http://pastebin.mozilla.org/779871>.

Comment 58

[(no longer active)]

In my talk with roc on this bug yesterday, he suggested that I try setting mAscent correctly when we're dealing with an empty text frame or one in which all of the content is collapsed white-space.  I tried that solution, and to my great surprise, it didn't change the vertical position of the caret in attachment 403378 [details] at all!  framePos.y is set to -720 when mAscent is not set, and to 0 when mAscent is set, but the position of the caret on the screen doesn't change in either of those cases!

Comment 59

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

I guess that makes sense. we add the ascent to the frameY to figure out where to position the caret.

I guess the reason that setting the ascent didn't work is that nsLineLayout::VerticalAlignFrames does this:

          case NS_STYLE_VERTICAL_ALIGN_BASELINE:
            // The elements baseline is aligned with the baseline of
            // the parent.
            pfd->mBounds.y = baselineY - pfd->mAscent;
            pfd->mVerticalAlign = VALIGN_OTHER;

I.e., it's aligning the baseline of the frame with the baseline of the line, and the baseline of the line is at 0.

I'm not sure of the best way to fix this. We don't really want to mess with line layout here. How about we leave the textframe ascent alone and make nsTextFrame::GetCaretBaseline just return fontMetrics->GetMaxAscent always? And ditto for nsInlineFrame?

Comment 60

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

Actually, I've got a better idea.

0) call GetCaretBaseline on the caret frame as you have already
1) find the line that the caret frame is on
2) get the line's ascent (nsLineBox::GetAscent).
3) if the line's ascent is less than the caret's ascent (i.e. what we get from the font), the line is too short for the caret. Move the caret down by the difference in the ascents.

This basically means "if the line's baseline is too close to the top of the line to fit the caret, treat the line's baseline as if it had moved down enough to fit the caret".

Comment 61

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

Although it really depends on where we want the caret to be for empty lines. Sometimes it seems you'd want the empty line baseline as being the Y-coordinate of the line --- e.g. if it's the last line of a block that clips its contents. Sometimes you want the empty line baseline to be below the Y coordinate of the line --- e.g. if it's the first line of a block that clips its contents.

If we can figure out what the desired behavior is, then I'm pretty sure we could implement it by extending the approach of comment #60.

Comment 62

[(no longer active)]

Attachment: Part 3: Use a centralized algorithm for caret positioning

Seems like modifying GetBaseline on first-letter frames caused a reftest to fail (http://mxr.mozilla.org/mozilla-central/source/layout/reftests/first-letter/429968-1a.html?force=1 and http://mxr.mozilla.org/mozilla-central/source/layout/reftests/first-letter/429968-1b.html?force=1).  I moved the code to GetCaretBaseline.

I'm wondering if I should do the same thing for the rest of the frames types that I have modified in this patch to add a GetBaseline function as well?

Comment 63

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

I don't think so. I think it should be OK to return the baseline in nsFirstLetterFrame::GetBaseline anyway. I'd like to understand the first-letter frame test failure when GetBaseline is overridden. How is nsFirstLetterFrame::GetBaseline even called during those testcases?

BTW it seems to me that mBaseline includes the top border+padding so you should not be adding that again in GetBaseline. But that shouldn't have affected those testcases.

Comment 64

[(no longer active)]

(In reply to comment #63)
> I don't think so. I think it should be OK to return the baseline in
> nsFirstLetterFrame::GetBaseline anyway. I'd like to understand the first-letter
> frame test failure when GetBaseline is overridden. How is
> nsFirstLetterFrame::GetBaseline even called during those testcases?

I was wrong in assuming that is the source of the problem.  I'm investigating it more closely now.

> BTW it seems to me that mBaseline includes the top border+padding so you should
> not be adding that again in GetBaseline. But that shouldn't have affected those
> testcases.

I don't think so.  This is where mBaseline is set <http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFirstLetterFrame.cpp#247>, and I can't see how it would include top border and padding...

Comment 65

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

because we just added the top border and padding to ascent in the previous line:
  aMetrics.ascent += bp.top;

Comment 66

[(no longer active)]

(In reply to comment #65)
> because we just added the top border and padding to ascent in the previous
> line:
>   aMetrics.ascent += bp.top;

Ah yes, don't know why I missed that.  Will fix it in the next iteration.

Comment 67

[(no longer active)]

Attachment: Part 1.1: Fix a test failing because of attachment 466746

The first patch in this bug caused a failure in <http://mxr.mozilla.org/mozilla-central/source/content/html/content/test/test_bug430392.html?force=1>:

TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug430392.html | adding and then deleting returns should not change text - got "\n    <span contenteditable=\"false\">A</span> ; <span contenteditable=\"false\">B</span> ;\n    <span contenteditable=\"false\">C</span>\n  ", expected "\n    <span contenteditable=\"false\">A</span> ;\n    <span contenteditable=\"false\">B</span> ;\n    <span contenteditable=\"false\">C</span>\n  "

I spent quite some time debugging this, here's the results.  Before this patch, this test didn't really test anything useful.  After the div is focused and a VK_RIGHT event is dispatched, nsFrameSelection::GetFrameForNodeOffset would be called with aNode set to the div element, and aOffset set to 0.  This would mean that later on the function would proceed to call GetPrimaryFrame() on the first child of the div which is a textnode consisting entirely of collapsed whitespace.  This would return null which would cause GetFrameForNodeOffset to fail, and the caret not to move at all.  So, when pressing Enter twice, the caret would be at the beginning of the editable area, which means that we were really testing something not related to bug 430392.

The first patch in this bug fixes the behavior of GetFrameForNodeOffset to try to grab the next frame if the textnode doesn't have a primary frame, which means that the caret movement to the right would actually succeed, and we would end up testing the right thing.  The reason that the test is failing is that it's expecting there to be a newline character after the semicolon, which is a misplaced expectation in that backspace doesn't provide undo semantics with regard to Enter, and we're not in a preformatted area, therefore pressing Enter would inject BR elements, and pressing backspace would remove them, which would cause the removal of the newline character (and also collapsing of the following space characters).  I think the best way to fix this test is to not include any collapsible whitespace inside the editable area.  This is what this patch does.

Comment 68

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

Comment on attachment 483605 [details] [diff] [review]
Part 3: Use a centralized algorithm for caret positioning

I guess there'll be another iteration of this one.

Comment 69

[(no longer active)]

(In reply to comment #60)
> Actually, I've got a better idea.
> 
> 0) call GetCaretBaseline on the caret frame as you have already
> 1) find the line that the caret frame is on
> 2) get the line's ascent (nsLineBox::GetAscent).
> 3) if the line's ascent is less than the caret's ascent (i.e. what we get from
> the font), the line is too short for the caret. Move the caret down by the
> difference in the ascents.
> 
> This basically means "if the line's baseline is too close to the top of the
> line to fit the caret, treat the line's baseline as if it had moved down enough
> to fit the caret".

I tried this approach, and it mostly gives acceptable results on <http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/503531-1.html?force=1>, but not quite.  The caret appears a little bit above where it appears in the reference file.  I haven't been able to track down the why so far, though.

(In reply to comment #61)
> Although it really depends on where we want the caret to be for empty lines.
> Sometimes it seems you'd want the empty line baseline as being the Y-coordinate
> of the line --- e.g. if it's the last line of a block that clips its contents.
> Sometimes you want the empty line baseline to be below the Y coordinate of the
> line --- e.g. if it's the first line of a block that clips its contents.

I don't get the latter case.  Why would we want the caret to be below the Y coordinate of the line in this case?

> If we can figure out what the desired behavior is, then I'm pretty sure we
> could implement it by extending the approach of comment #60.

One rule of thumb could be putting the caret somewhere so that it doesn't jump up and down when the user types of deletes text...

Comment 70

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

(In reply to comment #69)
> I don't get the latter case.  Why would we want the caret to be below the Y
> coordinate of the line in this case?

If the first line of a block that clips its contents is empty, its Y coordinate will be 0. Displaying the caret with its baseline at 0 means that the entire caret will be clipped out.

> One rule of thumb could be putting the caret somewhere so that it doesn't jump
> up and down when the user types of deletes text...

That is a very good point. That argues for moving the caret baseline down by the font ascent for empty lines ... if we can avoid moving it completely out of sight.

Comment 71

[(no longer active)]

(In reply to comment #70)
> (In reply to comment #69)
> > I don't get the latter case.  Why would we want the caret to be below the Y
> > coordinate of the line in this case?
> 
> If the first line of a block that clips its contents is empty, its Y coordinate
> will be 0. Displaying the caret with its baseline at 0 means that the entire
> caret will be clipped out.

Can you please give a sample test case of such a situation?

> > One rule of thumb could be putting the caret somewhere so that it doesn't jump
> > up and down when the user types of deletes text...
> 
> That is a very good point. That argues for moving the caret baseline down by
> the font ascent for empty lines ... if we can avoid moving it completely out of
> sight.

As in the case of a block which clips its contents and has an empty first line?

Comment 72

[(no longer active)]

Attachment: Part 3: Use a centralized algorithm for caret positioning

Actually, subtracting the difference of the caret frame's baseline with the line's centered font baseline seems to give the correct results for <http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/503531-1.html?force=1>.  Roc, what do you think about this approach?

Comment 73

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

(In reply to comment #71)
> (In reply to comment #70)
> > If the first line of a block that clips its contents is empty, its Y coordinate
> > will be 0. Displaying the caret with its baseline at 0 means that the entire
> > caret will be clipped out.
> 
> Can you please give a sample test case of such a situation?

<div style="overflow:hidden;">
  Hello
  <div id="d">
  </div>
</div>

Place the caret inside "d".

> As in the case of a block which clips its contents and has an empty first line?

That's a different case, but yeah, we want to do the same thing there.

The basic principle should be to show the caret where it would be if there was text at the caret. Then inserting more text there should not move the caret vertically (hopefully).

But we should also place the caret where it can be seen.

One possibility is to follow the first principle and ensure the caret can be seen by making clipping not apply to it.

Comment 74

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

(In reply to comment #72)
> Actually, subtracting the difference of the caret frame's baseline with the
> line's centered font baseline seems to give the correct results for
> <http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/503531-1.html?force=1>.
>  Roc, what do you think about this approach?

I'm not completely sure what you mean. Can I see the code? :-)

Comment 75

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

er whoops, there it is

Comment 76

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

+    nscoord lineAscent = line->GetAscent();
+    // Move the caret down by the difference of the caret baseline and the
+    // centered font baseline if the line is too short to fit the caret.
+    if (baseline > lineAscent) {
+      baseline += -lineAscent +
+        nsLayoutUtils::GetCenteredFontBaseline(fm,
+          nsHTMLReflowState::CalcLineHeight(aFrame->GetStyleContext(), NS_AUTOHEIGHT));

So I don't understand it. baseline never included lineAscent. This seems kinda random.

Comment 77

[(no longer active)]

Attachment: Diagram showing the reasoning behind my patch

(In reply to comment #76)
> +    nscoord lineAscent = line->GetAscent();
> +    // Move the caret down by the difference of the caret baseline and the
> +    // centered font baseline if the line is too short to fit the caret.
> +    if (baseline > lineAscent) {
> +      baseline += -lineAscent +
> +        nsLayoutUtils::GetCenteredFontBaseline(fm,
> +          nsHTMLReflowState::CalcLineHeight(aFrame->GetStyleContext(),
> NS_AUTOHEIGHT));
> 
> So I don't understand it. baseline never included lineAscent. This seems kinda
> random.

My understanding of lineAscent and centered-font-baseline might be wrong, but here's how I reached this formula.  Please correct me if I'm wrong.

Comment 78

[(no longer active)]

(This is where I got my impression about centered-font-baseline from BTW: <http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.h#871>)

Comment 79

[(no longer active)]

(In reply to comment #73)
> (In reply to comment #71)
> > (In reply to comment #70)
> > > If the first line of a block that clips its contents is empty, its Y coordinate
> > > will be 0. Displaying the caret with its baseline at 0 means that the entire
> > > caret will be clipped out.
> > 
> > Can you please give a sample test case of such a situation?
> 
> <div style="overflow:hidden;">
>   Hello
>   <div id="d">
>   </div>
> </div>
> 
> Place the caret inside "d".

OK, this testcase does that:

data:text/html,<div contenteditable style="overflow:hidden;">Hello<div id="d"></div></div><script>document.querySelector("div").focus();var sel=getSelection();sel.removeAllRanges();sel.collapse(document.getElementById("d"),0);</script>

So, our current behavior is that we do not display the caret, and the same thing happens with my patch as well.  Do we really want to display a caret there outside of the parent DIV?  I think the "display the caret where we will show text when it's typed" argument doesn't hold much water here because we will also redraw the selection border to include the second line when a character is typed.  On the other hand, I'm not sure if not displaying the caret is the best choice either...

This test case shows one reason why we don't want to display the caret outside of the parent div:

data:text/html,<div contenteditable style="overflow:hidden;">Hello<div id="d"></div></div><img src="http://mozcom-cdn.mozilla.net/img/tignish/home/feature-logo.png"><script>document.querySelector("div").focus();var sel=getSelection();sel.removeAllRanges();sel.collapse(document.getElementById("d"),0);</script>

> > As in the case of a block which clips its contents and has an empty first line?
> 
> That's a different case, but yeah, we want to do the same thing there.
> 
> The basic principle should be to show the caret where it would be if there was
> text at the caret. Then inserting more text there should not move the caret
> vertically (hopefully).
> 
> But we should also place the caret where it can be seen.
> 
> One possibility is to follow the first principle and ensure the caret can be
> seen by making clipping not apply to it.

What about the testcase above?

Comment 80

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

(In reply to comment #77)
> My understanding of lineAscent and centered-font-baseline might be wrong, but
> here's how I reached this formula.  Please correct me if I'm wrong.

I think lineAscent starts at the "top of line".

Comment 81

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

(In reply to comment #79)
> So, our current behavior is that we do not display the caret, and the same
> thing happens with my patch as well.  Do we really want to display a caret
> there outside of the parent DIV?  I think the "display the caret where we will
> show text when it's typed" argument doesn't hold much water here because we
> will also redraw the selection border to include the second line when a
> character is typed.  On the other hand, I'm not sure if not displaying the
> caret is the best choice either...

OK, I'm fine with not solving this problem in this bug :-).

> This test case shows one reason why we don't want to display the caret outside
> of the parent div:
> 
> data:text/html,<div contenteditable style="overflow:hidden;">Hello<div
> id="d"></div></div><img
> src="http://mozcom-cdn.mozilla.net/img/tignish/home/feature-logo.png"><script>document.querySelector("div").focus();var
> sel=getSelection();sel.removeAllRanges();sel.collapse(document.getElementById("d"),0);</script>

To be honest I'd be OK with displaying the caret in this case.

Comment 82

[(no longer active)]

(In reply to comment #81)
> (In reply to comment #79)
> > This test case shows one reason why we don't want to display the caret outside
> > of the parent div:
> > 
> > data:text/html,<div contenteditable style="overflow:hidden;">Hello<div
> > id="d"></div></div><img
> > src="http://mozcom-cdn.mozilla.net/img/tignish/home/feature-logo.png"><script>document.querySelector("div").focus();var
> > sel=getSelection();sel.removeAllRanges();sel.collapse(document.getElementById("d"),0);</script>
> 
> To be honest I'd be OK with displaying the caret in this case.

Hmm, on top of the image?

Should I file a more general bug on the question of a general algorithm on where to put the caret?

Comment 83

[(no longer active)]

(In reply to comment #80)
> (In reply to comment #77)
> > My understanding of lineAscent and centered-font-baseline might be wrong, but
> > here's how I reached this formula.  Please correct me if I'm wrong.
> 
> I think lineAscent starts at the "top of line".

Hmm, if that's the case, it should be equivalent to centered-font-baseline, which is clearly not the case for <http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/503531-1.html?force=1> at least.

Comment 84

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

(In reply to comment #82)
> Hmm, on top of the image?

Yes.

> Should I file a more general bug on the question of a general algorithm on
> where to put the caret?

This patch here is a general algorithm for positioning the caret. What we need is a better strategy for making the caret visible when it would otherwise not be. You can file a bug for that if you want.

(In reply to comment #83)
> Hmm, if that's the case, it should be equivalent to centered-font-baseline,
> which is clearly not the case for
> <http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/503531-1.html?force=1>
> at least.

In that example, I guess lineAscent is 0 and centered-font-baseline is nonzero. Right? The whole line is empty.

Comment 85

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The line's ascent should be the distance from the top of the line to the line's baseline.  (It's possible it might be wrong for lines that collapse to 0 height; I haven't checked.)  There might be text within the line whose baseline is at other positions other than the line's baseline, though (see 'vertical-align').

I don't yet understand what the patch is trying to do.

Comment 86

[(no longer active)]

(In reply to comment #84)
> > Should I file a more general bug on the question of a general algorithm on
> > where to put the caret?
> 
> This patch here is a general algorithm for positioning the caret. What we need
> is a better strategy for making the caret visible when it would otherwise not
> be. You can file a bug for that if you want.

Filed bug 607424 for that.

> (In reply to comment #83)
> > Hmm, if that's the case, it should be equivalent to centered-font-baseline,
> > which is clearly not the case for
> > <http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/503531-1.html?force=1>
> > at least.
> 
> In that example, I guess lineAscent is 0 and centered-font-baseline is nonzero.
> Right? The whole line is empty.

Yes.  So in this case, we can't position the caret correctly by relying on lineAscent.  One other possibility that comes to my mind is something like this:

if (baseline > lineAscent) {
  if (lineAscent == 0) {
    baseline += centeredFontBaseline;
  } else {
    baseline += baseline - lineAscent;
  }
}

Would that be a better adjustment?

Comment 87

[(no longer active)]

(In reply to comment #85)
> The line's ascent should be the distance from the top of the line to the line's
> baseline.  (It's possible it might be wrong for lines that collapse to 0
> height; I haven't checked.)  There might be text within the line whose baseline
> is at other positions other than the line's baseline, though (see
> 'vertical-align').

That shouldn't be a concern for 0-height lines, right?

Comment 88

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

How about setting baseline to lineBox.y + NS_MAX(lineAscent, nsLayoutUtils::GetCenteredFontBaseline())?

Comment 89

[(no longer active)]

Attachment: Part 3: Use a centralized algorithm for caret positioning

(In reply to comment #88)
> How about setting baseline to lineBox.y + NS_MAX(lineAscent,
> nsLayoutUtils::GetCenteredFontBaseline())?

We can't use line->mBounds.y because we adjust the y position based on the parent scroll frame later on, but I think this is what you meant.

Comment 90

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

+  if (line) {
+    // Get the line's ascent
+    nscoord lineAscent = line->GetAscent();
+    // Move the caret down by the difference of the caret baseline and the
+    // centered font baseline if the line is too short to fit the caret.
+    if (baseline > lineAscent) {
+      baseline += NS_MAX(lineAscent,
+        nsLayoutUtils::GetCenteredFontBaseline(fm,
+          nsHTMLReflowState::CalcLineHeight(aFrame->GetStyleContext(), NS_AUTOHEIGHT)));

Comparing baseline to lineAscent can't be right since baseline is relative to aFrame and lineAscent is relative to the line mBounds.y.

I think the clearest way to think about this is that we want the top of the caret to be no higher than the top of the line. So:
-- compute the current top of the caret: baseline - ascent + aFrame->GetOffsetTo(block containing the line)
-- compute the top of the line: line.mBounds.y
-- take the max
-- set framePos.y to the result

Comment 91

[(no longer active)]

I tried this approach, and it doesn't work correctly on <http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/503531-1.html>.  the empty line's top actually intersects with the line containing "BLOCK", therefore the caret's top appears inside the above line...  This may be a layout bug, I need to investigate it more I think.

Comment 92

[(no longer active)]

Based on comment 91, and my conversation with roc, I'll spend some time to test this patch to make sure that it doesn't regress any other caret functionality besides that reftest, and once I can confirm that, I think we should land this patch on trunk as soon as possible, and disable this reftest with that landing, and file a new (perhaps blocking) bug to investigate the failure in the reftest and re-enable it once the reason for that failure is known.

I'll report the results of my testing here soon.

Comment 93

[(no longer active)]

Note to myself: what I tried in comment 91 for future reference: http://pastebin.mozilla.org/827531

Comment 94

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

The reftest clearly exposes a layout bug; the word "BLOCK" should appear on the first line, not the second --- and typing a character should not cause it to jump to the first line! So we should file a new bug for that and disable the test for now.

Comment 95

[(no longer active)]

(In reply to comment #94)
> The reftest clearly exposes a layout bug; the word "BLOCK" should appear on the
> first line, not the second

That's not the layout bug in the test.  "BLOCK" is displayed on the second line because of a br node added because of the contenteditable attribute.  The layout bug is the incorrect top value for the 2nd line.  I've filed bug 607548 on that, with test cases attached to demonstrate the problem.

> --- and typing a character should not cause it to
> jump to the first line!

The reason that typing a single character moves the text empty text frame down is that after the reflow, the y position of the line gets corrected.

> So we should file a new bug for that and disable the
> test for now.

Agreed.  -> bug 607548.

Comment 96

[(no longer active)]

Attachment: Part 3: Use a centralized algorithm for caret positioning

Comment 97

[(no longer active)]

Attachment: Part 3.1: Disable the reftest

Comment 98

[(no longer active)]

Attachment: Part 4: Add a reftest for caret height in block elements

(See comment 42)

Comment 99

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

Hmm, maybe a stricter test would be to have the reference be a contenteditable div containing a  ? Then we'd be testing that the caret looks the same in an empty vs nonempty block, which is really what we want.

Comment 100

[(no longer active)]

(In reply to comment #99)
> Hmm, maybe a stricter test would be to have the reference be a contenteditable
> div containing a  ? Then we'd be testing that the caret looks the same in
> an empty vs nonempty block, which is really what we want.

That's a different test.  In this reftest I wanted to make sure that the caret doesn't get resized to the height of the block element for empty blocks.  I'll add another test for the empty vs non-empty block case as well.

Comment 101

[(no longer active)]

(In reply to comment #59)
> I guess that makes sense. we add the ascent to the frameY to figure out where
> to position the caret.
> 
> I guess the reason that setting the ascent didn't work is that
> nsLineLayout::VerticalAlignFrames does this:
> 
>           case NS_STYLE_VERTICAL_ALIGN_BASELINE:
>             // The elements baseline is aligned with the baseline of
>             // the parent.
>             pfd->mBounds.y = baselineY - pfd->mAscent;
>             pfd->mVerticalAlign = VALIGN_OTHER;
> 
> I.e., it's aligning the baseline of the frame with the baseline of the line,
> and the baseline of the line is at 0.

So, I tested this theory, and I don't think that this is what's happening.  I'm not a layout guru, but my investigations so far show that this is what's different between attachment 467483 [details] and attachment 467549 [details]: <http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsLineLayout.cpp#1553>.  The comments seem to be describing the exact same situation, too.  Unfortunately I don't see how we can fix this except by changing the line layout behavior there, which may break other stuff too.  The only other option would be to special case caret positioning for this sort of situation.

> I'm not sure of the best way to fix this. We don't really want to mess with
> line layout here. How about we leave the textframe ascent alone and make
> nsTextFrame::GetCaretBaseline just return fontMetrics->GetMaxAscent always? And
> ditto for nsInlineFrame?

FWIW, GetCaretBaseline returns the correct values for the text and inline frames in question.  it's their position which is wrong...

Comment 102

[(no longer active)]

Attachment: Part 4: Add a reftest for caret height in block elements and empty/non-empty block elements

More reftests.  I also reduced the 300px height in the previous test to 50px, because the test would otherwise show a scrollbar, and fail because of that.

Comment 103

[(no longer active)]

BTW, roc, if you think that the layout issue in attachment 467483 [details] doesn't need to be addressed in this bug, this set of patches are ready for landing, as far as I can tell.

Comment 104

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

I think we should go ahead and land.

Comment 105

[(no longer active)]

OK, I'll land this when the tree reopens.

Comment 106

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/acd9dc85ca3f
http://hg.mozilla.org/mozilla-central/rev/8389e11781ed
http://hg.mozilla.org/mozilla-central/rev/5d3216e3857f
http://hg.mozilla.org/mozilla-central/rev/ed0befc22bb7
http://hg.mozilla.org/mozilla-central/rev/38e63e07395a
http://hg.mozilla.org/mozilla-central/rev/217e60535a77

Comment 107

[(no longer active)]

[e-mail]: it would be really helpful if you can test tonight's Firefox nightly to see if the issues you've been seeing with Google Spreadsheets are now fixed.

Comment 108

[bmccann]

Awesome.  Thanks so much.  You all really stuck with this bug!  I'll make sure it gets tested.

Comment 109

[(no longer active)]

(In reply to comment #108)
> Awesome.  Thanks so much.  You all really stuck with this bug!  I'll make sure
> it gets tested.

Great!  Please do comminicate back with us on any remaining issues, so that we can make sure to fix them in time for Firefox 4.