194937 - Turn off caret browsing when Firefox relaunched

Status: NEW

(Firefox :: Disability Access, defect)

Description

[Pascal Chevrel:pascalc (PTO until Sept 2)]

build 2003022508 WinXP

Reproducible : always
1 open any web page and click on one of its links
2 wait for the page to display
3 hit the page down key

Expected result : page should scroll down 1 page
actual result : nothing happens, you have to click with the mouse into the page
to have the page down and page up keys working. Home and End keys do not have
this problem.

Netscape 7.02 does not have this problem

Comment 1

[Boris Zbarsky [:bzbarsky]]

ere, could this be your window focus stuff?

Comment 2

[Ere Maijala (slow)]

Maybe, but I don't see this problem. Same build (2003022508 trunk) on WinXP Pro SP1.

Comment 3

[Ere Maijala (slow)]

No problem with 2003023408 either on a different WinXP machine.

Comment 4

[Pascal Chevrel:pascalc (PTO until Sept 2)]

I am going to try with a new profile, I'll keep you informed

Comment 5

[Pascal Chevrel:pascalc (PTO until Sept 2)]

I've found what is causing the bug, it happens only if you have this line in
prefs.js :

user_pref("accessibility.browsewithcaret", true);

Is it normal ?

Comment 6

[Ere Maijala (slow)]

Ok, now I see it too after turning caret browsing on using F7. Just wondering if
this is related to caret moving to wrong position in Composer when backspacing
as it seems the caret is already at the end of the page when it has loaded.
Anyway I'd say this isn't related to the focus thingy.

Comment 7

[Kathleen :Brade]

Is this a problem with today's builds?  If not, it's probably related to the
caret bug which was turned off yesterday.

Comment 8

[Pascal Chevrel:pascalc (PTO until Sept 2)]

I am still seeing it with build 2003022516

Comment 9

[Ere Maijala (slow)]

Yep, it wasn't the same problem although they could be related. The caret is
initially positioned in the very end of the document.

Comment 10

[Pascal Chevrel:pascalc (PTO until Sept 2)]

Still seeing the bug with build 2003042208

Comment 11

[Benjamin Streeck]

Still seeing this on 2003121709.

Changing component and assigning.

Comment 12

[Aaron Leventhal]

What's really causing the bug is that people turn on caret browsing it don't
realize it, and don't know how to turn it off.

So, I don't think caret browsing should persist between browser sessions.

Comment 13

[Aaron Leventhal]

Neil, Dean -- any thoughts?

Comment 14

[:deanis74]

I don't use caret browsing, but if I did I'd want it to persist across sessions.

Comment 15

[[e-mail]]

We need to save the setting as a preference so that it affects new windows...

Comment 16

[Ere Maijala (slow)]

It would be nice to have an indicator for caret browsing. One thing making it
less  useful is that it's quite flaky, at least in some situations. I tried to
use it short while ago but had to toggle it off and on constantly to actually
get the caret.

Comment 17

[Aaron Leventhal]

