214843 - Space doesn't scroll when link's child has focus

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Jesse Ruderman]

Steps to reproduce:
1. Load the testcase.
2. Drag or middle-click the link labelled "bold in link (broken)".
3. Press spacebar.

Result: nothing happens
Expected: page scrolls down

This could be a dup of bug 65917 (except that it's marked as a style system bug)
or bug 102663.

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Jesse Ruderman]

*** Bug 229064 has been marked as a duplicate of this bug. ***

Comment 3

[Nian Liu(n/a in a long time)]

the reason is that once there is any element between <a> and </a>, the focused
element is set to that element. navigate the node tree can solve this problem.
however, i'm not sure whether it will bring other issues. whatever, i'll post
patch later.

jst, saari and Aaron, any comments on that?

Comment 4

[Nian Liu(n/a in a long time)]

Attachment: patch for 214843

Comment 5

[Nian Liu(n/a in a long time)]

Attachment: patch for 214843

Comment 6

[Nian Liu(n/a in a long time)]

Attachment: patch for 214843

fix bug of space should also work when there is no focus element in previous
patch

Comment 7

[Nian Liu(n/a in a long time)]

Attachment: patch for 214843

wrong attachment in previous patch

Comment 8

[Aaron Leventhal]

Comment on attachment 147204 [details] [diff] [review]
patch for 214843

Couldn't the <a> also be a grandparent? Maybe you want something like this:

// Check to see if linked
for (nsIContent *findAnchorAncestor = child; findAnchorAncestor; 
     findAnchorAncestor = findAnchorAncestor->GetParent()) {
  nsCOMPtr<nsIDOMHTMLAnchorElement> anchor  = 
    do_QueryInterface(findAnchorAncestor);
  if (anchor) {
    break; // Found an anchor
  }
}
if (!findAnchorAncestor) {
  ...

Comment 9

[Nian Liu(n/a in a long time)]

well, i think the while loop can find the grandparent, too. correct me if i made
any mistake.

Comment 10

[Aaron Leventhal]

Comment on attachment 147204 [details] [diff] [review]
patch for 214843

Sorry Neo, I missed thta in your patch. Not enough coffee or something.

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 147204 [details] [diff] [review]
patch for 214843

>-      
>-      if (focusedElement) {
>-        nsAutoString tagName;
>-        focusedElement->GetTagName(tagName);
>+      PRBool isA = PR_FALSE;
>+      if (focusedElement){
>+        while (focusedElement != nsnull) {      
>+          nsAutoString tagName;
>+          focusedElement->GetTagName(tagName);
>         
>-        // if the focused element is a link then we do want space to
>-        // scroll down.
>-        if (tagName != NS_LITERAL_STRING("A"))
>+          // if the focused element is a link then we do want space to
>+          // scroll down. focused element may be an element in a link,
>+          // we need to check the parent node too.
>+          if (tagName == NS_LITERAL_STRING("A")) {
>+            isA = PR_TRUE;
>+            break;
>+          }
>+
>+          nsCOMPtr<nsIDOMNode> parentNode;
>+          focusedElement->GetParentNode(getter_AddRefs(parentNode));
>+          focusedElement = do_QueryInterface(parentNode);
>+        }
>+        if ( !isA )
>           return NS_OK;

This would work, but it only fixes part of the problem, it doesn't fix the
problem for XLinks. How about making that loop look like this to take care of
both problems:

      PRBool isLink = PR_FALSE;
      nsCOMPtr<nsIContent> focusedContent = do_QueryInterface(focusedElement);
      nsIContent *content = focusedContent;

      // if the focused element is a link then we do want space to
      // scroll down. focused element may be an element in a link,
      // we need to check the parent node too.

      while (content) {
	if (content->Tag() == nsHTMLTags::a &&
	    content->IsContentOfType(nsIContent::eHTML)) {
	  isLink = PR_TRUE;
	  break;
	}

	if (content->HasAttr(kNameSpaceID_XLink, nsHTMLAtoms::type)) {
	  nsAutoString type;
	  content->GetAttr(kNameSpaceID_XLink, nsHTMLAtoms::type);

	  isLink = type.Equals(NS_LITERAL_STRING("simple"));

	  if (isLink) {
	    break;
	  }
	}

	content = content->GetParent();
      }

      if (!isLink)
	return NS_OK;

sr=jst with tha change.

Comment 12

[Aaron Leventhal]

If we're going to do that, I suggest we encapsulate that logic in a method
somewhere, such as nsIContent->IsLinked().

We use similar checks in accessibility, find as you type, the focus code and
probably elsewhere, and we're adding lots of code each time. Also, what happens
when there's some new kind of link?

Comment 13

[Johnny Stenback (:jst)]

Aaron, I agree that it's lame to spread this code around, and I would love to
put it in a centralized place, but a method like that does IMO not belong in
nsIContent. If we'd have a singleton nsIContentUtils or somesuch, that would be
where this should go, but we don't yet, so there's no place yet to put a helper
like this.

Feel free to file a followup bug on centralizing the code to figure out if an
element is a link or not, but until then, I'd like us to proceed with the
proposal in this bug.

Comment 14

[Nian Liu(n/a in a long time)]

jst,

i'm not familla with XML. so could you help me explain why we just need consider
"simple" type with name space "xmlns:xlink", why not others such as "extended"?

Comment 15

[Kyle Yuan]

Attachment: the final patch (revised according to jst's comment)

Comment 16

[Kyle Yuan]

attachment 147954 [details] [diff] [review] has been checked in.

Comment 17

[[e-mail]]

Why do we need this loop at all? Surely it's up to those elements that want to
prevent space from scrolling to take steps to prevent it? Unfortunately it looks
like only textboxes and textareas do at the moment...

Comment 18

[Nian Liu(n/a in a long time)]

neil,

could you make a test case to break it?

thanks in advance and best regards,

Liu, Nian

Comment 19

[[e-mail]]

Attachment: Test case

You can't toggle the checkboxes with the space bar because the page scrolls.

Comment 20

[Nian Liu(n/a in a long time)]

my test result is that checkbox is toggled with page scroll. for input element
with type "textarea", it works fine( space is consumed by input element ). 

Neil, seems that you have an idea where "textarea" consumes the space. where is it?

Comment 21

[[e-mail]]

textarea actually uses nsPlaintextEditor.cpp to do its editing, the function
that handles keypresses is nsPlaintextEditor::HandleKeyPress which calls
aKeyEvent->PreventDefault() to stops the space key from scrolling the page. I
believe we need to do this for all elements that need to prevent scrolling. Then
we can get rid of this special code in the xbl prototype handler.

Comment 22

[Nian Liu(n/a in a long time)]

there are several things.
1.for checkbox. patch code got executed before nsHTMLInputElement::DoSetChecked.
so we can't use aEvent->PreventDefault() to stop propagation.
2.for textarea. there is an nsTextEditorKeyListener binded with it. so
aEvent->PreDefault() works.

i think the root cause is that Frame should get the event before
nsGenericHTMLElement::HandleDOMEvent

Comment 23

[[e-mail]]

DoSetChecked gets called on key up...

Comment 24

[Frank Wein [:mcsmurf]]

Comment on attachment 147114 [details] [diff] [review]
patch for 214843

cancel obsolete review request