144000 - Caret browsing is not available if there is no selection ranges

Status: NEW

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

Description

[dmitry]

Now upon pressing f7 and then some arrow keys, the caret is always at the bottom
of a page. Correct behavior is position it at the topmost currently visible line
(i.e. at the top of the current screen). This way, if I scrolled to a middle of
a long document I can switch caret on and make some selections without losing my
position.

Comment 1

[Matthias Versen [:Matti]]

-> Keyboard Navigation

Comment 2

[dmitry]

More points to consider:

After you move caret to some point on a page, switch it off, then scroll away
from that place and switch caret on again, it emerges in the place where you
left it, which may not be visible in your new scroll position. This is really a
bad idea, because:

- without seeing the caret I have no visual clue if f7 worked at all or not;

- if I then press an arrow key, I'm suddenly jumped to a quite different
position in the page, which is (most likely) not what I need;

- the old bug 4302 which results in a caret staying at the same point when you
scroll by pgdn/pgup has a chance to be finally fixed soon (if not already), and
so the caret emerging outside of the currently visible portion of the page
should be similarly fixed (these two problems are similar from the end user
perspective).

Comment 3

[Boris Zbarsky [:bzbarsky]]

The caret is always there (try shift-arrow in non-caret-browsing mode).  It's
just hidden when you are not using caret browsing.....

Comment 4

[brewthatistrue]

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

Comment 5

[Ginn Chen]

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

Comment 6

[Ginn Chen]

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

Comment 7

[Rick Ju]

The root problem is nsSelection don't have a focusNode yet at that time. How
about setting an initial pos for nsSelection in nsPresShell::EndLoad?

This can resolve most website's problems. Some sites still don't work because
CompleteMove can't always find first selectable content, e.g. www.sun.com

