Bug 38548 – REGRESSION: Weird focus behavior affects quoting on University of Washington message board system

• Component: DOM
• Status: RESOLVED FIXED

[Beth Dakin]

The UW message boards allow you to quote sections of someone else's post by highlighting the content you want to quote, and then clicking a "Quote" link that appears in the upper right corner of each post. Prior to r50315, this feature worked as expected, and the selected text is quoted in the editing box at the bottom of the screen where one can continue composing a new post with the quoted text.

After r50315, the first time you select text and click "Quote," the entire post gets quoted in the editing compose box rather than just the selected text. If you are careful to leave the focus on the link (ie, don't click on the page or anything), then if you try selecting text a second time, the quoting will work. When you are selecting text the second time, you will see both the text selection, and a focus ring around the link, implying that we allow the two selections to co-exist. But it appears that we do not allow the selections to coexist when clicking the link creates the second selection; then we just annihilate the text selection rather than allowing both.

This is a regression caused by http://trac.webkit.org/changeset/50315

I have attached a reduction. To reproduce the bug with the attached reduction:
1. Select some subset of the text "I like cherry pie. Want to make some for me?" For example, just select the work "cherry."
2. Click the "Quote" link. Prior to r50315, you would see markup in the textarea appear that just quotes the word cherry. In TOT, you will see the full sentence regardless of the subset that you highlighted. This is a bug.


<rdar://problem/7912732>

[Beth Dakin]

Created attachment 55047 [details]
Reduction

[Steven Lai]

The quoting script the site use checks if there is text selection, if there is no text selection, then the whole message would be copied

when you click on the link, the text becomes deselected before the onclick handler runs, so the quoting script would fail to find any text selection.

regression since 50315

<http://trac.webkit.org/changeset/50315>

[Beth Dakin]

In case it's not clear from the description, I would like to add that this is a really bad bug that seriously affects the usability of the UW message boards in WebKit. In many UW classes, participating in message board discussions is a graded part of the course, so there is a high volume of activity, and quoting is a very commonly-used feature.

[Erik Arvidsson]

What I'm mostly surprised by is that this works in other browsers. All browser focuses links on click.

[Beth Dakin]

Created attachment 55078 [details]
A Less-Reduced Reduction that works in Firefox

Erik, the reduction that I attached does nothing in Firefox when you click the "Quote" link. In other words, it doesn't fail the way TOT does, nor does it succeed like older version of WebKit; just -- nothing happens. Here is an earlier version of the reduction where the markup still works in Firefox. Unfortunately, there are two massive JS files in this version of the reduction. 

Despite the fact that the true reduction does nothing in Firefox, my running theory is NOT that the extended-JS does a bunch of WebKit-specific stuff. Rather, I think that Firefox allows the link selection, but prevents the link selection from annihilating any text selection when it is selected.

[Steven Lai]

Created attachment 55080 [details]
Reduction that works on Firefox

[Beth Dakin]

Created attachment 55081 [details]
Tiny test case

Here's another test that show the difference between WebKit TOT and Firefox. Steven made this one too. If you select some text in WebKit TOT, and then click the link, the text selection is gone, and thee is a focus rings around the link. In ffox, the text selection stays AND there is a focus ring around the link.

[Erik Arvidsson]

Simplified test case:

<p>Select</p>
<a href="#" tabindex=-1>Click</a>

http://www.plexode.com/cgi-bin/eval3.py#ht=%3Cp%3ESelect%3C%2Fp%3E%0A%3Ca%20href%3D%22%23%22%20tabindex%3D-1%3EClick%3C%2Fa%3E&ohh=1&ohj=1&jt=&ojh=1&ojj=1&ms=100&oth=0&otj=0&cex=1

The difference is that Firefox does not collapse the selection when a focusable element takes the focus.

[Steven Lai]

Not too sure if this observation is right:
It appears that on Webkit, whenever text loses focus, the text selection is also automatically lost
(except the case the topmost window lost focus)

[Erik Arvidsson]

Sorry, I didn't see your test case in time. I was busy creating mine.

The obvious work around in js is to call preventDefault in mousedown which
prevents focus to be moved.

To fix this in WebKit we could either prevent links to be mouse focusable (like
in Safari 4) or we could change the logic for when to collapse selections to
match Firefox.

Reverting the focus fix is trivial. I have yet to fully understand what makes
Firefox decide when to collapse current selection.

[Erik Arvidsson]

I managed to make sense out of the Firefox collapsing behavior.

In Firefox the selection is not collapsed if the current target cannot be selected, either by doing preventDefault, being draggable or having user-select: none.

[Steven Lai]

And, when the current target is not in the "at the same window" as the original focused text selection
for example,
1. select some text on a web page, and then use the mouse to selection the address bar textfield
2. inside the page there's an <iframe>, and inside the iframe, there's a <textarea>, select some text on the web page outside the <iframe>, then select text within the <textarea> inside the <iframe>

I'm not really sure if we could come up with an exhaustive list that generalizes the selection/focus behaviors across browser and platform.

[Steven Lai]

Created attachment 55083 [details]
Focus behavior in Firefox (1)

[Steven Lai]

Created attachment 55084 [details]
Focus behavior in Firefox (2)

[Beth Dakin]

Erik, do you have any progress working on a fix for this? I feel like this regression is bad enough that we should consider rolling out the original change if we can't get a fix in the tree soon.

[Erik Arvidsson]

I'm looking into this at the moment and it seems like HTMLAnchorElement::canStartSelection is broken. (Also, Node::canStartSelection is also having problems with user-select none.)

Fixing the canStartSelection seems like a better solution than rolling back r50315.

[Beth Dakin]

(In reply to comment #16)
> Fixing the canStartSelection seems like a better solution than rolling back
> r50315.

I agree. But I also think that the regression is bad enough that now that we know about it, we shouldn't leave it it the tree for long if we can help it. If you are actively working on a solution for the selection problem though, then I can wait.

[Erik Arvidsson]

I got a fix coming that fixes the issue by not collapsing selection on mouse down when the target->canStartSelection() returned false.

[Erik Arvidsson]

Created attachment 55330 [details]
Patch

[Erik Arvidsson]

Created attachment 55333 [details]
Patch

[Beth Dakin]

Comment on attachment 55333 [details]
Patch

> -    if (Node* mousePressNode = newFocusedFrame->eventHandler()->mousePressNode())
> -        if (mousePressNode->renderer() && !mousePressNode->canStartSelection())
> -            if (Node* root = s->rootEditableElement())
> -                if (Node* shadowAncestorNode = root->shadowAncestorNode())
> +    if (Node* mousePressNode = newFocusedFrame->eventHandler()->mousePressNode()) {
> +        if (mousePressNode->renderer() && !mousePressNode->canStartSelection()) {
> +            // In case selection was in an input or textarea we need to clear the selection. See bug 38696.
> +            if (Node* root = s->rootEditableElement()) {
> +                if (Node* shadowAncestorNode = root->shadowAncestorNode()) {
>                      // Don't do this for textareas and text fields, when they lose focus their selections should be cleared
>                      // and then restored when they regain focus, to match other browsers.
>                      if (!shadowAncestorNode->hasTagName(inputTag) && !shadowAncestorNode->hasTagName(textareaTag))
>                          return;
> +                }
> +            } else {
> +              return;
> +            }
> +        }
> +    }
>      
>      s->clear();
>  }

I have a few questions about this patch. First, the comment that you added is "In case selection was in an input or textarea we need to clear the selection. See bug 38696." But isn't the opposite true? Isn't it that we do NOT want to clear the selection in the case of an input or textarea? 

Also, are you certain that adding an else to the (Node* root = s->rootEditableElement()) case is correct? Can you be certain that this will only apply to inputs and textareas like your comment implies?

[Erik Arvidsson]

(In reply to comment #21)
> (From update of attachment 55333 [details])
> > -    if (Node* mousePressNode = newFocusedFrame->eventHandler()->mousePressNode())
> > -        if (mousePressNode->renderer() && !mousePressNode->canStartSelection())
> > -            if (Node* root = s->rootEditableElement())
> > -                if (Node* shadowAncestorNode = root->shadowAncestorNode())
> > +    if (Node* mousePressNode = newFocusedFrame->eventHandler()->mousePressNode()) {
> > +        if (mousePressNode->renderer() && !mousePressNode->canStartSelection()) {
> > +            // In case selection was in an input or textarea we need to clear the selection. See bug 38696.
> > +            if (Node* root = s->rootEditableElement()) {
> > +                if (Node* shadowAncestorNode = root->shadowAncestorNode()) {
> >                      // Don't do this for textareas and text fields, when they lose focus their selections should be cleared
> >                      // and then restored when they regain focus, to match other browsers.
> >                      if (!shadowAncestorNode->hasTagName(inputTag) && !shadowAncestorNode->hasTagName(textareaTag))
> >                          return;
> > +                }
> > +            } else {
> > +              return;
> > +            }
> > +        }
> > +    }
> >      
> >      s->clear();
> >  }
> 
> I have a few questions about this patch. First, the comment that you added is
> "In case selection was in an input or textarea we need to clear the selection.
> See bug 38696." But isn't the opposite true? Isn't it that we do NOT want to
> clear the selection in the case of an input or textarea? 

We currently clear the selection of textareas and inputs because if there is an existing selection in editable element in the document then any keyboard event gets routed back to the editable element (even if it does not have focus). This is what bug 38696 is about.

> Also, are you certain that adding an else to the (Node* root =
> s->rootEditableElement()) case is correct? Can you be certain that this will
> only apply to inputs and textareas like your comment implies?

Bug 38696 covers this. contentEditable elements go all the way into the innermost if check but they don't match the condition so we exit the function there. That is why they steal the keyboard input and focus.

However, I did not want to make that change here since I'd rather do that in another patch since I'd like to focus on one issue at a time.

Part of fixing 38696 would be to make that last if statement simply:

if (Node* mousePressNode = newFocusedFrame->eventHandler()->mousePressNode()) {
    if (mousePressNode->renderer() && !mousePressNode->canStartSelection()) {
        return;
    }
}

HTH

[Ojan Vafai]

Comment on attachment 55333 [details]
Patch

In either case, this patch fixes the regression and I think is correct. r- due to a few nits below.

(In reply to comment #22)
> (In reply to comment #21)
> > Also, are you certain that adding an else to the (Node* root =
> > s->rootEditableElement()) case is correct? Can you be certain that this will
> > only apply to inputs and textareas like your comment implies?
> 
> Bug 38696 covers this. contentEditable elements go all the way into the innermost if check but they don't match the condition so we exit the function there. That is why they steal the keyboard input and focus.
> 
> However, I did not want to make that change here since I'd rather do that in another patch since I'd like to focus on one issue at a time.

I agree that this should be a separate patch as it has a good chance of having compat problems. I made a slightly more comprehensive test case to check the correctness of this patch: 

http://www.plexode.com/cgi-bin/eval3.py#ht=%3Cp%3Enot%20editable%3C%2Fp%3E%0A%3Cp%20contentEditable%3EcontentEditable%3C%2Fp%3E%0A%3Ctextarea%3Etextarea%3C%2Ftextarea%3E%3Cbr%3E%0A%3Cinput%20value%3Dinput%3E%3Cbr%3E%0A%3Ca%20href%3D%22%23%22%20tabindex%3D-1%3ESelect%20text%20then%20click%20here%20(tabIndex%3D-1).%3C%2Fa%3E%3Cbr%3E%0A%3Ca%20href%3D%22%23%22%3ESelect%20text%20then%20click%20here%20(no%20tabindex).%3C%2Fa%3E&ohh=1&ohj=1&jt=&ojh=1&ojj=1&ms=100&oth=0&otj=0&cex=1

> Part of fixing 38696 would be to make that last if statement simply:
> 
> if (Node* mousePressNode = newFocusedFrame->eventHandler()->mousePressNode()) {
>     if (mousePressNode->renderer() && !mousePressNode->canStartSelection()) {
>         return;
>     }
> }

We can discuss that in bug 38696, but it's not totally clear what the ideal behavior is.

LayoutTests/editing/selection/script-tests/click-in-focusable-link-should-not-clear-selection.js:3
 +  function getElementCenter(el) {
We haven't been very consistent about this with JS code, but webkit style is to put the first curly brace on a new line.

LayoutTests/editing/selection/script-tests/click-in-focusable-link-should-not-clear-selection.js:15
 +      console.log(point.x, point.y);
This doesn't look like it's logging both values. In either case, doesn't seem like this needs to stay in the test.

LayoutTests/editing/selection/script-tests/click-in-focusable-link-should-not-clear-selection.js:29
 +  function mouseDownAt(point) {
No need to make this a helper function. It's only used once above.

LayoutTests/editing/selection/script-tests/click-in-focusable-link-should-not-clear-selection.js:62
 +      document.body.removeChild(span);
It's OK to leave these in the tree. They'll end up in the text dump, but that's fine.

+            if (Node* root = s->rootEditableElement()) {
+                if (Node* shadowAncestorNode = root->shadowAncestorNode()) {
                     // Don't do this for textareas and text fields, when they lose focus their selections should be cleared
                     // and then restored when they regain focus, to match other browsers.
                     if (!shadowAncestorNode->hasTagName(inputTag) && !shadowAncestorNode->hasTagName(textareaTag))
                         return;
+                }
+            } else {
+              return;
+            }

I find an else with just a return statement hard to read. How about something like:
                 Node* root = s->rootEditableElement());
                 if (!root)
                     return; 

                 if (Node* shadowAncestorNode = root->shadowAncestorNode()) {
                     // Don't do this for textareas and text fields, when they lose focus their selections should be cleared
                     // and then restored when they regain focus, to match other browsers.
                     if (!shadowAncestorNode->hasTagName(inputTag) && !shadowAncestorNode->hasTagName(textareaTag))
                         return;
                 }

[Erik Arvidsson]

Created attachment 56619 [details]
Patch

[Ojan Vafai]

Comment on attachment 56619 [details]
Patch

Please improve the comment before committing.

WebCore/page/FocusController.cpp:516
 +              // In case selection was in an input or textarea we need to clear the selection. See bug 38696.
I find this comment a bit confusing and it doesn't say much over the one below. How about:
// Don't clear the selection for contentEditable elements, but do clear it for input and textarea. See bug 38696.

[Erik Arvidsson]

Created attachment 57699 [details]
Patch

[Eric Seidel (no email)]

The buildbots seem to show this as being landed and having broken a test on mac:
http://build.webkit.org/results/Tiger%20Intel%20Release/r60580%20(12791)/editing/pasteboard/drop-link-pretty-diff.html

[Erik Arvidsson]

Created attachment 57721 [details]
New patch with updated expectations for drop-link.html

[Erik Arvidsson]

Created attachment 57810 [details]
Added a comment about why the expectation changed.

[Ojan Vafai]

Comment on attachment 57810 [details]
Added a comment about why the expectation changed.

> +        * editing/pasteboard/drop-link-expected.txt: Mouse down on an element where canStartSelection returns
> +                                                     false no longer clears the selection so we get one less
> +                                                     notification from the editing delegate.

This description doesn't match what I see when I run the test manually. I don't see the selection getting cleared any differently before or after this patch. I think this is probably benign, but we should understand why this has changed.

[Erik Arvidsson]

(In reply to comment #30)
> (From update of attachment 57810 [details])
> > +        * editing/pasteboard/drop-link-expected.txt: Mouse down on an element where canStartSelection returns
> > +                                                     false no longer clears the selection so we get one less
> > +                                                     notification from the editing delegate.
> 
> This description doesn't match what I see when I run the test manually. I don't see the selection getting cleared any differently before or after this patch. I think this is probably benign, but we should understand why this has changed.

The test does a window.getSelection().setBaseAndExtent on the link that is being dragged. With my patch we no longer clear the selection when the mouse is pressed on an element where canStartSelection returns false and the selection is not in an rootEditableElement. Therefore we do not get the webViewDidChangeSelection:WebViewDidChangeSelectionNotification.

[WebKit Commit Bot]

Comment on attachment 57810 [details]
Added a comment about why the expectation changed.

Rejecting patch 57810 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
Last 500 characters of output:
ng Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Skipped list contained 'compositing/iframes/composited-iframe.html', but no file of that name could be found
Testing 19060 test cases.
fast/loader/recursive-before-unload-crash.html -> failed

Exiting early after 1 failures. 14084 tests run.
201.82s total testing time

14083 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
4 test cases (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/3239010

[Erik Arvidsson]

Committed r60859: <http://trac.webkit.org/changeset/60859>

[WebKit Review Bot]

http://trac.webkit.org/changeset/60859 might have broken SnowLeopard Intel Release (Tests)

[Ojan Vafai]

Patch accidentally included logging lines and was rolled back. The patch above is still r+'ed as is.

[Erik Arvidsson]

Comment on attachment 57810 [details]
Added a comment about why the expectation changed.

Last time the commit queue failed on this was due to an unrelated issue with skip list.

[WebKit Commit Bot]

Comment on attachment 57810 [details]
Added a comment about why the expectation changed.

Clearing flags on attachment: 57810

Committed r60873: <http://trac.webkit.org/changeset/60873>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.