[Webkit-unassigned] [Bug 38548] REGRESSION: Weird focus behavior affects quoting on University of Washington message board system

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=38548


Ojan Vafai <ojan at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #55333|review?                     |review-
               Flag|                            |




--- Comment #23 from Ojan Vafai <ojan at chromium.org>  2010-05-19 18:25:57 PST ---
(From update of attachment 55333)
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;
                 }

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.