334626 - Arrow keys move caret inconsistently in and around floats

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Uri Bernstein]

Caret movement near floats (using left/right arrow keys) is a mess, especially when the float is at the beginning or end of a line.

When the float is at the middle of a line, the rule seems to be that you can't get the caret into the float using arrow keys (the caret just skips the float).
If the caret is already inside the float, you can exit the float going forwards, but not going backwards.

When the float is at a line beginning/end, the situation becomes even more complex. you can sometimes enter the float in one direction but not in the other, and exiting the float can put you in unexpected places.

We need to decide on a policy, and implement it consistently. Specifically:
1. Should the caret be allowed to enter floats, or should it skip them?
2. If the caret is inside a float, should it be allowed to escape it?

Testcase coming up.

Comment 1

[Uri Bernstein]

Attachment: testcase

This testcase demonstrates several caret issues with floats.
Turn on caret browsing, and try moving into, and out of, each of the floats, in both directions.

Comment 2

[Uri Bernstein]

I assume this is All/All.

And sorry for the stupid typo in the testcase ("brousing").

Comment 3

[Uri Bernstein]

Just found this:
http://www.mozilla.org/access/keyboard/proposal#X._Unresolved_Issues

Ginn, are you still interested in this? Any thoughts?

Comment 4

[Ginn Chen]

No, I won't work on Keyboard navigation until Bug 333492 is done.

Comment 5

[Uri Bernstein]

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

Comment 6

[Uri Bernstein]

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

Comment 7

[Uri Bernstein]

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

Comment 8

[Uri Bernstein]

Attachment: patch v1

You should be able to enter and exit floats (and absolute/fixed positioned elements) freely. To implement this, the LEAF and VISUAL iterators need to handle replacement frames for out-of-flows, like the FOCUS iterator does.

This patch is pretty much a re-write of nsFrameTraversal.cpp, unifying the Next() and Prev() methods of all the iterators, using derived classes to implement the "out-of-flow awareness" and "bidi visual" features, and sorting out the three types of traversal (leaf, pre-order, and post-order). The ridiculous notion of an "extensive" leaf iterator, which isn't a leaf iterator at all, was removed, of course.

Notice that I'm using multiple inheritance (for nsVisualOOFAwareIterator). This is not strictly necessary, because nsVisualIterator is (currently) never used as a concrete class, but I feel that doing it this way is more elegant.
Due to the use of virtual base classes, I had to use NS_DECL_ISUPPORTS/NS_IMPL_ISUPPORTS2 on each of the classes, or I would get warnings when freeing the objects. I'm not sure why that happened, and I hope my solution is OK.
Also, I replaced the parametrized constructors with Init() methods, in order not to rely on the order in which constructors are called.

Suggestions for better names (e.g. for the "OOFAware" classes, and the "Inner" methods) are welcome.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How nasty would it be to use a boolean to control following of out-of-flows instead of virtual base classes? I'd *really* like to avoid VBCs

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Since you're doing this rewrite, are there any interface changes we should consider? maybe deCOMtaminate it? nsIEnumerator is deprecated.

Comment 11

[Uri Bernstein]

