Caret browsing is not available if there is no selection ranges
Categories
(Core :: DOM: UI Events & Focus Handling, defect, P3)
Tracking
()
People
(Reporter: dmitry, Unassigned)
References
(Blocks 1 open bug)
Details
(Keywords: access, fixed1.8.1.1, parity-chrome, Whiteboard: [not needed for 1.9] sunport17)
Attachments
(4 files, 3 obsolete files)
1.57 KB,
patch
|
aaronlev
:
review+
bzbarsky
:
superreview+
bzbarsky
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
1.53 KB,
patch
|
dveditz
:
approval1.8.1.1+
|
Details | Diff | Splinter Review |
3.52 KB,
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
3.47 KB,
patch
|
uriber
:
review+
peterv
:
superreview+
beltzner
:
approval1.9+
|
Details | Diff | Splinter Review |
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•22 years ago
|
||
-> Keyboard Navigation
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•22 years ago
|
||
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•20 years ago
|
||
*** Bug 205423 has been marked as a duplicate of this bug. ***
Updated•20 years ago
|
*** Bug 241041 has been marked as a duplicate of this bug. ***
*** Bug 254996 has been marked as a duplicate of this bug. ***
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 ); }
Updated•20 years ago
|
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•19 years ago
|
||
Comment on attachment 176704 [details] [diff] [review] patch Oops, Ginn. That was the wrong patch.
Comment 10•19 years ago
|
||
sorry for that
Comment 11•19 years ago
|
||
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•19 years ago
|
||
What about this one? nsEventStateManager::GetBrowseWithCaret() doesn't work here.
Comment 13•19 years ago
|
||
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•19 years ago
|
||
I just spoke with neil@parkwaycc.co.uk on IRC and his first thought is also to put this in nsEventStateManager.
Comment 15•19 years ago
|
||
What about put the hunk here? I think it's much better than patch v2.
Comment 16•19 years ago
|
||
Comment on attachment 177843 [details] [diff] [review] patch v3 Much better, in my opinion.
Comment 17•19 years ago
|
||
Neil, do you have time to review this?
Comment 18•19 years ago
|
||
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•19 years ago
|
||
January is still better than no review and comment in last 8 months
Comment 20•18 years ago
|
||
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•18 years ago
|
||
(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•18 years ago
|
||
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•18 years ago
|
||
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•18 years ago
|
||
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•18 years ago
|
||
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•18 years ago
|
||
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•18 years ago
|
||
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•18 years ago
|
||
I'd like to see this get some trunk bake time (till end of March?) before we try for branch....
Updated•18 years ago
|
Comment 29•18 years ago
|
||
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•18 years ago
|
||
I think we should back it out since it caused problems for some users. We need another solution for this bug.
Comment 31•18 years ago
|
||
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•18 years ago
|
||
(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•18 years ago
|
||
(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•18 years ago
|
||
*** Bug 360412 has been marked as a duplicate of this bug. ***
Comment 35•18 years ago
|
||
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.
Updated•18 years ago
|
Comment 37•18 years ago
|
||
The patch was backed out from trunk and 1.8 branch. So it's not fixed1.8.1.1
Comment 38•18 years ago
|
||
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•18 years ago
|
||
Sounds like that patch should have been on a _different_ bug...
Comment 40•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
||
I'm not likely to be able to get to this review for at least two weeks....
Comment 45•17 years ago
|
||
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•17 years ago
|
||
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
Updated•17 years ago
|
Comment 47•16 years ago
|
||
regression is bug 383109
This bug caused two regressions bad enough to be marked blockers. Should we just back this one out instead?
Comment 49•16 years ago
|
||
(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•16 years ago
|
||
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•16 years ago
|
||
Builds with the backout patch included available here: https://build.mozilla.org/tryserver-builds/2008-04-15_17:51-jst@mozilla.com-144000-backout/
Comment 52•16 years ago
|
||
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•16 years ago
|
||
Comment on attachment 315835 [details] [diff] [review] Back out above patch. r=me. Sorry for not being around to handle this myself.
Updated•16 years ago
|
Comment 54•16 years ago
|
||
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•16 years ago
|
||
Since the "fix" for this bug is (about to be) backed out, this bug should be reopened, right?
Comment 56•16 years ago
|
||
Comment on attachment 315835 [details] [diff] [review] Back out above patch. a1.9=beltzner
Comment 57•16 years ago
|
||
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•16 years ago
|
||
Landed the backout patch: mozilla/layout/base/nsPresShell.cpp 3.1116
Comment 59•16 years ago
|
||
Thanks Gavin!
Updated•16 years ago
|
Updated•15 years ago
|
Comment 60•14 years ago
|
||
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•14 years ago
|
||
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•11 years ago
|
||
Confirm. This bug is still present in Firefox 22/23.
Assignee | ||
Updated•5 years ago
|
Comment 63•5 years ago
|
||
Comment 64•2 years ago
|
||
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•2 years ago
|
||
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•2 years ago
|
||
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•2 years ago
|
||
We just reviewed this super old bug a few days ago.
Description
•