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
: WebKit
HTML Editing
: 412
: Macintosh Mac OS X 10.4
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2006-07-26 14:02 PST by
Modified: 2011-11-03 13:47 PST (History)


Attachments
for openning in blot.app to test keyboard navigation (3.20 KB, text/html)
2006-09-19 12:54 PST, 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 PST, Rosen Dash
rniwa: review-
Review Patch | Details | Formatted Diff | Diff
Update on patch 109275 (6.87 KB, patch)
2011-10-05 01:21 PST, Rosen Dash
rniwa: review-
Review Patch | Details | Formatted Diff | Diff
Update on patch 109760 (9.23 KB, patch)
2011-10-12 04:23 PST, Rosen Dash
no flags Review Patch | Details | Formatted Diff | Diff
Update on patch 109760 (9.27 KB, patch)
2011-10-12 04:35 PST, Rosen Dash
rniwa: review-
Review Patch | Details | Formatted Diff | Diff
Solved the navigation issue with rtl texts. (11.01 KB, patch)
2011-10-13 07:53 PST, Rosen Dash
rniwa: review-
Review Patch | Details | Formatted Diff | Diff
Update on patch 110843 (11.51 KB, patch)
2011-10-14 07:31 PST, Rosen Dash
webkit.review.bot: commit‑queue-
Review Patch | 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 PST, Rosen Dash
no flags Review Patch | Details | Formatted Diff | Diff
Update on patch 111211 (14.77 KB, patch)
2011-10-16 23:32 PST, Rosen Dash
rniwa: review+
rniwa: commit‑queue+
Review Patch | Details | Formatted Diff | Diff
Updated on patch 111217 (15.09 KB, patch)
2011-10-17 01:04 PST, Rosen Dash
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2006-07-26 14:02:10 PST
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 From 2006-08-11 15:25:08 PST -------
Robert, can you please attach a simple HTML file that will more easily demonstrate the issue?
------- Comment #2 From 2006-09-19 12:54:15 PST -------
Created an attachment (id=10651) [details]
for openning in blot.app to test keyboard navigation

Here's the requested sample.
------- Comment #3 From 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 From 2010-01-08 17:08:47 PST -------
Created an attachment (id=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 From 2011-09-29 23:49:53 PST -------
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 From 2011-09-30 03:20:41 PST -------
Created an attachment (id=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 From 2011-09-30 03:32:21 PST -------
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 From 2011-10-04 11:43:00 PST -------
(From update of attachment 109275 [details])
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 From 2011-10-04 11:44:32 PST -------
(From update of attachment 109275 [details])
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 From 2011-10-05 01:21:59 PST -------
Created an attachment (id=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 From 2011-10-07 10:51:48 PST -------
(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 #12 From 2011-10-07 21:21:56 PST -------
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] [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 From 2011-10-10 22:14:23 PST -------
(From update of attachment 109760 [details])
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 From 2011-10-12 04:23:59 PST -------
Created an attachment (id=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 From 2011-10-12 04:26:34 PST -------
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 From 2011-10-12 04:35:12 PST -------
Created an attachment (id=110666) [details]
Update on patch 109760

Updated style issues on previous patch.
------- Comment #17 From 2011-10-12 13:08:40 PST -------
(From update of attachment 110666 [details])
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 From 2011-10-13 07:53:09 PST -------
Created an attachment (id=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 From 2011-10-13 12:35:46 PST -------
(From update of attachment 110843 [details])
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 From 2011-10-14 07:31:24 PST -------
Created an attachment (id=111012) [details]
Update on patch 110843

Updated considering of multi level bidi text in generated contents.
------- Comment #21 From 2011-10-14 07:59:28 PST -------
(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
------- Comment #22 From 2011-10-14 15:52:36 PST -------
(In reply to comment #21)
> (From update of attachment 111012 [details] [details])
> Attachment 111012 [details] [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 From 2011-10-16 22:27:31 PST -------
Created an attachment (id=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 From 2011-10-16 23:32:55 PST -------
Created an attachment (id=111217) [details]
Update on patch 111211

Added test case for bidi text in generated content.
------- Comment #25 From 2011-10-17 00:41:18 PST -------
(From update of attachment 111217 [details])
Great. I'm happy to see this bug is finally fixed.
------- Comment #26 From 2011-10-17 01:04:25 PST -------
Created an attachment (id=111221) [details]
Updated on patch 111217

Added the portion changed copyright.
------- Comment #27 From 2011-10-17 02:17:41 PST -------
(From update of attachment 111221 [details])
Clearing flags on attachment: 111221

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