Arrow keys move caret inconsistently in and around floats

RESOLVED FIXED in mozilla1.9alpha1

Status

()

RESOLVED FIXED
13 years ago
10 years ago

People

(Reporter: uriber, Assigned: uriber)

Tracking

({testcase})

Trunk
mozilla1.9alpha1
testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

13 years ago
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.
(Assignee)

Comment 1

13 years ago
Created attachment 218974 [details]
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.
(Assignee)

Comment 2

13 years ago
I assume this is All/All.

And sorry for the stupid typo in the testcase ("brousing").
Keywords: testcase
OS: MacOS X → All
Hardware: Macintosh → All
(Assignee)

Comment 3

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

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

Comment 4

13 years ago
No, I won't work on Keyboard navigation until Bug 333492 is done.
(Assignee)

Updated

12 years ago
Blocks: 241023
(Assignee)

Comment 5

12 years ago
*** Bug 257509 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 6

12 years ago
*** Bug 335617 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 7

12 years ago
*** Bug 331539 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 8

12 years ago
Created attachment 234748 [details] [diff] [review]
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.
Assignee: selection → uriber
Status: NEW → ASSIGNED
Attachment #234748 - Flags: review?(roc)
(Assignee)

Updated

12 years ago
Blocks: 140644
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
Since you're doing this rewrite, are there any interface changes we should consider? maybe deCOMtaminate it? nsIEnumerator is deprecated.
(Assignee)

Comment 11

12 years ago
(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.
(Assignee)

Comment 12

12 years ago
(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.
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.
(Assignee)

Comment 14

12 years ago
(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.
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.
(Assignee)

Comment 16

12 years ago
Created attachment 235639 [details] [diff] [review]
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.
Attachment #234748 - Attachment is obsolete: true
Attachment #235639 - Flags: review?(roc)
Attachment #234748 - Flags: review?(roc)
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.
Attachment #235639 - Flags: superreview+
Attachment #235639 - Flags: review?(roc)
Attachment #235639 - Flags: review+
(Assignee)

Comment 18

12 years ago
Created attachment 236069 [details] [diff] [review]
Patch for checkin

This is what I'm about to check in, with all of ROC's comments addressed.
Attachment #235639 - Attachment is obsolete: true
(Assignee)

Comment 19

12 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha
Depends on: 351292
You need to log in before you can comment on or make changes to this bug.