[Webkit-unassigned] [Bug 10123] when CSS pseudo selectors are applied (:before and :after) the *-of-line keyboard navigation does not work

Mon Oct 10 22:14:23 PDT 2011

https://bugs.webkit.org/show_bug.cgi?id=10123

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #109760|review?                     |review-
               Flag|                            |

--- Comment #13 from Ryosuke Niwa <rniwa at webkit.org>  2011-10-10 22:14:23 PST ---
(From update of attachment 109760)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.