(In reply to comment #16)
> It would be nice to have an indicator for caret browsing. One thing making it
> less  useful is that it's quite flaky, at least in some situations. I tried to
> use it short while ago but had to toggle it off and on constantly to actually
> get the caret.

All the flaky caret browsing behaviors are bugs in the selection code -- those
same bugs are problems in the editor. Notice how flaky the editor is? :(

See bug 241023 -- it's the meta bug for caret browser flakiness/issues.

Comment 18

[Aaron Leventhal]

(In reply to comment #15)
> We need to save the setting as a preference so that it affects new windows...

Neil, in Seamonkey we could clear the pref when you relaunch, and make the user
hit F7 again if they still want it.

Or, come up with a better suggestion to prevent these situations where people
suddenly get in this mode where there's a caret, and don't know how to turn it
off. They can't even scroll normally anymore, and exiting and relaunching won't
get them back. I have to admit it was a bad idea to have this toggle persist.
What if the cat or your kid accidentally triggers it? You're stuck with it then,
unless you can figure it out, or uninstall and reinstall.

Comment 19

[[e-mail]]

We already have the caret browsing dialog...

How about adding the pref to the Keyboard preferences?

Comment 20

[Aaron Leventhal]

(In reply to comment #19)
> We already have the caret browsing dialog...
Based on the number of bugs filed such as "arrow keys no longer scroll" I have
learned a lesson in how many people read confirmation dialogs.

> How about adding the pref to the Keyboard preferences?
How likely is someone who just hit's OK without reading after they hit F7 ever
to see advanced -> keyboard prefs? If they don't see it, they're pretty much
stuck forever unless they happen to ask the right person. Perhaps we should
remove the F7 binding altogether and only have it in prefs, as firefox does.

Comment 21

[Ere Maijala (slow)]

(In reply to comment #20)
> (In reply to comment #19)
> > We already have the caret browsing dialog...
> Based on the number of bugs filed such as "arrow keys no longer scroll" I have
> learned a lesson in how many people read confirmation dialogs.

Besides, if it was a kid or just someone else, people didn't even have a chance.

> > How about adding the pref to the Keyboard preferences?
> How likely is someone who just hit's OK without reading after they hit F7 ever
> to see advanced -> keyboard prefs? If they don't see it, they're pretty much
> stuck forever unless they happen to ask the right person. Perhaps we should

How about reversing the behavior and make it persist only if the pref in
keyboard prefs is turned on?

Comment 22

[[e-mail]]

OK, how about a two-pronged approach; in prefs:
[X] Turn Caret Browsing on
[X] Use the F7 key to turn Caret Browsing on and off
    [X] Warn me before using F7 to turn Caret Browsing on
Then, in the warning dialog, instead of
[X] Do not show me this dialog box again
use
[X] Remember this decision

Comment 23

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

I think comment 22 is a bit of overkill in terms of just throwing options at the user. I think that the pref as set by F7 shouldn't persist across sessions without some visual notification in the chrome that it's on or off; otherwise people will just plain forget which way they had it flipped. But since I doubt that visual indication will be useful to people with accessibility concerns ... it should probably just not persist ;)

But the pref as set by the checkbox *should* persist.

Also, see bug 308603 for a suggested re-wording of the checkbox pref.

Comment 24

[Mark Pilgrim (inactive)]

After discussion in IRC, Lucy, mconnor, aaronlev, and I have decided on the following behavior:

- F7 will be temporary; the option in Preferences/Advanced/Accessibility will be permanent.
- Pressing F7 will toggle caret browsing globally (all windows, all tabs), but only until the user presses F7 again or the entire application shuts down.
- On application startup, caret browsing mode will revert to the state given in the Preferences window.
- Technically, this will be accomplished by creating a new preference, accessibility.browsewithcaretonstartup.  The existing accessibility.browsewithcaret (which all code currently uses to detect whether caret browsing is on) will become a temporary preference; on application startup, we will overwrite it with the value of accessibility.browsewithcaretonstartup.  The option in Preferences window will set both preferences.  Pressing F7 will only set accessibility.browsewithcaret.  This will minimize the scope of the required patch, because all existing code that checks accessibility.browsewithcaret will continue to work unmodified.
- Pressing F7 will change the statusbar text to "Caret browsing on/off" to indicate the current mode.
- If caret browsing is on (temporarily or permanently), all new windows and tabs will use a default status text of "Caret browsing on".  If caret browsing is off, the status text will not mention caret browsing.
- The current option in the Preferences/Advanced/Accessibility section should be changed per bug 308603 to something like "Always use caret browsing to navigate in web pages".
- The current dialog that pops up when you press F7 should be rewritten to explain that the effect is only temporary, and that if the user wishes to turn it on permanently, they may do so in the Preferences window.

Comment 25

[Alan Cantor]

<quote>
But since I doubt
that visual indication will be useful to people with accessibility concerns ...
it should probably just not persist ;)
</quote>

A visual indication will be extremely useful to many folks with accessibility concerns, including people who cannot or prefer not to use a mouse, individuals with certain forms of low-vision, and even people with some learning disabilities. A visual indication will help someone figure out what happened after pressing F7 by accident. It will also be good for users who serendipitously discover that Firefox supports novel ways to navigate web pages. 

The root of this "bug" I believe is that the application gives absolutely no cues -- visual, auditory, or otherwise -- that the browsing mode has switched. Furthermore, the application provides only one way to restore order: by pressing a function key. Perhaps something as simple as causing the speaker to beep when changing modes would be enough to alert users that SOMETHING happens when you press F7.

I think a visual indication for caret browsing mode is essential. My vote is for an icon (or text equivalent) on the status line and/or a toolbar; plus a checked item on one of the main menus (Tools? View?) to show that caret browsing is on or off.

I also think there should be a name to describe browsing when NOT in caret mode -- "Normal mode" perhaps? -- to distinguish two different keyboard navigation strategies. Then it becomes easier to describe how Firefox works: 

"Using the keyboard, you can browse in two ways: in NORMAL mode, use the tab key to move from link to link... In CARET mode, move around a web page exactly like you move around a word processing document. For example, use directional keys (e.g., down and up arrows) to move by character, or by line... or Ctrl + left arrow and Ctrl + right arrow to move by word. To switch between normal and caret modes, press F7. You can tell which mode you are in by looking at the icon on the status bar..."

Caret browsing is extremely useful. However, I doubt most users will ever use it, so I don't think anything will be lost by disabling the feature in the default installation. My guess is that most people who benefit most from caret browsing mode use it only occasionally. But it may be that screen reader users will prefer caret browsing, and want it as a default setting. So we need to be careful before turning off the setting every time Firefox is exited.

Comment 26

[Mark Pilgrim (inactive)]

Attachment: patch implementing comment 24

Aaron, please sanity-check this and then help me figure out who else needs to review it.

Comment 27

[Aaron Leventhal]

Comment on attachment 219753 [details] [diff] [review]
patch implementing comment 24

I do notice:
\ No newline at end of file

However, I'm not the best person to run this by at the moment. Let's try timeless.

Comment 28

[timeless]

Comment on attachment 219753 [details] [diff] [review]
patch implementing comment 24

no new line isn't his fault , but...

afaik, this breaks seamonkey, which isn't ok. include equivalent xxpfe changes .

Comment 29

[Mark Pilgrim (inactive)]

Attachment: Patch with timeless's feedback

Should no longer break Seamonkey.  Also added missing newline.

Comment 30

[[e-mail]]

Comment on attachment 222176 [details] [diff] [review]
Patch with timeless's feedback

>@@ -237,16 +237,24 @@ nsEventStateManager::Init()
Note: there's more than one nsEventStateManager

>+            window.XULBrowserWindow.setDefaultStatus(document.getElementById("bundle_browser").getString(defaultStatusString));
Unhelpful dependency on navigator.xul (note that toolkit's browser.xml already seems to depend on browser.xul, which isn't helpful).

Comment 31

[Mark Pilgrim (inactive)]

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

Comment 32

[Mark Pilgrim (inactive)]

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

Comment 33

[Anatoly Kochergin]

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

Comment 34

[Mark Pilgrim (inactive)]

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

Comment 35

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

Mark/Aaron, do you have a timeframe for this fix? Timeless, can we get a review? I'd like to see a target milestone set (preferably beta1) if possible so we can triage it for 1.8.1/Fx2.

Comment 36

[Aaron Leventhal]

Mark, when this gets checked make sure to inform the various a11y newsgroups about the change. Send a message something like "Important change to caret browsing for Firefox 2."  That way the few folks are really using that setting will know how to reset the fact that they realy do want it on by default, and we'll get fewer dupes filed later :)

Comment 37

[Mark Pilgrim (inactive)]

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

Comment 38

[Mark Pilgrim (inactive)]

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

Comment 39

[Mark Pilgrim (inactive)]

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

Comment 40

[Mike Schroepfer]

We'd take a low-risk patch for 1.8.1 - but won't block for it at this time.

Comment 41

[Mark Pilgrim (inactive)]

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

Comment 42

[Andrew Schultz]

Comment on attachment 222176 [details] [diff] [review]
Patch with timeless's feedback

>+++ toolkit/locales/en-US/chrome/global/tabbrowser.properties	16 May 2006 14:03:34 -0000

You also need to update the seamonkey version (xpfe/global/resources/locale/en-US/tabbrowser.properties).  As long as you're not mentioning the pref dialog, the text can be the same.

>++browsewithcaret.checkLabel=Pressing F7...
 ^^

one "+" please, at least for the xpfe version.  :)

Comment 43

[Andrew Schultz]

Comment on attachment 222176 [details] [diff] [review]
Patch with timeless's feedback

>+      nsContentUtils::SetBoolPref("accessibility.browsewithcaret",
>+                                  browseWithCaretOnStartup);

There are no pref setters in nsContentUtils

Comment 44

[Andrew Schultz]

Comment on attachment 222176 [details] [diff] [review]
Patch with timeless's feedback

>-          // Toggle the pref
>+           // Toggle the pref

...and you shouldn't need to change this line.

Comment 45

[Mark Pilgrim (inactive)]

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

Comment 46

[Mark Pilgrim (inactive)]

Attachment: Patch with feedback incorporated

Incorporated all feedback to date.

Comment 47

[timeless]

Comment on attachment 230007 [details] [diff] [review]
Patch with feedback incorporated

>Index: toolkit/locales/en-US/chrome/global/tabbrowser.properties
>@@ -1,14 +1,18 @@
>-tabs.closeWarningPromptMe=Warn me when I attempt to close multiple tabs
>\ No newline at end of file
>+tabs.closeWarningPromptMe=Warn me when I attempt to close multiple tabs

I presume this is some unrelated change:
>+refreshBlocked.goButton=Allow
>+refreshBlocked.goButton.accesskey=o
>+refreshBlocked.refreshLabel=%S prevented this page from automatically reloading.
>+refreshBlocked.redirectLabel=%S prevented this page from automatically redirecting to another page.

>Index: toolkit/content/widgets/browser.xml
>@@ -835,21 +835,31 @@
>+          // Set the status bar
bad indent:
>+           try {
>+            var defaultStatusString = "nv_done";
>+            if (browseWithCaretOn)
>+              defaultStatusString = "nv_done_browsewithcaret";
>+            window.XULBrowserWindow.setDefaultStatus(document.getElementById("bundle_browser").getString(defaultStatusString));
>+           } catch (ex) {
>+           }
>+         ]]>
>Index: xpfe/global/resources/content/bindings/browser.xml
>@@ -509,19 +509,29 @@
>+          // Set the status bar
again :)
>+           try {
>+            var defaultStatusString = "nv_done";
>+            if (browseWithCaretOn)
>+              defaultStatusString = "nv_done_browsewithcaret";
>+            window.XULBrowserWindow.setDefaultStatus(document.getElementById("bundle_browser").getString(defaultStatusString));
>+           } catch (ex) {
>+           }
>+         ]]>

