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

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

[Robert Burns]

apply a style declaration through the style element.

p:before {content: "“";} p.after {content: "”";}. This will not allow command-right-arrow and command-left-arrow to select the end and beginning of the line. Also when using up-arrow to move up towards the first line the pseudo element will block the selection from entering the first line.

[Mark Rowe (bdash)]

Robert, can you please attach a simple HTML file that will more easily demonstrate the issue?

[Robert Burns]

Created attachment 10651 [details]
for openning in blot.app to test keyboard navigation

Here's the requested sample.

[Joseph Holsten]

Why do you expect command-right-arrow to select to the beginning of the line? I would only expect that in a textarea or a text input. I'm also dumbfounded by all the metadata in your sample page. Does that actually play a role in reproducing this bug?
Seems like not a bug to me.

[Robert Burns]

Created attachment 46179 [details]
A better example that works in browser (no blot.app required)

My apologies for all of that superfluous metadata. I included that inadvertantly.

The behavior appears different in the latest Safari WebKit. In general the existence of generated content still hinders keybaord navigation. The up/down navigation works better, but is still not right. A down-arrow key event should always move to the next line whereas depending on where the generated content is, it could move to later in the same line.

Sometimes the keyboard navigation fails entirely to cross the element boundary (especially with left and right arrow keydowns). In that case the user can repeatedly depress the left arrow and the cursor remains in the same place.

This bug does not really affect textarea and input elements but is instead related to content editable and related elements (whereas the textarea and input are strictly text/plain).

Perhaps this bug is fixed and a new bug needs to be opened for the remaining keyboard navigation problems. In terms of start-of-line and end-of-line the major problem appears to be fixed. I think the behavior is still wrong in terms of the left and right arrows should not allow the curor to end up on the previous line or next line respectively (for ltr text). Instead in the event of a start-of-line action, the curor should end up on the same line and after any line-wrapping generated content. In the case of moving the selection to the start-of-line the portion of the generated content itself should be selected on the same line. Though I imagine this would require a change in the design since there would be a selectedDOMRange that diverges from the NSRange selection in the view class.

Perhaps an umbrella bug for keyboard navigation/selection with sub bugs for each specific problem. Other related keyboard navigation and selection issues (other than this one regarding generated content):

1) paragraph (shift-option-arrow) selection does not work as it does in NSTextView, but instead does something like selecting to the same place in the next paragraph (rather than to the beginning or end of the current paragraph).
2) the issue you raise where extending selections using the keyboard should work even without contentEditable=true. It works for shift-option-arrow but not for shif-command-arrow.

There may be others, but those are the big ones I can think of now.

[Rosen Dash]

Refer to this bug reported in chromium.
http://code.google.com/p/chromium/issues/detail?id=98247

This is a serious bug as the page hangs the browser consuming more than 100% cpu.

[Rosen Dash]

Created attachment 109275 [details]
This patch solves issues regarding navigation and infinite loop cases with CSS pseudo elements before/after .

This patch addresses folllowing two issues:

 1. In case of using CSS pseudo elements before/after with content attribute containing single character at the start/end of text and there
    is no text in parent node, when we try to move cursor beyond the inserted character using left/right arrow key, the page freezes
    falling into an infinite loop.

 2. When these elements try to insert some text between a text line, navigation by right arrow key is prohibited.

For better understanding please use the following test case.
http://chromium.googlecode.com/issues/attachment?aid=982470003000&name=crash_test_set.html&token=426675a18b21d857fd199be14c019274

[Rosen Dash]

This bug is verified with the following test case in GtkLauncher, chromium on ubuntu 11.04 and safari on Mac.

http://chromium.googlecode.com/issues/attachment?aid=982470003000&name=crash_test_set.html&token=426675a18b21d857fd199be14c019274

