Bug 8333

This problem is quite serious, as it makes even selection in bugzilla (and highlighting of Find results) look awful. It's not just an editing issue.

The newlines must be rendered. Here's an example that illustrates how the newline needs to be considered, since it could affect the height of the blank line! <pre>One<span style="font-size:72px"> Two</span</pre> Our code that does the "skip" doesn't use the style of the newline. It uses the line-height of the block. The newline needs to receive a box so that line boxes will be constructed for all the intermediate inline flows that could wrap that newline.

I think that as much as possible we need to unify the code for <br> and preserved newlines, giving them the same kind of box.

*** Bug 6432 has been marked as a duplicate of this bug. ***

Created attachment 7664 [details] Patch that includes newlines in the text boxes. So here's an initial interesting cut. All I did was made newlines get included as part of the line. The break will be incremented to move past the newline (similar to what we do with <br>s). The reason I find this interesting is that if we patch text rendering to know that it should ignore newlines from "preserveNewline" white-space, then I think this may just work.

I think I want to keep hacks out of the text renderer though, so I'm going to try using the midpoint stuff to separate single newline characters into self-contained runs. That way I can special case the boxes that they make (like brs do).

Created attachment 7683 [details] Getting closer. This patch gets newlines working as separate boxes and adds an isLineBreak function to line boxes that is true for <br> boxes or for newline boxes. It also gets left/right caret navigation working properly. Up/down caret navigation is still finding the wrong visible position though.

Created attachment 7690 [details] Almost. Fail one test. This patch is pretty complete. I fail only one layout test. The problem is I don't really understand the code behind the test (it's about merging of blocks). editing/deleting/merge-whitespace-pre.html

Created attachment 7696 [details] Patch for review. Here is a patch for review. This patch also turns off all of the layout test hacks (and so it changes every single layout test in the tree). I should make a general comment about the merge-whitespace-pre.html test. After spending about 5 hours on the problem, I have decided to go ahead and check in updated results for the test. The current rendering is already buggy (the layout test is demonstrating an unfixed bug). My patch does make the rendering worse, but the reason it does is that much of the editing code simply doesn't know how to deal properly with preserved newlines yet. The deletion issue has to do with the fact that a placeholder doesn't get made once newlines are rendered positions and so you hit code that already has a FIXME... // FIXME: Deletion has bugs and it doesn't always add a placeholder. If it fails, still do pruning. else prune(placeholder); The layout test in question is already badly broken. If you hit enter in newline-preserving content (like white-space: pre) we are doubling the spacing between lines. I think the code is so confused that it is splitting the <pre> block (which has margins on it by default). Deletion that crosses white-space boundaries also doesn't handle the fact that line breaks are different (<br>s in some cases and preserved newlines in others). This has to be dealt with when merging blocks. Many of the places in the editing code that deal with <br>s simply don't know what to make of preserved newlines. This was true both before and after my patch. The upshot is that editing of preformatted text is in a sorry state. :(

Comment on attachment 7696 [details] Patch for review. Made some comments over irc. I suggest you find a way to turn this into an static inline function: + if (!stoppedIgnoringSpaces && pos > 0) { + // We need to stop right before the newline and then start up again. + BidiIterator midpoint(0, o, pos); + addMidpoint(BidiIterator(0, o, pos-1)); // Stop + addMidpoint(BidiIterator(0, o, pos)); // Start + } Also, there is no way for me to address the question of whether your test changes here are sane or not, so I'll just have to trust you there. You might still want someone else more familiar with this code to review this.

Fixed.