Bug 8182 - some text-field-related layout tests are failing
Summary: some text-field-related layout tests are failing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P1 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-04-04 10:35 PDT by Darin Adler
Modified: 2006-04-04 20:53 PDT (History)
0 users

See Also:


Attachments
fixes in WebCore, layout tests updates in LayoutTests (17.96 KB, patch)
2006-04-04 10:40 PDT, Darin Adler
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2006-04-04 10:35:16 PDT
A few text-field-related layout tests are failing. I have fixes.
Comment 1 Darin Adler 2006-04-04 10:40:45 PDT
Created attachment 7504 [details]
fixes in WebCore, layout tests updates in LayoutTests
Comment 2 Justin Garcia 2006-04-04 14:42:34 PDT
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? 
Comment 3 Justin Garcia 2006-04-04 14:51:21 PDT
> @@ -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.
Comment 4 Darin Adler 2006-04-04 14:53:23 PDT
(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.
Comment 5 Darin Adler 2006-04-04 15:02:39 PDT
Comment on attachment 7504 [details]
fixes in WebCore, layout tests updates in LayoutTests

Justin reviewed the editing parts, Adele reviewed the other parts.
Comment 6 Darin Adler 2006-04-04 20:53:25 PDT
This ended up fixing only one test.