363198 - Activating a link doesn't put the caret in the expected position

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Willie Walker]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20061208 Minefield/3.0a1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20061208 Minefield/3.0a1

When I activate a link in Firefox 3 (nightly as of afternoon 08-DEC EST), the page scrolls to the right spot, but the caret ends up somewhere else.

Reproducible: Always

Steps to Reproduce:
1. Enable caret navigation
2. Go to http://cvs.gnome.org/viewcvs/*checkout*/orca/docs/doc-set/orca.html
3. Arrow down to the first link on the page ("Orca User Experience Design")
4. Press Enter - the page scrolls to the right spot
5. Press Down - the page jumps to section 3.1.

Actual Results:  
After the link has been 'activated', the page scrolls to the correct spot, but pressing 'Down' arrow after that causes the page to jump to a most unexpected position.  

Expected Results:  
When the link is activated, both the page should scroll to the right spot *and* the caret should be positioned appropriately (I'm going to guess 'appropriately' here means at the first visual thing in the target).

I searched the 200 some odd bugs that had 'caret' in their summary, but I didn't seem to find one similar to this.  Sorry if this is a duplicate.

Comment 1

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

i guess content in the url above is changed?

Comment 2

[Willie Walker]

(In reply to comment #1)
> i guess content in the url above is changed?
> 

What are (or are not) you seeing?  I just checked and it's still there.

Comment 3

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

(In reply to comment #2)
> (In reply to comment #1)
> > i guess content in the url above is changed?
> > 
> 
> What are (or are not) you seeing?  I just checked and it's still there.
> 

i can't reproduce with the url and the steps you described.

Comment 4

[Willie Walker]

It happens for me 100% of the time.  Are you sure you are following the steps exactly as written?  You need to have caret navigation enabled, and you need to use the keyboard.  Notice that when you press "Enter" to activate the link, the page scrolls to the correct spot, but then when you then press the "Down" arrow, the caret jumps to a completely unexpected spot.  The exact line it ends up on has the following content:

You can override this setting in your ~/.orca/orca-customizations.py module if you wish.

If this isn't happening for you, where does the caret end up when you press the "Down" arrow just after you've pressed the "Enter" button to activate the link?

Comment 5

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

(In reply to comment #4)
> It happens for me 100% of the time.  Are you sure you are following the steps
> exactly as written?  You need to have caret navigation enabled, and you need to
> use the keyboard.  Notice that when you press "Enter" to activate the link, the
> page scrolls to the correct spot, but then when you then press the "Down"
> arrow, the caret jumps to a completely unexpected spot.  The exact line it ends
> up on has the following content:
> 
> You can override this setting in your ~/.orca/orca-customizations.py module if
> you wish.
> 
ah, suddenly I can reproduce it.

for the keyboard navigation, I don't think orca should depend on the firefox's implementation. besides on that, what can I help you on this bug?
> If this isn't happening for you, where does the caret end up when you press the
> "Down" arrow just after you've pressed the "Enter" button to activate the link?
> 

Comment 6

[Ginn Chen]

I think the root cause is the caret position is not set when the anchor is followed.
Press up/down arrow at this situation will act as ctrl+home/ctrl+end, i.e. to the top/bottom of the page.

837 NS_IMETHODIMP
838 nsTextInputSelectionImpl::LineMove(PRBool aForward, PRBool aExtend)
839 {
840   if (mFrameSelection)
841   {
842     nsresult result = mFrameSelection->LineMove(aForward, aExtend);
843     if (NS_FAILED(result))
844       result = CompleteMove(aForward,aExtend);
845     return result;
846   }
847   return NS_ERROR_NULL_POINTER;
848 

Comment 7

[Willie Walker]

> for the keyboard navigation, I don't think orca should depend on the firefox's
> implementation. besides on that, what can I help you on this bug?

We definitely cannot rely on Firefox's caret navigation implementation, which is why we're in the unfortunate situation of writing our own. Ideally, Firefox/Gecko would fix its implementation so more people would benefit from a usable solution. I'm not going to give up hope on that. Aaron has reminded me many times of how horrible Gecko's caret handling code is, however, and how ensnarled it is with the core workings of Gecko.  As such, it seems like it would really take a skilled engineer with the pride, knowledge, and determination to fix it.  Maybe that's one of you guys?  ;-)  You'd definitely become a hero in my eyes.

Having said that, we really could use some help from Firefox/Gecko here.  When someone activates a link, ideally Firefox/Gecko will get the navigation scheme right and will position the caret correctly.  If not, I guess we'll be in the unfortunately position of having to do that, too.  If we have to do that, what tools do I have in place to help me?  For example, I'd at least need this:

1) Something in the AT-SPI implementation that provides me with information to efficiently determine where the new caret position will be.  I'm not sure I really have this information, please let me know what it is if I'm wrong.

2) Something to let me tell Firefox/Gecko to scroll to the right spot.  Ideally, I could just set the caret position and the scroll would happen automatically (see bug 363200).  If that ends up being another 'too bad, gotta do it yourself' problem, do you have an idea for how we might be able to accomplish it?

Comment 8

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

> 1) Something in the AT-SPI implementation that provides me with information to
> efficiently determine where the new caret position will be.  I'm not sure I
> really have this information, please let me know what it is if I'm wrong.
can you provide some scenerios? then we can discuss what can be got by what.

> 
> 2) Something to let me tell Firefox/Gecko to scroll to the right spot. 
> Ideally, I could just set the caret position and the scroll would happen
> automatically (see bug 363200).  If that ends up being another 'too bad, gotta
> do it yourself' problem, do you have an idea for how we might be able to
> accomplish it?
> 
i just add a patch for bug 363200. you can use it as a tmp solution

Comment 9

[Ginn Chen]

(In reply to comment #8)
> i just add a patch for bug 363200. you can use it as a tmp solution
> 

I don't think it's related to this bug.

I didn't find any clue, why this happens to some anchors, but not others.
e.g. "Lee the Programmer" works.

Can we work out a minimal testcase?

Comment 10

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

(In reply to comment #9)
> (In reply to comment #8)
> > i just add a patch for bug 363200. you can use it as a tmp solution
> > 
> 
> I don't think it's related to this bug.

yep. i just double ref to let Willie know that setCaretOffset can scroll text into view with a tmp solution in bug 363200^_^
> 
> I didn't find any clue, why this happens to some anchors, but not others.
> e.g. "Lee the Programmer" works.
> 
> Can we work out a minimal testcase?
> 

Comment 11

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

willie, you should modify your html document. you have two anchor with same name "USEREXPERIENCE" which confuses firefox. with removing the useless(i think so) one after <div class="BOOK">, and firefox works fine

Comment 12

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

also modify as <A NAME="PERSONAS"></A>Chapter 2. Personas

to <A NAME="PERSONAS">Chapter 2. Personas</A>

Comment 13

[Willie Walker]

> > 1) Something in the AT-SPI implementation that provides me with information to
> > efficiently determine where the new caret position will be.  I'm not sure I
> > really have this information, please let me know what it is if I'm wrong.
> can you provide some scenerios? then we can discuss what can be got by what.

The main scenario is what is being reported by this bug, which is that the user activates a link and the caret ends up in the expected spot.

> willie, you should modify your html document. you have two anchor with same
> name "USEREXPERIENCE" which confuses firefox. with removing the useless
> (i think so) one after <div class="BOOK">, and firefox works fine
> ...
> also modify as <A NAME="PERSONAS"></A>Chapter 2. Personas
> to <A NAME="PERSONAS">Chapter 2. Personas</A>

Aha!  Thanks.  I'll give these a shot.  It's not going to be terribly easy, though, as the ultimate source for the HTML is DocBook.  I'll need to dig into what DocBook is doing to generate the suspect HTML.  

It still surprises me that Firefox will scroll to the right spot but apparently position the caret at the wrong spot.  From what Aaron has described to me regarding the caret code in Firefox, though, it sounds as though several things want to move the caret and they end up competing with each other.

Comment 14

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

(In reply to comment #13)

> The main scenario is what is being reported by this bug, which is that the user
> activates a link and the caret ends up in the expected spot.
if the html content is rightly described, there's no problem.

> It still surprises me that Firefox will scroll to the right spot but apparently
> position the caret at the wrong spot.  From what Aaron has described to me
> regarding the caret code in Firefox, though, it sounds as though several things
> want to move the caret and they end up competing with each other.
> 
To scroll to the right spot is because of the wrong node is sibling with right node and there is no visual space/distance between them.

To not set the caret right is because of anchor has no content to set caret on.


i really suggest that you dig into the html doc generator. it's not a bug of firefox currentlly.

Comment 15

[Willie Walker]

Thanks for the response! 

> > It still surprises me that Firefox will scroll to the right spot but apparently
> > position the caret at the wrong spot.  From what Aaron has described to me
> > regarding the caret code in Firefox, though, it sounds as though several things
> > want to move the caret and they end up competing with each other.
> > 
> To scroll to the right spot is because of the wrong node is sibling with right
> node and there is no visual space/distance between them.

I'm not sure what you're saying here.  Can you please expand?

> To not set the caret right is because of anchor has no content to set caret on.

It seems as though it should set the caret to the closest content.  Perhaps Firefox is trying to do this, but it just gets plain confused and puts the caret at some odd location?  I realize it's a pain to make Firefox try to accomodate unexpected content (we have to do similar things all the time with Orca - the Orca script for Gecko is approaching 10% of the overall Orca code base, for example), but it would be great if it were able to position the cursor a little better under these situations.
 
> i really suggest that you dig into the html doc generator. it's not a bug of
> firefox currentlly.

I'm digging into it.  It's a very popular environment called "DocBook", though  I'm afraid I might not have the facilities to make it generate HTML that Firefox can work with.  I will try, however, to fix the raw Orca DocBook pages to see if I can solve the specific problem with the Orca pages.  The Orca pages are only part of our testing, however, and resolving the problem with them doesn't resolve the problem people will experience with similar pages.

Comment 16

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

aaron, what's your opinion here. should firefox handle these scenenios?

Comment 17

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

(In reply to comment #15)
> Thanks for the response! 
> 

> > To scroll to the right spot is because of the wrong node is sibling with right
> > node and there is no visual space/distance between them.
> 
> I'm not sure what you're saying here.  Can you please expand?
scroll and show caret is two different steps. your case includes two scenerios.
1)two anchor node has the same name. the two nodes have same visula top-left point, so the scroll is correct.
2)anchor has no static text. scroll is to the right node.

however, when show caret on static text, either node of each scenerio has no static text. so firefox is confused.

> I'm digging into it.  It's a very popular environment called "DocBook", though 
> I'm afraid I might not have the facilities to make it generate HTML that
> Firefox can work with.  I will try, however, to fix the raw Orca DocBook pages
> to see if I can solve the specific problem with the Orca pages.  The Orca pages
> are only part of our testing, however, and resolving the problem with them
> doesn't resolve the problem people will experience with similar pages.
it's more like a bug of DocBook. if possible, pls. file bug on it.

Comment 18

[Aaron Leventhal]

Willie, you might try layout.selectanchor to true via about:config, as a temporary solution.

Nian, it seems like the code which tries to set the caret starts here:
http://lxr.mozilla.org/seamonkey/source/layout/base/nsPresShell.cpp#3866

Where does that fail? Is it intentional that we don't set the caret in that scenario? Please look at CVS blame and read any related bugs.

Otherwise, my instinct is that we should fix this. But, we should check with whoever has touched the code most recently.

Comment 19

[Willie Walker]

(In reply to comment #18)
> Willie, you might try layout.selectanchor to true via about:config, as a
> temporary solution.

Thanks Aaron!  I tried this with both Firefox 3.0a1 from 18-Dec and Firefox 3.0a2pre from 05-Jan (had to modify browser.xul in browser.jar to get Firefox to run -- someone forgot to close a <commandset> in the file) and the property didn't seem to exist.  When I created it and set it to true, I didn't see any difference.  

Is there a different propery somewhere?

Comment 20

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

> Nian, it seems like the code which tries to set the caret starts here:
> http://lxr.mozilla.org/seamonkey/source/layout/base/nsPresShell.cpp#3866
> 
aaron, pls. see my commnets 11 & 12. for willie's case, selection is set on an anchor which has no static text. and why the selection is set to the wrong anchor is because only html document content itself.

i don't think we can have a way to guess where to set caret next when these scenerios happen.

what's your opinion?

Comment 21

[Aaron Leventhal]

I agree with comment 11 about the duplicate name. That's a weird case.

I disagree with comment 12. It's quite common to have <a name="foo"></a> with no text inside. In that case, I *think* it should be okay to select the anchor node itself instead of any text inside (which there isn't any). So unless there's some specific reason we don't do that, I think we should be selecting that node.

Comment 22

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

Attachment: patch

when the content has no child node and offset equals 0, we should return current node frame not null.

Comment 23

[Aaron Leventhal]

Comment on attachment 251160 [details] [diff] [review]
patch

That's a very low level change, which could be risky. What else uses GetFrameForNodeOffset() and how might those things be affected? Are you sure this is correct?

Also, we would need to add a comment as to what the code is now doing and why.

Comment 24

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

(In reply to comment #23)
> (From update of attachment 251160 [details] [diff] [review])
> That's a very low level change, which could be risky. What else uses
> GetFrameForNodeOffset() and how might those things be affected? Are you sure
> this is correct?
> 
it's definely a bug to return nsnull when current node has no children and offset is zero.
> Also, we would need to add a comment as to what the code is now doing and why.
> 

Comment 25

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

aaron, can you find another guy to review the bug?

Comment 26

[[e-mail]]

Comment on attachment 251160 [details] [diff] [review]
patch

Sorry, I don't really understand what either the original code or this code is supposed to do.

Comment 27

[Mats Palmgren (inactive)]

Willie, if you want to workaround this bug in older UAs (eg. Firefox 2.*)
then you only need to add a space in the anchors, like so:
<A NAME="USEREXPERIENCE"> </A>

Comment 28

[Mats Palmgren (inactive)]

The Selection code depends heavily on GetFrameForNodeOffset() so I agree
with Aaron that this patch seems a bit risky. I think this bug can be
fixed one level up though, in nsCaret::GetCaretFrameForNodeOffset().
Uri knows this code better than me so I suggest him as reviewer.

Comment 29

[Mats Palmgren (inactive)]

Attachment: Alternative patch

Comment 30

[Uri Bernstein]

I think GetFrameForNodeOffset() is broken in several ways, and should be fixed once and for all, even if doing so is risky. I actually started working on this (in the context of bug 366720), but my bandwidth is a bit narrow right now. What I'm planning is a bit more radical than what Nian Liu is suggesting.

Give me a couple of weeks to try and complete what I'm doing. If I can't produce anything within this time frame, we can reconsider one of the patches suggested here.

Comment 31

[Aaron Leventhal]

Uri, is it possible that we get one of these in as a stopgap so that the screen reader developers at Sun and IBM can continue working on their testcases and code and move past this issue?

If the refactoring happens later -- that's fine.

Comment 32

[Uri Bernstein]

Comment on attachment 251160 [details] [diff] [review]
patch

OK... For now, I tend to go with the original patch, because I can see at least one codepath that should be affected, and doesn't go through GetCaretFrameForNodeOffset (in nsTypedSelection::GetPrimaryFrameForFocusNode(), if aVisual==PR_FALSE).
I also note that I proposed a similar change in my patch for bug 16311, but then reverted it in bug 305239, because it seemed to no longer be necessary (bug 305239 comment 11).

> +    if (aOffset != 0 || numChildren != 0) {

Why can't this just be |if (numChildren != 0)|?

Also, I'd like to do some testing with this patch to try and catch any possible major breakage. I'll only be able to do that tomorrow, and if I'm happy with the results, I'll r+ the patch.

Comment 33

[Uri Bernstein]

Comment on attachment 251160 [details] [diff] [review]
patch

After doing some testing, and reviewing the callers of GetFrameForNodeOffset(), I'm comfortable with this patch. I guess that testing also for the offset to be 0 if numChildren == 0 (and failing otherwise) is reasonable.

Two comments:

>+    if (aOffset != 0 || numChildren != 0) {

1. Please use |childIndex|, not |aOffset | for the test (aOffset might be originally 1, and adjusted to 0 - which is a legitimate case we don't want to fail in).

2. Please use ">" instead of "!=" in both tests, since both childIndex and numChildren are guaranteed to be non-negative at this point.

r=me with that.

Comment 34

[Uri Bernstein]

(In reply to comment #7)

> We definitely cannot rely on Firefox's caret navigation implementation, which
> is why we're in the unfortunate situation of writing our own. Ideally,
> Firefox/Gecko would fix its implementation so more people would benefit from a
> usable solution. I'm not going to give up hope on that. Aaron has reminded me
> many times of how horrible Gecko's caret handling code is, however, and how
> ensnarled it is with the core workings of Gecko.

Please specify bug numbers for the problems that hurt you most, or, if they are not yet filed as bugs, please file them (and CC me).
It's much easier for engineers to fix stuff if they know exactly what's broken.

Comment 35

[Aaron Leventhal]

Uri, I have the caret navigation bugs filed in a meta bug: https://bugzilla.mozilla.org/show_bug.cgi?id=caretnav

However, only William Walker from Sun can probably tell you which of those is most critical.

I had told Will there wasn't much hope of fixing them all in his time frame, once you had moved to Google. My understanding was that you probably wouldn't be available much. No one else qualified has been interested and able to fix all of these caret and selection bugs. But it's good to see you around again!

Comment 36

[Willie Walker]

> Please specify bug numbers for the problems that hurt you most, or, if they are
> not yet filed as bugs, please file them (and CC me).
> It's much easier for engineers to fix stuff if they know exactly what's broken.

It's great to hear that someone capable of fixing the problems cares and is willing to help!  Yeah!

I just got back from a week out of town, and I'm due to leave again in a week.  I'll ask the team members to help with identifying/prioritizing these bugs, though.  You've given me renewed hope.  Thanks!

Comment 37

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

Attachment: patch

patch addressing uri's comment

Comment 38

[Uri Bernstein]

Comment on attachment 253138 [details] [diff] [review]
patch

I'm not a super-reviewer. Requesting sr from roc instead.

Comment 39

[Uri Bernstein]

Checked in:
Checking in mozilla/layout/generic/nsSelection.cpp;
/cvsroot/mozilla/layout/generic/nsSelection.cpp,v  <--  nsSelection.cpp
new revision: 3.274; previous revision: 3.273
done

Comment 40

[Uri Bernstein]

Checked in.

Comment 41

[Steve England [:stevee]]

Could this have caused bug 368893?

Comment 42

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

(In reply to comment #41)
> Could this have caused bug 368893?
> 

backout with my local workspace show that 368893 still exists

Comment 43

[Willie Walker]

I still haven't had time to dig into each caret navigation bug yet.  I have a suggestion for how you can experience the problems first hand, however: unplug your mouse, enable caret navigation and navigate around Firefox via the keyboard.  Try to do this for a whole day.

This suggestion isn't meant to be snarky...I think you'll quickly see the problems if you force yourself to abandon the mouse.

For example, try arrowing around the text in this bug report, try paging up and down, try giving focus to the "Additional Comments" text area on this page, try activating a link, etc.  Ask yourself these questions:

1) Is it efficient?  (My answer is "no," but that is often due to the web page author and/or Firefox putting the caret in unexpected spots.)

2) Does the caret go where I expect? (My answer is "sometimes not" - I have no idea how or why Firefox decides to jump the caret suddenly to the end/middle/top of the page.)

3) Can I get to everything I need to get to?  (My answer is "usually, but with a lot of keyboarding" but I don't always grok the focus mechanism or understand how to work with some components such as comboboxes.)

4) Does focus really occur when it is supposed to?  (For text areas, I can position the caret in them using the arrow keys, but they often don't take focus. I need to tab out and tab back in to get them to take focus.  Tabbing out often seems to take me to new and strange places.)

5) When I've positioned the caret somewhere, do Tab and Shift+Tab take me where I expect?  (As mentioned above, it often doesn't seem to take me anywhere that's in reasonable relationship to the caret - it will jump to the top, jump to the bottom.)

I realize these types of nondescript bugs are a pain, but I think there might be so many issues to raise that it's best if you just try it out.  In addition, the problems often seem intermittent: I can position the caret reliably sometimes, but fail other times.  As such, trying it yourself for a day would probably be very illuminating.

In the meantime, I'll try to find some more specific examples, and I thank you all for your energy and commitment to accessibilty.

Thanks!

Will

Comment 44

[Uri Bernstein]

(In reply to comment #43)
> I still haven't had time to dig into each caret navigation bug yet.  I have a
> suggestion for how you can experience the problems first hand, however: unplug
> your mouse, enable caret navigation and navigate around Firefox via the
> keyboard.  Try to do this for a whole day.
> 

Unfortunately, I don't have the time (and frankly, the motivation) to do this. Using the mouse has long become a second nature to me, and I imagine I'll get frustrated very quickly by not having it, even if I don't encounter any bugs at all.

Since you (unlike me) seem to encounter these problems during your normal work, I think it would be much more productive if you report the problems, and I (and other Mozilla engineers) then try to fix them.

What you should do, whenever you encounter one of the bugs/annoyances you describe above, is:
- Try to remember exactly what you did, and what happened.
- See if you can reproduce the problem.
- (Preferably) create a minimal test case.
- Submit a bug, with the URL of the page on which you encountered the problem (and the testcase, if you made one), and precise instructions for reproducing the problem.
- CC me on the bug report.

Even then, I can't promise I'll fix everything (or anything), given the limited time I now have for working on Mozilla. But I do promise to give each bug at least some investigation.

Comment 45

[Willie Walker]

> I don't have the time (and frankly, the motivation) to do this.

Bummer. If you were to even try for 20 minutes just to navigate this bug report -- only 20 minutes -- you'd probably see the majority of the problems I'm seeing.

> I imagine I'll get frustrated very quickly

That's my point.  Now imagine you need to use only the keyboard all day every day.   Good, reliable, predictable caret navigation would certainly help reduce the frustration.  

In any case, we're bumping heads here and that's not what I'm trying to do.  As time frees up, I'll take a look at the bugs already logged and comment.

Comment 46

[Willie Walker]

I added a comment to bug 205846 (https://bugzilla.mozilla.org/show_bug.cgi?id=205846).  It appears to expose the worst of the reliability and predictability problems I've been seeing. If you solve these, you'll have taken Firefox to a whole new level of usability.  Thanks!