256269 - Caret browsing: up/down arrow should not get into button text

Status: RESOLVED FIXED

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

Description

[Ginn Chen]

go to http://bugzilla.mozilla.org

find " Enter a bug # or some search terms:"
put caret in the word "search"(above button "Show")
press down arrow, caret get into button "Show"
you can use left/right move caret in button text now.

Comment 1

[Prognathous]

The expected result is selection of the button, just like tabbing into it.

Prog.

Comment 2

[Ginn Chen]

Attachment: don't let caret goes into input or button

also fixed bug 241038

Comment 3

[Aaron Leventhal]

Comment on attachment 176545 [details] [diff] [review]
don't let caret goes into input or button

Mats, would you be willing to look at this?

Comment 4

[Mats Palmgren (inactive)]

(In reply to comment #1)
> The expected result is selection of the button, just like tabbing into it.

That can be handled in a separate bug IMO.

Comment 5

[Mats Palmgren (inactive)]

Comment on attachment 176545 [details] [diff] [review]
don't let caret goes into input or button

>Index: nsFrame.cpp
>+
>+  // External while(!found)
>   while (!found)

Commenting block start/end like this is not useful IMO and
I haven't seen that in other code either.

> #ifdef IBMBIDI
>       result = it->FindFrameAt(searchingLine, newDesiredX, aPresContext->BidiEnabled(), &resultFrame, &isBeforeFirstFrame, &isAfterLastFrame);
> #else
>       result = it->FindFrameAt(searchingLine, newDesiredX, &resultFrame, &isBeforeFirstFrame, &isAfterLastFrame);
> #endif // IBMBIDI
>-      if(NS_FAILED(result))
>+
>+      while ( resultFrame && ( resultFrame->GetType() == nsLayoutAtoms::gfxButtonControlFrame || resultFrame->GetType() == nsLayoutAtoms::textInputFrame ) )
>+      {
>+        result = it->GetNextSiblingOnLine(resultFrame, searchingLine);
>+        if (NS_FAILED(result) || !resultFrame)
>+          break;
>+      }
>+
>+      if (NS_FAILED(result) || !resultFrame)
>         continue;

If you wrote "while (NS_SUCCEEDED(result) && resultFrame && ...)" you wouldn't
need the 'if' inside the loop.
Also, why test for 'nsLayoutAtoms::textInputFrame' ?
(I don't see a problem moving the caret inside an <input>.)


>-        if (!resultFrame->HasView())
>-        {
>-          rect = resultFrame->GetRect();
>-          if (!rect.width || !rect.height)
>-            result = NS_ERROR_FAILURE;
>-          else
>-            result = resultFrame->GetContentAndOffsetsFromPoint(context,point,
>-                                          getter_AddRefs(aPos->mResultContent),
>-                                          aPos->mContentOffset,
>-                                          aPos->mContentOffsetEnd,
>-                                          aPos->mPreferLeft);
>-        }
>+        rect = resultFrame->GetRect();
>+        if (!rect.width || !rect.height)
>+          result = NS_ERROR_FAILURE;
>+        else
>+          result = resultFrame->GetContentAndOffsetsFromPoint(context,point,
>+                                        getter_AddRefs(aPos->mResultContent),
>+                                        aPos->mContentOffset,
>+                                        aPos->mContentOffsetEnd,
>+                                            aPos->mPreferLeft);
>+

I'm not sure if removing "if (!resultFrame->HasView())" here is a good thing -
it seems to me that this would affect a lot more than <button>s.
Don't you need to translate the coordinates to the new view for example?
Besides - do you really need this change? if so, why?

The next two hunks where you test "IsSelectable()" only inside
"if (NS_SUCCEEDED(result))" seems reasonable.

In fact, couldn't you change the first hunk to use "IsSelectable()" too?
Something like:

      while (NS_SUCCEEDED(result) && resultFrame) {
	PRBool selectable = PR_TRUE;
	resultFrame->IsSelectable(&selectable, nsnull);
	if (selectable) {
	  break;
	} else {
	  result = it->GetNextSiblingOnLine(resultFrame, searchingLine);
	}
      }
      if (NS_FAILED(result) || !resultFrame)
	continue;

Comment 6

[Ginn Chen]

Attachment: patch revised

Thank you very much, Mats!

I revised the patch to address your comments.

In the last patch, I removed 'if (!resultFrame->HasView())' to resolve bug
241038.
With this line, caret is blocked by plugins.
I think to add 'else result = NS_ERROR_FAILURE' is more reasonable.

Comment 7

[Ginn Chen]

Comment on attachment 176716 [details] [diff] [review]
patch revised

With this patch, on www.mozilla.org, press ctrl+home and then down arrow,
mozilla hangs.
But with the first version mozilla won't hang.

Comment 8

[Ginn Chen]

The problem is the first hunk.
I think we can remove this hunk.
I'll continue work on it tomorrow.

Comment 9

[Ginn Chen]

Attachment: patch v2

Removed first hunk.

The only case it doesn't work is <button> in <TH>, it's another story.
I'll not fix it in this patch.

Mats, please review it again. Thanks!

Comment 10

[Mats Palmgren (inactive)]

Comment on attachment 177834 [details] [diff] [review]
patch v2

>Index: nsFrame.cpp
>-      if(NS_FAILED(result))
>+      if (NS_FAILED(result) || !resultFrame)

I don't think this change is needed anymore.

>+        else
>+          result = NS_ERROR_FAILURE;
>+

The code would be more readable if you move the "IsSelectable" test
inside the "!resultFrame->HasView()" block instead, like so:

   if (!resultFrame->HasView())
   {
     [...]

     if (NS_SUCCEEDED(result))
     {
       PRBool selectable;
       resultFrame->IsSelectable(&selectable, nsnull);
       if (selectable)
       {
	 found = PR_TRUE;
	 break;
       }
     }
   }

   if (aPos->mDirection == eDirPrevious && (resultFrame == farStoppingFrame))
   ...

The rest looks fine, r=mats with the above addressed.

Comment 11

[Ginn Chen]

Attachment: patch v3

addressed comments of Mats.

Comment 12

[Ginn Chen]

Checking in nsFrame.cpp;
/cvsroot/mozilla/layout/generic/nsFrame.cpp,v  <--  nsFrame.cpp
new revision: 3.552; previous revision: 3.551
done

Comment 13

[Ginn Chen]

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

Comment 14

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381