(In reply to comment #9)
> How nasty would it be to use a boolean to control following of out-of-flows
> instead of virtual base classes? I'd *really* like to avoid VBCs
> 

It would be much less elegant, but probably not very difficult to do. Out of curiosity, why are VBCs considered such a bad thing?

(In reply to comment #10)
> Since you're doing this rewrite, are there any interface changes we should
> consider? maybe deCOMtaminate it? nsIEnumerator is deprecated.
> 

This is used by stuff outside layout (i.e., typeahead find, spacial navigation). Doesn't that mean that it should have some COM interface? If so, what's the alternative to nsIEnumerator? I'm willing to do the work here (as far as my time allows me), but I'm not sure I'm capable of doing the design myself.

Comment 12

[Uri Bernstein]

(In reply to comment #9)
> How nasty would it be to use a boolean to control following of out-of-flows
> instead of virtual base classes? I'd *really* like to avoid VBCs
> 

Alternatively, since the visual-but-not-following-OOFs iterator is never actually used, I could just make the visual iterator derive from the following-OOFs one (and implement the "visualness" directly on it). That will get rid of VBCa while avoiding the boolean, with the price of not having a visual-but-not-following-OOFs iterator if we ever need it.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If we had the following-OOF behaviour controlled by a flag, we could control that with a method, and have more functionality with less classes.

VBCs easily get really complex. If we use them, that'd be the first use in Mozilla code. Let's not set a precedent and go down that road unless there's a huge win.

Comment 14

[Uri Bernstein]

(In reply to comment #13)
> If we had the following-OOF behaviour controlled by a flag, we could control
> that with a method, and have more functionality with less classes.

I don't see the benefit of controlling it with a method, since I can't imagine a case were we'd want to change it after the iterator is initialized.
Regarding "having more functionality with less classes" - this argument could be applied to the "visual" behavior as well (which could also be controlled by a flag). Would you consider having just one class, with flags for everything, a better design? To me it seems like we'd just be giving up the benefits of polymorphism.

I do see your point with VBCs, so they're out of the question.

I'll happily do this any way you prefer. I'm just trying to better understand the motivation.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

My thinking is that whether to descend into out-of-flows is a parameter entirely orthogonal to the other parameters of frame traversal, so it should be specified as an extra parameter to NS_NewFrameTraversal, which would be best implemented by setting a flag in the traversal object.

Now it seems to me that the code using nsIFrameTraversal, especially the users outside layout in TAF and spatial navigation, really should be doing content traversal instead, plus using getClientRects (when that lands) to determine the true geometry of elements. So it's probably not worth fixing up the rest of the interface.

Comment 16

[Uri Bernstein]

Attachment: patch v2 (with interface changes)

This gets rid of VBCs (using a flag to indicate "descends into OOFs"). It also changes the interfaces to nsIFrameTraversal::NewFrameTraversal and NS_NewFrameTraversal, replacing the limited set of possible "traversal types" (LEAF, VISUAL, etc.) by a set of parameters that allow users to fully control the attributes of the iterator (including "visualness" and descending into OOFs).

I hope this is more or less what you had in mind. If not, I also have a version of this patch which does not change the interfaces.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 235639 [details] [diff] [review]
patch v2 (with interface changes)

This is exactly what I had in mind!

It's hard to check the logic of the traversal, but as far as I can tell it's OK.

+  PRBool mLockScroll;
+  PRBool mFollowOOFs;

PRPackedBool

+            mLockScroll && result->GetType() == nsLayoutAtoms::scrollFrame) {

I'd prefer you to parenthesize the && here

+  nsIFrame* next;
+  while ((next = result->GetNextSibling())) {
+    result = next;
   }

You can use an nsFrameList to do this for you.

Comment 18

[Uri Bernstein]

Attachment: Patch for checkin

This is what I'm about to check in, with all of ROC's comments addressed.

Comment 19

[Uri Bernstein]

Checked in.

BTW, regarding the logic of the traversal: I did my best to ensure that I understand it and it's correct, consistent, and symmetrical. I also did some testing, of course, but it's difficult to test all the combinations and situations.

Checking in layout/base/nsIFrameTraversal.h;
/cvsroot/mozilla/layout/base/nsIFrameTraversal.h,v  <--  nsIFrameTraversal.h
new revision: 1.10; previous revision: 1.9
done
Checking in layout/base/nsFrameTraversal.h;
/cvsroot/mozilla/layout/base/nsFrameTraversal.h,v  <--  nsFrameTraversal.h
new revision: 3.12; previous revision: 3.11
done
Checking in layout/base/nsFrameTraversal.cpp;
/cvsroot/mozilla/layout/base/nsFrameTraversal.cpp,v  <--  nsFrameTraversal.cpp
new revision: 3.52; previous revision: 3.51
done
Checking in layout/generic/nsFrame.cpp;
/cvsroot/mozilla/layout/generic/nsFrame.cpp,v  <--  nsFrame.cpp
new revision: 3.669; previous revision: 3.668
done
Checking in layout/generic/nsSelection.cpp;
/cvsroot/mozilla/layout/generic/nsSelection.cpp,v  <--  nsSelection.cpp
new revision: 3.261; previous revision: 3.260
done
Checking in content/events/src/nsEventStateManager.cpp;
/cvsroot/mozilla/content/events/src/nsEventStateManager.cpp,v  <--  nsEventStateManager.cpp
new revision: 1.667; previous revision: 1.666
done
Checking in extensions/typeaheadfind/src/nsTypeAheadFind.cpp;
/cvsroot/mozilla/extensions/typeaheadfind/src/nsTypeAheadFind.cpp,v  <--  nsTypeAheadFind.cpp
new revision: 1.121; previous revision: 1.120
done
Checking in extensions/spatialnavigation/src/nsSpatialNavigation.cpp;
/cvsroot/mozilla/extensions/spatialnavigation/src/nsSpatialNavigation.cpp,v  <--  nsSpatialNavigation.cpp
new revision: 1.12; previous revision: 1.11
done
Checking in extensions/spatialnavigation/src/nsSpatialNavigationPrivate.h;
/cvsroot/mozilla/extensions/spatialnavigation/src/nsSpatialNavigationPrivate.h,v  <--  nsSpatialNavigationPrivate.h
new revision: 1.6; previous revision: 1.5
done
Checking in extensions/spatialnavigation/src/nsSpatialNavigationUtils.cpp;
/cvsroot/mozilla/extensions/spatialnavigation/src/nsSpatialNavigationUtils.cpp,v  <--  nsSpatialNavigationUtils.cpp
new revision: 1.11; previous revision: 1.10
done
Checking in toolkit/components/typeaheadfind/src/nsTypeAheadFind.cpp;
/cvsroot/mozilla/toolkit/components/typeaheadfind/src/nsTypeAheadFind.cpp,v  <--  nsTypeAheadFind.cpp
new revision: 1.27; previous revision: 1.26
done