[webkit-reviews] review denied: [Bug 10123] when CSS pseudo selectors are applied (:before and :after) the *-of-line keyboard navigation does not work : [Attachment 109760] Update on patch 109275

Mon Oct 10 22:14:23 PDT 2011

Ryosuke Niwa <rniwa at webkit.org> has denied roseN <rosen.dash at motorola.com>'s
request for review:
Bug 10123: when CSS pseudo selectors are applied (:before and :after) the
*-of-line keyboard navigation does not work
https://bugs.webkit.org/show_bug.cgi?id=10123

Attachment 109760: Update on patch 109275
https://bugs.webkit.org/attachment.cgi?id=109760&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=109760&action=review

r- because of various issues listed below.

> LayoutTests/ChangeLog:9
> +	   * editing/selection/css-pseudo-element-expected.txt: Added.
> +	   * editing/selection/css-pseudo-element.html: Added.

Since this bug is about falling into an infinite loop, I feel like we need
another test where you just move across a bunch of generated contents from left
to right and from right to left.

> LayoutTests/editing/selection/css-pseudo-element.html:8
> +  <head>
> +    <style type="text/css"> /* pertinent to test cases */
> +	 .quote:before { content: "\""; }
> +	 .quote:after { content: "\""; }
> +    </style>
> +  </head>

No need to indent elements like this.

> LayoutTests/editing/selection/css-pseudo-element.html:24
> +	var li = document.createElement("li");

There's a tab here.

> LayoutTests/editing/selection/css-pseudo-element.html:35
> +	 function assert(bool) {
> +	if (!bool)
> +	  log("Failure");
> +	else
> +	  log("Success");
> +	 }

Please convert this into a real script test using
Tools/Scripts/make-new-script-test.

> LayoutTests/editing/selection/css-pseudo-element.html:44
> +	 window.getSelection().setPosition(edit, 2);
> +	 window.getSelection().modify('move', 'right', 'character');
> +	 assert(window.getSelection().anchorOffset == 1);

You certainly need to test the case where you move the caret to the left.

> Source/WebCore/ChangeLog:1
> +2011-10-05  roseN Dash  <rosen.dash at motorola.com>

I'm sorry I might be ignorant here but is this the correct capitalization of
your name? Contribution guide mandates that you use your real full name.

>>> Source/WebCore/editing/VisiblePosition.cpp:235
>>> +	     if ((p.isCandidate() && p.downstream() != downstreamStart) ||
p.atStartOfTree() || p.atEndOfTree() || p == m_deepPosition)
>> 
>> I don't understand why you return p when p == m_deepPosition. Same below in
rightVisuallyDistinctCandidate. Could you please clarify?
> 
> 

>The outer while loop in leftVisuallyDistinctCandidate() or
rightVisuallyDistinctCandidate() depends only on value of p. If we enter into
this loop without changing p, this leads into an infinite loop case. This
condition appears when we try to move beyond a CSS pseudoelement (like before
or after) without any text in its parent node. To prevent these cases we have
to check whether the value of p is other than m_deepPosition before entering
into this loop again.

We should be checking for that particular condition not that p is equal to
m_deepPosition.