Bug 38548 - REGRESSION: Weird focus behavior affects quoting on University of Washington message board system
Summary: REGRESSION: Weird focus behavior affects quoting on University of Washington ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar, Regression
Depends on: 40087 40334
Blocks:
  Show dependency treegraph
 
Reported: 2010-05-04 14:14 PDT by Beth Dakin
Modified: 2010-06-08 17:29 PDT (History)
11 users (show)

See Also:


Attachments
Reduction (2.96 KB, text/html)
2010-05-04 14:15 PDT, Beth Dakin
no flags Details
A Less-Reduced Reduction that works in Firefox (69.84 KB, application/octet-stream)
2010-05-04 17:32 PDT, Beth Dakin
no flags Details
Reduction that works on Firefox (2.92 KB, text/html)
2010-05-04 17:37 PDT, Steven Lai
no flags Details
Tiny test case (202 bytes, text/html)
2010-05-04 17:41 PDT, Beth Dakin
no flags Details
Focus behavior in Firefox (1) (20.20 KB, image/png)
2010-05-04 18:55 PDT, Steven Lai
no flags Details
Focus behavior in Firefox (2) (32.98 KB, image/png)
2010-05-04 18:56 PDT, Steven Lai
no flags Details
Patch (7.20 KB, patch)
2010-05-06 18:15 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch (7.54 KB, patch)
2010-05-06 18:33 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch (7.35 KB, patch)
2010-05-20 12:08 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch (7.16 KB, patch)
2010-06-02 13:57 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
New patch with updated expectations for drop-link.html (8.40 KB, patch)
2010-06-02 18:01 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Added a comment about why the expectation changed. (8.77 KB, patch)
2010-06-03 13:54 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2010-05-04 14:14:16 PDT
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>
Comment 1 Beth Dakin 2010-05-04 14:15:11 PDT
Created attachment 55047 [details]
Reduction
Comment 2 Steven Lai 2010-05-04 14:28:31 PDT
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>
Comment 3 Beth Dakin 2010-05-04 14:35:53 PDT
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.
Comment 4 Erik Arvidsson 2010-05-04 16:56:51 PDT
What I'm mostly surprised by is that this works in other browsers. All browser focuses links on click.
Comment 5 Beth Dakin 2010-05-04 17:32:07 PDT
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.
Comment 6 Steven Lai 2010-05-04 17:37:58 PDT
Created attachment 55080 [details]
Reduction that works on Firefox
Comment 7 Beth Dakin 2010-05-04 17:41:06 PDT
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.
Comment 8 Erik Arvidsson 2010-05-04 17:43:51 PDT
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.
Comment 9 Steven Lai 2010-05-04 17:49:30 PDT
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)
Comment 10 Erik Arvidsson 2010-05-04 17:59:13 PDT
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.
Comment 11 Erik Arvidsson 2010-05-04 18:08:14 PDT
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.
Comment 12 Steven Lai 2010-05-04 18:53:13 PDT
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.
Comment 13 Steven Lai 2010-05-04 18:55:40 PDT
Created attachment 55083 [details]
Focus behavior in Firefox (1)
Comment 14 Steven Lai 2010-05-04 18:56:53 PDT
Created attachment 55084 [details]
Focus behavior in Firefox (2)
Comment 15 Beth Dakin 2010-05-05 15:31:27 PDT
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.
Comment 16 Erik Arvidsson 2010-05-06 12:05:29 PDT
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.
Comment 17 Beth Dakin 2010-05-06 12:24:13 PDT
(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.
Comment 18 Erik Arvidsson 2010-05-06 16:22:12 PDT
I got a fix coming that fixes the issue by not collapsing selection on mouse down when the target->canStartSelection() returned false.
Comment 19 Erik Arvidsson 2010-05-06 18:15:10 PDT
Created attachment 55330 [details]
Patch
Comment 20 Erik Arvidsson 2010-05-06 18:33:13 PDT
Created attachment 55333 [details]
Patch
Comment 21 Beth Dakin 2010-05-07 10:39:05 PDT
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?
Comment 22 Erik Arvidsson 2010-05-07 11:01:27 PDT
(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
Comment 23 Ojan Vafai 2010-05-19 18:25:57 PDT
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;
                 }
Comment 24 Erik Arvidsson 2010-05-20 12:08:58 PDT
Created attachment 56619 [details]
Patch
Comment 25 Ojan Vafai 2010-05-20 14:00:03 PDT
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.
Comment 26 Erik Arvidsson 2010-06-02 13:57:38 PDT
Created attachment 57699 [details]
Patch
Comment 27 Eric Seidel (no email) 2010-06-02 15:55:13 PDT
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
Comment 28 Erik Arvidsson 2010-06-02 18:01:17 PDT
Created attachment 57721 [details]
New patch with updated expectations for drop-link.html
Comment 29 Erik Arvidsson 2010-06-03 13:54:38 PDT
Created attachment 57810 [details]
Added a comment about why the expectation changed.
Comment 30 Ojan Vafai 2010-06-03 15:30:26 PDT
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.
Comment 31 Erik Arvidsson 2010-06-07 18:03:04 PDT
(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.
Comment 32 WebKit Commit Bot 2010-06-07 20:15:53 PDT
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
Comment 33 Erik Arvidsson 2010-06-08 12:30:02 PDT
Committed r60859: <http://trac.webkit.org/changeset/60859>
Comment 34 WebKit Review Bot 2010-06-08 13:07:57 PDT
http://trac.webkit.org/changeset/60859 might have broken SnowLeopard Intel Release (Tests)
Comment 35 Ojan Vafai 2010-06-08 14:49:51 PDT
Patch accidentally included logging lines and was rolled back. The patch above is still r+'ed as is.
Comment 36 Erik Arvidsson 2010-06-08 15:14:49 PDT
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.
Comment 37 WebKit Commit Bot 2010-06-08 17:29:24 PDT
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>
Comment 38 WebKit Commit Bot 2010-06-08 17:29:35 PDT
All reviewed patches have been landed.  Closing bug.