Closed Bug 256269 Opened 16 years ago Closed 16 years ago

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

Categories

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

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: ginnchen+exoracle, Assigned: ginnchen+exoracle)

References

Details

(Keywords: access, Whiteboard: sunport17)

Attachments

(1 file, 3 obsolete files)

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.
Assignee: aaronleventhal → ginn.chen
Summary: up/down arrow should not get into button text → Caret browsing: up/down arrow should not get into button text
Whiteboard: sunport17
The expected result is selection of the button, just like tabbing into it.

Prog.
OS: Linux → All
also fixed bug 241038
Attachment #176545 - Flags: review?(aaronleventhal)
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?
Attachment #176545 - Flags: review?(aaronleventhal) → review?(mats.palmgren)
(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 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;
Attachment #176545 - Flags: review?(mats.palmgren) → review-
Attached patch patch revised (obsolete) — Splinter Review
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.
Attachment #176545 - Attachment is obsolete: true
Attachment #176716 - Flags: review?(mats.palmgren)
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.
Attachment #176716 - Attachment is obsolete: true
Attachment #176716 - Flags: review?(mats.palmgren)
The problem is the first hunk.
I think we can remove this hunk.
I'll continue work on it tomorrow.
Attached patch patch v2 (obsolete) — Splinter Review
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!
Attachment #177834 - Flags: review?(mats.palmgren)
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.
Attachment #177834 - Flags: review?(mats.palmgren) → review+
Attached patch patch v3Splinter Review
addressed comments of Mats.
Attachment #177834 - Attachment is obsolete: true
Attachment #178774 - Flags: superreview?(roc)
Attachment #178774 - Flags: review?(mats.palmgren)
Attachment #178774 - Flags: review?(mats.palmgren) → review+
Attachment #178774 - Flags: superreview?(roc) → superreview+
Checking in nsFrame.cpp;
/cvsroot/mozilla/layout/generic/nsFrame.cpp,v  <--  nsFrame.cpp
new revision: 3.552; previous revision: 3.551
done
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
*** Bug 241038 has been marked as a duplicate of this bug. ***
Component: Keyboard: Navigation → User events and focus handling
You need to log in before you can comment on or make changes to this bug.