Comment 48

[Mark Pilgrim (inactive)]

Attachment: patch with fixed spacing and removed cruft

Comment 49

[[e-mail]]

Comment on attachment 230610 [details] [diff] [review]
patch with fixed spacing and removed cruft

>+  updateBrowseWithCaret: function () 
>+  {
>+    var browseWithCaretOnStartupPref = document.getElementById("accessibility.browsewithcaretonstartup");
>+    var browseWithCaretPref = document.getElementById("accessibility.browsewithcaret");
>+
>+    browseWithCaretPref.value = browseWithCaretOnStartupPref.value;
>+  },
I'm not convinced that this is the best way to set the pref when the "default" pref's value changes. In fact this patch will never work properly in xpfe builds as the profile manager will init the pref to the wrong value and ignore any per-user preference. Is this change wanted on the branch?

>+          var browseWithCaretOn = !browseWithCaretOn;
Redeclaration (var browseWithCarentOn was declared earlier).

Comment 50

[Mark Pilgrim (inactive)]

Neil: I understand the issue now that you explained it on IRC.  If we agree to only land this on trunk, will you approve it?

Comment 51

[[e-mail]]

Comment on attachment 230007 [details] [diff] [review]
Patch with feedback incorporated

>+      // initialize caret browsing
>+      PRBool browseWithCaretOnStartup = PR_FALSE;
>+      browseWithCaretOnStartup =
>+        nsContentUtils::GetBoolPref("accessibility.browsewithcaretonstartup",
>+                                    browseWithCaretOnStartup);
>+      prefBranch->SetBoolPref("accessibility.browsewithcaret",
>+                              browseWithCaretOnStartup);
Needs a comment that it doesn't observe the pref so you need to change the browse with caret pref whenever you change the startup pref.