(In reply to comment #3)
> Why do you expect command-right-arrow to select to the beginning of the line? I would only expect that in a textarea or a text input. I'm also dumbfounded by all the metadata in your sample page. Does that actually play a role in reproducing this bug?
> Seems like not a bug to me.

[Ryosuke Niwa]

Comment on attachment 109275 [details]
This patch solves issues regarding navigation and infinite loop cases with CSS pseudo elements before/after .

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

> Source/WebCore/ChangeLog:14
> +        No new tests as this fixes internal implementation detail.

This change definitely needs a test. From what I understand, keyboard navigation can be tested by getSelection().modify. r- because of this.

[Ryosuke Niwa]

Comment on attachment 109275 [details]
This patch solves issues regarding navigation and infinite loop cases with CSS pseudo elements before/after .

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

> Source/WebCore/editing/VisiblePosition.cpp:235
> -        if ((p.isCandidate() && p.downstream() != downstreamStart) || p.atStartOfTree() || p.atEndOfTree())
> +        if ((p.isCandidate() && p.downstream() != downstreamStart) || p.atStartOfTree() || p.atEndOfTree() || p == m_deepPosition)

I don't understand. Why do we return p when it's equal to m_deepEquivalent?

[Rosen Dash]

Created attachment 109760 [details]
Update on patch 109275

I have added a test to validate this change which checks for proper right arrow key navigation.

[Enrica Casucci]

Comment on attachment 109760 [details]
Update on patch 109275

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

> 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?

[Rosen Dash]

Please look into this attachment crash_test_set.html at
http://code.google.com/p/chromium/issues/detail?id=98247

In the first two cases the page goes into an infinite loop.

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.


(In reply to comment #11)
> (From update of attachment 109760 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109760&action=review
> 
> > 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?

[Ryosuke Niwa]

Comment on attachment 109760 [details]
Update on patch 109275

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
> [phone]  roseN Dash  <[e-mail]>

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.

[Rosen Dash]

Created attachment 110665 [details]
Update on patch 109760

I tried to solve the navigation issue in a generic way.
In case of CSS pseudo elements render node is getting created but there is no corresponding DOM node for that. So We can't create a Postion for that.

I tried to add a test for the hang scenario.

[WebKit Review Bot]

Attachment 110665 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1

Source/WebCore/editing/VisiblePosition.cpp:171:  Missing space before ( in if(  [whitespace/parens] [5]
Source/WebCore/editing/VisiblePosition.cpp:172:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/editing/VisiblePosition.cpp:329:  Missing space before ( in if(  [whitespace/parens] [5]
Source/WebCore/editing/VisiblePosition.cpp:330:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 4 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Rosen Dash]

Created attachment 110666 [details]
Update on patch 109760

Updated style issues on previous patch.

[Ryosuke Niwa]

Comment on attachment 110666 [details]
Update on patch 109760

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

> Source/WebCore/editing/VisiblePosition.cpp:172
> +                        if (renderer->node() == m_deepPosition.anchorNode() && offset == m_deepPosition.offsetInContainerNode())
> +                            return Position();

When do we hit this case? You're essentially checking p == m_deepPosition here. r- because of this.

[Rosen Dash]

Created attachment 110843 [details]
Solved the navigation issue with rtl texts.

The changes around line no 122/273 are to solve edge case conditions with pesdoelements.(hanging and navigation)
The changes around line no 193/345 are to solve navigation with rtl texts in presence of peudoelements.

[Ryosuke Niwa]

Comment on attachment 110843 [details]
Solved the navigation issue with rtl texts.

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

> LayoutTests/editing/selection/css-pseudo-element-hang.html:16
> +<span class="quote">content</span>

I'd also like to see a test case where there is some text on before/after the span.

> LayoutTests/editing/selection/css-pseudo-element-hang.html:26
> +for(var i = 0; i < 9; ++i) {
> +    window.getSelection().modify('move', 'right', 'character');
> +}

Nit: Please put a space after "for" before (
Also not curly brackets around a single statement.

> Source/WebCore/editing/VisiblePosition.cpp:198
> +                    if (prevBox->prevLeafChild())
> +                        return box->isLeftToRightDirection() ? previousVisuallyDistinctCandidate(m_deepPosition) : nextVisuallyDistinctCandidate(m_deepPosition);

I don't think this is right. If we have mixed bidi in generated contents, then we'll have a separate line box for each one of them. So it's not guaranteed that prevBox->prevLeafChild() exists.

[Rosen Dash]

Created attachment 111012 [details]
Update on patch 110843

Updated considering of multi level bidi text in generated contents.

[WebKit Review Bot]

Comment on attachment 111012 [details]
Update on patch 110843

Attachment 111012 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10056573

New failing tests:
editing/selection/css-pseudo-element-hang.html

[Ryosuke Niwa]

(In reply to comment #21)
> (From update of attachment 111012 [details])
> Attachment 111012 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/10056573
> 
> New failing tests:
> editing/selection/css-pseudo-element-hang.html

Why is this test failing now?

[Rosen Dash]

Created attachment 111211 [details]
Updated css-pseudo-element-hang-expected.txt after adding a new test case.

Sorry about that.
I missed to update the css-pseudo-element-hang-expected.txt file after adding the new text as per your suggestion case last time.

[Rosen Dash]

Created attachment 111217 [details]
Update on patch 111211

Added test case for bidi text in generated content.

[Ryosuke Niwa]

Comment on attachment 111217 [details]
Update on patch 111211

Great. I'm happy to see this bug is finally fixed.

[Rosen Dash]

Created attachment 111221 [details]
Updated on patch 111217

Added the portion changed copyright.

[WebKit Review Bot]

Comment on attachment 111221 [details]
Updated on patch 111217

Clearing flags on attachment: 111221

Committed r97597: <http://trac.webkit.org/changeset/97597>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

*** Bug 37474 has been marked as a duplicate of this bug. ***