Bug 10123 - when CSS pseudo selectors are applied (:before and :after) the *-of-line keyboard navigation does not work
: when CSS pseudo selectors are applied (:before and :after) the *-of-line keyb...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: HTML Editing
: 412
: Macintosh Mac OS X 10.4
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-07-26 14:02 PDT by Robert Burns
Modified: 2011-11-03 13:47 PDT (History)
7 users (show)

See Also:


Attachments
for openning in blot.app to test keyboard navigation (3.20 KB, text/html)
2006-09-19 12:54 PDT, Robert Burns
no flags Details
A better example that works in browser (no blot.app required) (915 bytes, text/html)
2010-01-08 17:08 PST, Robert Burns
no flags Details
This patch solves issues regarding navigation and infinite loop cases with CSS pseudo elements before/after . (3.21 KB, patch)
2011-09-30 03:20 PDT, Rosen Dash
rniwa: review-
Details | Formatted Diff | Diff
Update on patch 109275 (6.87 KB, patch)
2011-10-05 01:21 PDT, Rosen Dash
rniwa: review-
Details | Formatted Diff | Diff
Update on patch 109760 (9.23 KB, patch)
2011-10-12 04:23 PDT, Rosen Dash
no flags Details | Formatted Diff | Diff
Update on patch 109760 (9.27 KB, patch)
2011-10-12 04:35 PDT, Rosen Dash
rniwa: review-
Details | Formatted Diff | Diff
Solved the navigation issue with rtl texts. (11.01 KB, patch)
2011-10-13 07:53 PDT, Rosen Dash
rniwa: review-
Details | Formatted Diff | Diff
Update on patch 110843 (11.51 KB, patch)
2011-10-14 07:31 PDT, Rosen Dash
webkit.review.bot: commit‑queue-
Details | Formatted Diff | Diff
Updated css-pseudo-element-hang-expected.txt after adding a new test case. (11.52 KB, patch)
2011-10-16 22:27 PDT, Rosen Dash
no flags Details | Formatted Diff | Diff
Update on patch 111211 (14.77 KB, patch)
2011-10-16 23:32 PDT, Rosen Dash
rniwa: review+
rniwa: commit‑queue+
Details | Formatted Diff | Diff
Updated on patch 111217 (15.09 KB, patch)
2011-10-17 01:04 PDT, Rosen Dash
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Burns 2006-07-26 14:02:10 PDT
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.
Comment 1 Mark Rowe (bdash) 2006-08-11 15:25:08 PDT
Robert, can you please attach a simple HTML file that will more easily demonstrate the issue?
Comment 2 Robert Burns 2006-09-19 12:54:15 PDT
Created attachment 10651 [details]
for openning in blot.app to test keyboard navigation

Here's the requested sample.
Comment 3 Joseph Holsten 2010-01-08 16:02:18 PST
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.
Comment 4 Robert Burns 2010-01-08 17:08:47 PST
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.
Comment 5 Rosen Dash 2011-09-29 23:49:53 PDT
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.
Comment 6 Rosen Dash 2011-09-30 03:20:41 PDT
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
Comment 7 Rosen Dash 2011-09-30 03:32:21 PDT
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.
Comment 8 Ryosuke Niwa 2011-10-04 11:43:00 PDT
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.
Comment 9 Ryosuke Niwa 2011-10-04 11:44:32 PDT
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?
Comment 10 Rosen Dash 2011-10-05 01:21:59 PDT
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.
Comment 11 Enrica Casucci 2011-10-07 10:51:48 PDT
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?
Comment 12 Rosen Dash 2011-10-07 21:21:56 PDT
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?
Comment 13 Ryosuke Niwa 2011-10-10 22:14:23 PDT
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
> +2011-10-05  roseN Dash  <rosen.dash@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.
Comment 14 Rosen Dash 2011-10-12 04:23:59 PDT
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.
Comment 15 WebKit Review Bot 2011-10-12 04:26:34 PDT
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.
Comment 16 Rosen Dash 2011-10-12 04:35:12 PDT
Created attachment 110666 [details]
Update on patch 109760

Updated style issues on previous patch.
Comment 17 Ryosuke Niwa 2011-10-12 13:08:40 PDT
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.
Comment 18 Rosen Dash 2011-10-13 07:53:09 PDT
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.
Comment 19 Ryosuke Niwa 2011-10-13 12:35:46 PDT
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.
Comment 20 Rosen Dash 2011-10-14 07:31:24 PDT
Created attachment 111012 [details]
Update on patch 110843

Updated considering of multi level bidi text in generated contents.
Comment 21 WebKit Review Bot 2011-10-14 07:59:28 PDT
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
Comment 22 Ryosuke Niwa 2011-10-14 15:52:36 PDT
(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?
Comment 23 Rosen Dash 2011-10-16 22:27:31 PDT
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.
Comment 24 Rosen Dash 2011-10-16 23:32:55 PDT
Created attachment 111217 [details]
Update on patch 111211

Added test case for bidi text in generated content.
Comment 25 Ryosuke Niwa 2011-10-17 00:41:18 PDT
Comment on attachment 111217 [details]
Update on patch 111211

Great. I'm happy to see this bug is finally fixed.
Comment 26 Rosen Dash 2011-10-17 01:04:25 PDT
Created attachment 111221 [details]
Updated on patch 111217

Added the portion changed copyright.
Comment 27 WebKit Review Bot 2011-10-17 02:17:41 PDT
Comment on attachment 111221 [details]
Updated on patch 111217

Clearing flags on attachment: 111221

Committed r97597: <http://trac.webkit.org/changeset/97597>
Comment 28 WebKit Review Bot 2011-10-17 02:17:48 PDT
All reviewed patches have been landed.  Closing bug.
Comment 29 Ryosuke Niwa 2011-11-03 13:47:45 PDT
*** Bug 37474 has been marked as a duplicate of this bug. ***