diff -u -3 -p -u -r3.756 nsPresShell.cpp
--- nsPresShell.cpp     10 Aug 2004 03:24:41 -0000      3.756
+++ nsPresShell.cpp     18 Aug 2004 07:15:18 -0000
@@ -3635,6 +3635,9 @@ PresShell::EndLoad(nsIDocument *aDocumen
   MOZ_TIMER_DEBUGLOG(("Stop: Style Resolution: PresShell::EndLoad(),
this=%p\n", this));
 #endif
   mDocumentLoading = PR_FALSE;
+
+  // move selection initial pos to doc begin
+  CompleteMove( PR_FALSE, PR_FALSE );
 }

Comment 8

[Ginn Chen]

Attachment: patch

CompleteMove is not very stable, it doesn't work with some sites with
css:float.
So I'd like to call CompleteMove(PR_FALSE, PR_FALSE); before making the first
caret movement.
Add CheckInitialPostion() for this purpose.

Comment 9

[Aaron Leventhal]

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

Oops, Ginn. That was the wrong patch.

Comment 10

[Ginn Chen]

Attachment: patch

sorry for that

Comment 11

[Aaron Leventhal]

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

This looks like it will leak:
+ nsIDOMNode* focusNode;
+ domselection->GetFocusNode(&focusNode);

In general this looks like the wrong place to fix the problem. For example, if
F7 is pressed the caret should be visible right away. This doesn't necessarily
appear to be the case with this patch.

Can we reset the selection to the top of the document in
nsEventStateManager::SendFocusBlur() when focus is getting set to the document
itself (focus content is null)?

Comment 12

[Ginn Chen]

Attachment: patch v2

What about this one?
nsEventStateManager::GetBrowseWithCaret() doesn't work here.

Comment 13

[Aaron Leventhal]

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

We definitely don't want to do this for XUL documents.

Let's ask Neil where to hook in. If you are going to put it in EndLoad maybe it
should go in nsHTMLDocument::EndLoad. Or perhaps it belongs as a response to
document focus somewhere in nsEventStateManager, either in PreHandleEvent,
PostHandleEvent, SendFocusBlur or SetContentState(focus). In any case it will
be better if you get one of the top architects to tell you where to put this
code.

Comment 14

[Aaron Leventhal]

I just spoke with [e-mail] on IRC and his first thought is also to
put this in nsEventStateManager.

Comment 15

[Ginn Chen]

Attachment: patch v3

What about put the hunk here?
I think it's much better than patch v2.

Comment 16

[Aaron Leventhal]

Comment on attachment 177843 [details] [diff] [review]
patch v3

Much better, in my opinion.

Comment 17

[Ginn Chen]

Neil, do you have time to review this?

Comment 18

[Boris Zbarsky [:bzbarsky]]

I don't really feel qualified to review this patch.  If you do want me to review it, I have to warn you that I won't have time to try to learn this code until at least January...

Comment 19

[Ginn Chen]

January is still better than no review and comment in last 8 months

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 177843 [details] [diff] [review]
patch v3

So I'm not sure I follow.  does this actually scroll the document (per the documentation on nsISelectionController)?  That is, will enabling caret browsing snap the document to top with this patch?

If not, what _does_ this patch do?

Comment 21

[Ginn Chen]

(In reply to comment #20)
> (From update of attachment 177843 [details] [diff] [review] [edit])
> So I'm not sure I follow.  does this actually scroll the document (per the
> documentation on nsISelectionController)?  That is, will enabling caret
> browsing snap the document to top with this patch?

Yes, as same as press Ctrl+Home with caret browsing enabled.
But if the document has focus, e.g. load with an anchor, the document will not be scrolled.

Comment 22

[Boris Zbarsky [:bzbarsky]]

That's not what this bug was about.  This bug was about putting the caret at the current scroll position, no?  It seems to me that the behavior of snapping to the very beginning would be very inconvenient -- almost worse than the behavior we have now (where you can at least reposition the caret to where you are manually after entering caret mode).

Comment 23

[Ginn Chen]

Agree.
It's also inconvenience to press Ctrl+Home manually for every new loaded page when user enabled caret browsing.

It will be better if we can fix this bug as the bug description,
to put the caret to the topmost currently visible line.

Can you suggest a function to do it? 

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 177843 [details] [diff] [review]
patch v3

I have no idea what function would do that, or even whether we have one.  As I said earlier in this bug, I don't know our focus/caret code particularly well.

If you think that this is an improvement in usability over what we have now, I can sr it, but please file a followup bug on the original issue this bug was filed on (with a pointer to this code in it so it can be undone).

Comment 25

[Ginn Chen]

Checking in nsEventStateManager.cpp;
/cvsroot/mozilla/content/events/src/nsEventStateManager.cpp,v  <--  nsEventStateManager.cpp
new revision: 1.638; previous revision: 1.637
done

Comment 26

[Ginn Chen]

Bug 328823 filed to followup.

I think it's less annoying since it only happens on enabling caret browsing, but this one happens to every new opened page.

Comment 27

[Aaron Leventhal]

Comment on attachment 177843 [details] [diff] [review]
patch v3

This is in layout code -- I can't be the one to approve it.

Comment 28

[Boris Zbarsky [:bzbarsky]]

I'd like to see this get some trunk bake time (till end of March?) before we try for branch....

Comment 29

[Ginn Chen]

Checking in nsEventStateManager.cpp;
/cvsroot/mozilla/content/events/src/nsEventStateManager.cpp,v  <--  nsEventStateManager.cpp
new revision: 1.595.2.16; previous revision: 1.595.2.15
done

Comment 30

[Ginn Chen]

Attachment: back out last patch

I think we should back it out since it caused problems for some users.
We need another solution for this bug.

Comment 31

[Ginn Chen]

backed out on trunk

Checking in nsEventStateManager.cpp;
/cvsroot/mozilla/content/events/src/nsEventStateManager.cpp,v  <--  nsEventStateManager.cpp
new revision: 1.670; previous revision: 1.669
done

Comment 32

[Martijn Wargers (dead)]

(In reply to comment #30)
> I think we should back it out since it caused problems for some users.
> We need another solution for this bug.

Is the plan also to back it out on the 1.8 branch? I've seen quite a few posts/comments indicating that they might be suffering from bug 334637.

Comment 33

[Ginn Chen]

(In reply to comment #32)
> Is the plan also to back it out on the 1.8 branch? I've seen quite a few
> posts/comments indicating that they might be suffering from bug 334637.
> 

Yes.
But it needs approval from drivers, I'm requesting it.

Comment 34

[:Gavin Sharp [email: [e-mail]]]

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

Comment 35

[Daniel Veditz [:dveditz]]

Comment on attachment 244860 [details] [diff] [review]
back out last patch

approved for 1.8 branch, a=dveditz for drivers

please remove the "fixed1.8.1" keyword when you have backed this out.

Comment 36

[Ginn Chen]

backed out from MOZILLA_1_8_BRANCH

Comment 37

[Ginn Chen]

The patch was backed out from trunk and 1.8 branch.
So it's not fixed1.8.1.1

Comment 38

[Reed Loden [:reed]]

Actually, in our case, it is fixed1.8.1.1, as the approved patch for 1.8.1.1 has landed on the 1.8.1.1 branch. By having the keyword in place, it gets this bug off our triage lists.

Comment 39

[Boris Zbarsky [:bzbarsky]]

Sounds like that patch should have been on a _different_ bug...

Comment 40

[Uri Bernstein]

It seems to me that the logical place to initialize the selection to the top of the page would be at document load.
On document load completion, however, would be too late (because the user can interact with the caret before the document is completely loaded).
PresShell::InitialReflow(), on the other hand, turns out to be a bit too soon, because the body frame is still empty when we get there.

So we need to find a place where the first leaf frame of the document is already constructed, but before the user can interact with the document.
Does such a place exists?

Comment 41

[Boris Zbarsky [:bzbarsky]]

You could try the point when we unsuppress painting.  That's not guaranteed to do what you want (as in, I could concoct server-side scripts which would wait 2-3 seconds before sending any actual data while at the same time the parent document triggers InitialReflow), but it would work in the 99.9% case for sure.

Comment 42

[Uri Bernstein]

Attachment: different approach, v1

Initialize the (collapsed) selection to the top of the document in PresShell::UnsuppressAndInvalidate().

I had to make sure that we do not scroll the document to the top, because scrolling to the top led to scrolling of the main document to the top of its subdocuments as they were loaded.

The whole thing isn't very elegant, but seems to work, in general. Note that many web pages tend to have hidden (or off-screen) stuff at the top, so you don't actually see the caret when you load them. But that's a separate issue.

Comment 43

[Uri Bernstein]

Hmmm, actually my patch isn't that different than patch v2 (I should have read the entire bug before posting, I guess).
Contrary to comment #15 and #16, I think that page load is, logically, a better place for this than any focus event.

Comment 44

[Boris Zbarsky [:bzbarsky]]

I'm not likely to be able to get to this review for at least two weeks....

Comment 45

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 254670 [details] [diff] [review]
different approach, v1

>Index: mozilla/layout/base/nsPresShell.cpp
>+PresShell::CompleteMoveInner(PRBool aForward, PRBool aExtend, PRBool aScrollIntoView)
>+{
>+    nsIScrollableView *scrollableView;

Fix that indent?

Other than that, let's give it a shot.  Watch out for focus regressions, though!

Comment 46

[Uri Bernstein]

Checked in, with the indent fixed.

I'll try to keep my eyes open for regressions.

Checking in mozilla/layout/base/nsPresShell.cpp;
/cvsroot/mozilla/layout/base/nsPresShell.cpp,v  <--  nsPresShell.cpp
new revision: 3.985; previous revision: 3.984
done

Comment 47

[Mike Beltzner [:beltzner, not reading bugmail]]

regression is bug 383109

Comment 48

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This bug caused two regressions bad enough to be marked blockers. Should we just back this one out instead?

Comment 49

[Mike Schroepfer]

(In reply to comment #48)
> This bug caused two regressions bad enough to be marked blockers. Should we
> just back this one out instead?
> 

Yes.  Immediately.

Comment 50

[Johnny Stenback (:jst)]

Attachment: Back out above patch.

This still needs some testing, but while the patch that went in didn't back out cleanly any more, it was pretty straight forward to revert this change by hand.

Comment 51

[Johnny Stenback (:jst)]

Builds with the backout patch included available here:

https://build.mozilla.org/tryserver-builds/2008-04-15_17:[e-mail]-144000-backout/

Comment 52

[Johnny Stenback (:jst)]

Comment on attachment 315835 [details] [diff] [review]
Back out above patch.

AFAICT this is the proper backout here now, and it tested out fine.

Comment 53

[Uri Bernstein]

Comment on attachment 315835 [details] [diff] [review]
Back out above patch.

r=me. Sorry for not being around to handle this myself.

Comment 54

[Johnny Stenback (:jst)]

Comment on attachment 315835 [details] [diff] [review]
Back out above patch.

Testing looks good on this, going for approval for backing this out.

Comment 55

[Nelson Bolyard (seldom reads bugmail)]

Since the "fix" for this bug is (about to be) backed out, 
this bug should be reopened, right?  

Comment 56

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 315835 [details] [diff] [review]
Back out above patch.

a1.9=beltzner

Comment 57

[Johnny Stenback (:jst)]

Reopening as this will be backed out. I would've landed this today if the tree had let me. If anyone else has the time to do so, please feel free.

Comment 58

[:Gavin Sharp [email: [e-mail]]]

Landed the backout patch:
mozilla/layout/base/nsPresShell.cpp 	3.1116

Comment 59

[Johnny Stenback (:jst)]

Thanks Gavin!

Comment 60

[Trevor Saunders (:tbsaunde)]

Testing this with 4.0b10pre seems to work re comments 0 and 2.  If enter  caret browsing mode say by comment 0 on this page and cursor around I am still by the description.  If I then exit caret browsing, move  to near the add comment section and reenter caret mode  and move around with the cursor I find myself near the last comment.  both of these behaviors appear correct to me.  So this seems like a worksforme unless someone can reproduce.

Comment 61

[Ginn Chen]

It doesn't work for me.
I tried http://www.mozilla.org/projects/firefox/prerelease.html.
Turn off caret browsing, reload the page, turn on caret browsing.
Now I've no idea where is the caret.
Press left/right arrow several times, it still doesn't appear.
Press Ctrl+Home, then press right arrow several times, I can see caret now.

So I think the bug is still there.

Comment 62

[chris]

Confirm.
This bug is still present in Firefox 22/23.

Comment 63

[Firefox Bug Husbandry Bot]

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

Comment 64

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:hsinyi, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 65

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

From point of view of DOM Selection, while caret browsing is not enabled, initial selection should be "nothing" (i.e., the rangeCount should be 0) for web-compat. And according to the chrome's behavior, it seems that Selection should be collapsed to first visible text or <img> etc in the document before handling users' caret/selection navigation if and only if caret browsing mode is enabled and there is no selection range.

This must be really inconvenient bug for caret browsing users, but a Tab key press makes caret appear. So there is a workaround.

Comment 66

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 3 duplicates.
:hsinyi, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 67

[Hsin-Yi Tsai (she/her) [:hsinyi]]

We just reviewed this super old bug a few days ago.