Bug 8182 – some text-field-related layout tests are failing

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Darin Adler]

A few text-field-related layout tests are failing. I have fixes.

[Darin Adler]

Created attachment 7504 [details]
fixes in WebCore, layout tests updates in LayoutTests

[Justin Garcia]

Index: WebCore/editing/InsertTextCommand.cpp
===================================================================
--- WebCore/editing/InsertTextCommand.cpp	(revision 13680)
+++ WebCore/editing/InsertTextCommand.cpp	(working copy)
@@ -94,6 +94,9 @@
     assert(text.find('\n') == -1);
 
     Selection selection = endingSelection();
+    // FIXME: I don't see how "start of line" could possibly be the right check here.
+    // It depends on line breaks which depends on the way things are current laid out,
+    // window width, etc. This needs to be rethought.
     bool adjustDownstream = isStartOfLine(VisiblePosition(selection.start().downstream(), DOWNSTREAM));

I'm not sure that this FIXME is correct.  Code below tries to prevent inserted text from uncollapsing collapsed whitespace.  It does that by always putting the inserted text at the upstream() version of the caret unless the caret was at the start of a line, and in that case it puts it at the downstream() version of the caret.  This happens to work, but it's a confusing way to do it.  I think using isStartOfParagraph would break the case where there is collapsed whitespace just after a line wrap (at the beginning of the next line).

@@ -622,7 +622,7 @@
         downstream = positionAvoidingSpecialElementBoundary(downstream);
         if (downstream.node()->hasTagName(brTag) && downstream.offset() == 0 && 
             fragment.hasInterchangeNewlineAtEnd() &&
-            isStartOfLine(VisiblePosition(downstream, VP_DEFAULT_AFFINITY)))
+            isStartOfParagraph(VisiblePosition(downstream, VP_DEFAULT_AFFINITY)))
             linePlaceholder = downstream.node();

isStartOfLine(v) and isStartOfParagraph(v) only differ if v is just after a line-wrap.  A br can never be just after a line-wrap, so I don't see how this change would effect behavior.  Does it?  In any case, isStartOfParagraph is fine.

@@ -634,14 +634,14 @@
     if (m_smartReplace) {
         VisiblePosition visiblePos = VisiblePosition(startPos, VP_DEFAULT_AFFINITY);
         assert(visiblePos.isNotNull());
-        addLeadingSpace = startPos.leadingWhitespacePosition(VP_DEFAULT_AFFINITY, true).isNull() && !isStartOfLine(visiblePos);
+        addLeadingSpace = startPos.leadingWhitespacePosition(VP_DEFAULT_AFFINITY, true).isNull() && !isStartOfParagraph(visiblePos);

!isStartOfParagraph is true more than !isStartOfLine, so how does this change _avoid_ adding extra spaces?    Was it just that isStartOfLine was returning false erroneously? 

[Justin Garcia]

> @@ -634,14 +634,14 @@
>      if (m_smartReplace) {
>          VisiblePosition visiblePos = VisiblePosition(startPos,
> VP_DEFAULT_AFFINITY);
>          assert(visiblePos.isNotNull());
> -        addLeadingSpace =
> startPos.leadingWhitespacePosition(VP_DEFAULT_AFFINITY, true).isNull() &&
> !isStartOfLine(visiblePos);
> +        addLeadingSpace =
> startPos.leadingWhitespacePosition(VP_DEFAULT_AFFINITY, true).isNull() &&
> !isStartOfParagraph(visiblePos);
> 
> !isStartOfParagraph is true more than !isStartOfLine, so how does this change
> _avoid_ adding extra spaces?    Was it just that isStartOfLine was returning
> false erroneously? 

I should make it clear that I think that this change and the others are correct, except for perhaps the fixme, which I'm still fuzzy on.  so r=me on the editing changes.

[Darin Adler]

(In reply to comment #2)
> Index: WebCore/editing/InsertTextCommand.cpp
> ===================================================================
> --- WebCore/editing/InsertTextCommand.cpp       (revision 13680)
> +++ WebCore/editing/InsertTextCommand.cpp       (working copy)
> @@ -94,6 +94,9 @@
>      assert(text.find('\n') == -1);
> 
>      Selection selection = endingSelection();
> +    // FIXME: I don't see how "start of line" could possibly be the right
> check here.
> +    // It depends on line breaks which depends on the way things are current
> laid out,
> +    // window width, etc. This needs to be rethought.
>      bool adjustDownstream =
> isStartOfLine(VisiblePosition(selection.start().downstream(), DOWNSTREAM));
> 
> I'm not sure that this FIXME is correct.  Code below tries to prevent inserted
> text from uncollapsing collapsed whitespace.  It does that by always putting
> the inserted text at the upstream() version of the caret unless the caret was
> at the start of a line, and in that case it puts it at the downstream() version
> of the caret.  This happens to work, but it's a confusing way to do it.  I
> think using isStartOfParagraph would break the case where there is collapsed
> whitespace just after a line wrap (at the beginning of the next line).

I'm not suggesting using isStartOfParagraph. What I'm suggesting is that isStartOfLine is not correct to check here, because something that depends on the current position of line breaks seemingly can't possibly be correct.

> isStartOfLine(v) and isStartOfParagraph(v) only differ if v is just after a
> line-wrap.  A br can never be just after a line-wrap, so I don't see how this
> change would effect behavior.  Does it?

It doesn't.

> @@ -634,14 +634,14 @@
>      if (m_smartReplace) {
>          VisiblePosition visiblePos = VisiblePosition(startPos,
> VP_DEFAULT_AFFINITY);
>          assert(visiblePos.isNotNull());
> -        addLeadingSpace =
> startPos.leadingWhitespacePosition(VP_DEFAULT_AFFINITY, true).isNull() &&
> !isStartOfLine(visiblePos);
> +        addLeadingSpace =
> startPos.leadingWhitespacePosition(VP_DEFAULT_AFFINITY, true).isNull() &&
> !isStartOfParagraph(visiblePos);
> 
> !isStartOfParagraph is true more than !isStartOfLine, so how does this change
> _avoid_ adding extra spaces?    Was it just that isStartOfLine was returning
> false erroneously? 

Yes. isStartOfLine erroneously returns false in the case where there is no line at all -- an empty text field is such a case.

I could try to fix that by changing the behavior isStartOfLine, but instead I thought it was best to first remove calls to the line-related functions in places where they were clearly not the right ones to call.

[Darin Adler]

Comment on attachment 7504 [details]
fixes in WebCore, layout tests updates in LayoutTests

Justin reviewed the editing parts, Adele reviewed the other parts.

[Darin Adler]

This ended up fixing only one test.