Bug 118897 – Minor refactoring in VisibleUnits.cpp for code pertaining to caret movement between lines.

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

[Arpita Bahuguna]

Minor refactoring required in code pertaining to moving in between lines; specifically for the "up" and "down" caret movement.

[Arpita Bahuguna]

Created attachment 207079 [details]
Patch

[Build Bot]

Comment on attachment 207079 [details]
Patch

Attachment 207079 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1110869

New failing tests:
editing/selection/move-by-word-visually-mac.html

[Build Bot]

Created attachment 207089 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[Arpita Bahuguna]

editing/selection/move-by-word-visually-mac.html test seems to be failing even without this patch.

[Build Bot]

Comment on attachment 207079 [details]
Patch

Attachment 207079 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1099872

New failing tests:
editing/selection/move-by-word-visually-mac.html

[Build Bot]

Created attachment 207096 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

[Build Bot]

Comment on attachment 207079 [details]
Patch

Attachment 207079 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1249439

New failing tests:
dom/xhtml/level1/core/documentinvalidcharacterexceptioncreateentref1.xhtml
dom/svg/level3/xpath/XPathEvaluator_evaluate_INVALID_EXPRESSION_ERR.svg
dom/html/level2/events/dispatchEvent04.html
dom/html/level2/html/HTMLSelectElement20.html
dom/svg/level3/xpath/XPathEvaluator_createExpression_INVALID_EXPRESSION_ERR.svg
dom/svg/level3/xpath/XPathEvaluator_createExpression_NAMESPACE_ERR_02.svg
dom/html/level1/core/documentinvalidcharacterexceptioncreateentref.html
dom/html/level2/core/hc_namednodemapinvalidtype1.html
dom/svg/level3/xpath/XPathEvaluator_evaluate_NAMESPACE_ERR.svg
dom/html/level2/events/dispatchEvent01.html
dom/xhtml/level1/core/hc_attrappendchild4.xhtml
dom/html/level2/events/dispatchEvent03.html
dom/html/level2/events/dispatchEvent02.html
dom/html/level2/core/createDocumentType04.html
dom/html/level1/core/documentinvalidcharacterexceptioncreateentref1.html
dom/html/level1/core/documentinvalidcharacterexceptioncreatepi1.html
dom/html/level2/events/dispatchEvent06.html
css1/basic/comments.html
dom/xhtml/level1/core/documentinvalidcharacterexceptioncreatepi1.xhtml
dom/html/level2/events/dispatchEvent07.html
dom/html/level2/core/setAttributeNS10.html
dom/html/level2/events/dispatchEvent05.html
dom/html/level1/core/hc_attrappendchild2.html
dom/html/level2/core/createAttributeNS06.html
dom/html/level1/core/hc_attrappendchild4.html
dom/xhtml/level1/core/documentinvalidcharacterexceptioncreatepi.xhtml
dom/svg/level3/xpath/XPathEvaluator_createExpression_NAMESPACE_ERR_01.svg
dom/xhtml/level1/core/documentinvalidcharacterexceptioncreateentref.xhtml
dom/xhtml/level1/core/hc_attrappendchild2.xhtml
dom/html/level1/core/documentinvalidcharacterexceptioncreatepi.html

[Build Bot]

Created attachment 207581 [details]
Archive of layout-test-results from APPLE-EWS-5 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: APPLE-EWS-5  Port: win-future  Platform: CYGWIN_NT-6.1-WOW64-1.7.20-0.266-5-3-i686-32bit

[Ryosuke Niwa]

Comment on attachment 207079 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=207079&action=review

> Source/WebCore/editing/VisibleUnits.cpp:83
> -    while (previousNode && inSameLine(firstPositionInOrBeforeNode(previousNode), visiblePosition))
> +    while (previousNode && inSameLine(lastPositionInOrAfterNode(previousNode), visiblePosition))

This definitely changes the behavior.

> Source/WebCore/editing/VisibleUnits.cpp:87
> -        if (highestEditableRoot(firstPositionInOrBeforeNode(previousNode), editableType) != highestRoot)
> +        if (highestEditableRoot(lastPositionInOrAfterNode(previousNode), editableType) != highestRoot)

Ditto.

[Arpita Bahuguna]

Created attachment 207850 [details]
Patch

[Arpita Bahuguna]