>+nv_done_browsewithcaret=Done. Caret Browing is on
(sp)

>-            msg = gNavigatorBundle.getString("nv_done");
>+            if (!gPrefService)
>+              gPrefService = Components.classes["@mozilla.org/preferences-service;1"]
>+                                       .getService(Components.interfaces.nsIPrefBranch);
>+            if (gPrefService.getBoolPref("accessibility.browsewithcaret"))
>+              msg = gNavigatorBundle.getString("nv_done_browsewithcaret");
>+            else
>+              msg = gNavigatorBundle.getString("nv_done");
No nsBrowserStatusHandler.js version of this.

>+          var browseWithCaretOn = !browseWithCaretOn;
Redeclaration (in case you forgot).

>+            var defaultStatusString = "nv_done";
>+            if (browseWithCaretOn)
>+              defaultStatusString = "nv_done_browsewithcaret";
browseWithCaretOn ? "nv_done_browsewithcaret" : "nv_done";

>+            window.XULBrowserWindow.setDefaultStatus(document.getElementById("bundle_browser").getString(defaultStatusString));
We're not supposed to refer to navigator.xul from tabbrowser.xml, also window.XULBrowserWindow has no setDefaultStatus method.

Comment 52

[Ria Klaassen (not reading all bugmail)]

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

