[webkit-reviews] review denied: [Bug 38548] REGRESSION: Weird focus behavior affects quoting on University of Washington message board system : [Attachment 55333] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Erik Arvidsson <arv at chromium.org>'s
request for review:
Bug 38548: REGRESSION: Weird focus behavior affects quoting on University of
Washington message board system
https://bugs.webkit.org/show_bug.cgi?id=38548

Attachment 55333: Patch
https://bugs.webkit.org/attachment.cgi?id=55333&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
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%3
Cp%20contentEditable%3EcontentEditable%3C%2Fp%3E%0A%3Ctextarea%3Etextarea%3C%2F
textarea%3E%3Cbr%3E%0A%3Cinput%20value%3Dinput%3E%3Cbr%3E%0A%3Ca%20href%3D%22%2
3%22%20tabindex%3D-1%3ESelect%20text%20then%20click%20here%20(tabIndex%3D-1).%3
C%2Fa%3E%3Cbr%3E%0A%3Ca%20href%3D%22%23%22%3ESelect%20text%20then%20click%20her
e%20(no%20tabindex).%3C%2Fa%3E&ohh=1&ohj=1&jt=&ojh=1&ojj=1&ms=100&oth=0&otj=0&c
ex=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-c
lear-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-c
lear-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-c
lear-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-c
lear-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;
		 }