(In reply to comment #9)
> (From update of attachment 207079 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=207079&action=review
> 
> > Source/WebCore/editing/VisibleUnits.cpp:83
> > -    while (previousNode && inSameLine(firstPositionInOrBeforeNode(previousNode), visiblePosition))
> > +    while (previousNode && inSameLine(lastPositionInOrAfterNode(previousNode), visiblePosition))
> 
> This definitely changes the behavior.
> 
> > Source/WebCore/editing/VisibleUnits.cpp:87
> > -        if (highestEditableRoot(firstPositionInOrBeforeNode(previousNode), editableType) != highestRoot)
> > +        if (highestEditableRoot(lastPositionInOrAfterNode(previousNode), editableType) != highestRoot)
> 
> Ditto.

Agree! I naively verified only the editing layout testcases.

[Ryosuke Niwa]

Comment on attachment 207850 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=207850&action=review

> Source/WebCore/ChangeLog:10
> +        No new tests required as there is no change in functionality.

This is not true.

> Source/WebCore/editing/VisibleUnits.cpp:951
> +        Node* child = node->childNode(p.deprecatedEditingOffset());
> +        node = child ? child : node->firstDescendant();

Again, this changes the behavior.

[Arpita Bahuguna]

(In reply to comment #12)
> (From update of attachment 207850 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=207850&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        No new tests required as there is no change in functionality.
> 
> This is not true.
> 
> > Source/WebCore/editing/VisibleUnits.cpp:951
> > +        Node* child = node->childNode(p.deprecatedEditingOffset());
> > +        node = child ? child : node->firstDescendant();
> 
> Again, this changes the behavior.

Hi rniwa, i tried to come up with some testcases for verifying this change but was unable to come up with a case that would actually give some different output.

With or without this change I get the same result, at least for the testcases that I tried with, using contenteditable/non-contenteditable lines.

Going by that, is this change really required, or shall we drop it?

[Arpita Bahuguna]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 207850 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=207850&action=review
> > 
> > > Source/WebCore/ChangeLog:10
> > > +        No new tests required as there is no change in functionality.
> > 
> > This is not true.
> > 
> > > Source/WebCore/editing/VisibleUnits.cpp:951
> > > +        Node* child = node->childNode(p.deprecatedEditingOffset());
> > > +        node = child ? child : node->firstDescendant();
> > 
> > Again, this changes the behavior.
> 
> Hi rniwa, i tried to come up with some testcases for verifying this change but was unable to come up with a case that would actually give some different output.
> 
> With or without this change I get the same result, at least for the testcases that I tried with, using contenteditable/non-contenteditable lines.
> 
> Going by that, is this change really required, or shall we drop it?

Probably I need to come up with a testcase wherein the childNode is different based on the offset.

[Ryosuke Niwa]

I'd like to know why we're making this change at least. It's certainly not just a cleanup and it DOES change the behavior.

[Ryosuke Niwa]

Comment on attachment 207850 [details]
Patch

r- since there is a behavior change even though the change log claims otherwise.

FYI, that particular code was added in http://trac.webkit.org/changeset/77521. Maybe we'll be able to come up with a test case if we dug further.

[Arpita Bahuguna]

(In reply to comment #16)
> (From update of attachment 207850 [details])
> r- since there is a behavior change even though the change log claims otherwise.
> 
> FYI, that particular code was added in http://trac.webkit.org/changeset/77521. Maybe we'll be able to come up with a test case if we dug further.

I did manage to get a case wherein the change in behavior is visible using the testcase mentioned in http://trac.webkit.org/changeset/77521.
However, am afraid, this particular change is not proper, at least for the following testcase:

If we have two text lines such as:

<p>This is some long text.<i>Hello</i> This is some long long text. This is some long long text.</p>
<table><tr><td><span></span>Cursor should be at the end</td></tr></table>

Place the cursor at the end of the of second line i.e. "Cursor should be at the end".
Now trying moving back a line by using the up arrow key.

Currently the caret will land on the previous line just above its current position.
However, with (change in prevLinePosition()):

Node* child = node->childNode(p.deprecatedEditingOffset());
node = child ? child : node->firstDescendant();


The node comes as HTMLTableElement whose childNode for the given offset is returned as NULL. Thus node->firstDescendant() is called upon which now returns the <span> element instead of the Text node.
Thus, when obtaining the previousLeafNode in previousRootInlineBoxCandidatePosition() the computation differs and now the caret shall not be placed directly above the current position but further ahead.

I can't think of another scenario wherein this change shall bring some improvement to our current behavior thus I request closing of this issue.
Sorry for wasting time. :(

Perhaps though the FIXME in nextLinePosition() regarding this change can be removed. (?)

[Ryosuke Niwa]

It would be definitely good to add a comment if we believe we have to have a different code in those two functions.

[Arpita Bahuguna]

(In reply to comment #18)
> It would be definitely good to add a comment if we believe we have to have a different code in those two functions.

Hi rniwa,

My previous comment (#17) was actually incorrect in that there really is no change in behavior if we were to add the following piece of code:

Node* child = node->childNode(p.deprecatedEditingOffset());
node = child ? child : node->firstDescendant();

to previousLinePosition().

Even for the example I gave in comment #17, there is no change in behavior. At the time of verification I had some small changes in previousRootInlineBoxCandidatePosition() which were actually causing the change in behavior and which I had forgotten to remove. :(

Sorry for the confusion.

However, am still unable to come up with a testcase which might ratify the inclusion of the aforementioned change. :(

[Ahmad Saleem]

If I am not mistaken, the FIXME is still present in Webkit Source, which this bug was trying to resolve:

https://github.com/WebKit/WebKit/blob/main/Source/WebCore/editing/VisibleUnits.cpp#L1034

[e-mail] - is it something valid or need to be fixed? Thanks!

[Ryosuke Niwa]

Yeah, this still needs fixing.

[Ryosuke Niwa]

However, I don't think there is much point in reusing this bug at this point. -> Later.