Comment 53

[Aaron Leventhal]

Mark doesn't work on Mozilla anymore. This bug needs an owner to finish up the work.

Comment 55

[Sylvestre Ledru [:Sylvestre]]

Decreasing the priority as no update for the last 2 years on this bug.
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage 
about the priority meaning.

Comment 56

[Andrei Purice]

Hey Pascal,
Can you still reproduce this or should we close it?
It works for me on the latest Firefox versions. When pressing page down it scrolls down 1 page.

Comment 57

[Pascal Chevrel:pascalc (PTO until Sept 2)]

(In reply to Andrei Purice from comment #56)

Hey Pascal,
Can you still reproduce this or should we close it?
It works for me on the latest Firefox versions. When pressing page down it scrolls down 1 page.

I filed this bug 19 years ago, honestly I don't know but bug 1703633 looks related. I think the problem may still exist but is harder to hit now.

Comment 58

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

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.

Comment 59

[Henri Sivonen (:hsivonen)]

F7 is still sticky across browser relaunch. However, these days F7 shows a warning about what it is about to do, which mitigates the problem reported here somewhat.

While this isn't strictly about a11y APIs but about a built-in a11y feature, sending over to the a11y component, since the stickiness of the mode isn't really a DOM issue.

Comment 60

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

The severity field is not set for this bug.
:eeejay, could you have a look please?

For more information, please visit BugBot documentation.