568283 - Add checkbox for accessibility.browsewithcaret (F7-by-default mode)

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, enhancement)

Description

[Jens Hatlak (:InvisibleSmiley)]

Spin-off from bug 465303: I suggest we add a checkbox for accessibility.browsewithcaret (F7-by-default mode) in Preferences, e.g. Advanced/Keyboard Navigation. FF has it under Advanced/General/Accessibility/Always use the cursor keys to navigate within pages.

Comment 1

[Philip Chee]

Not much room there. Perhaps the Find-as-you-type groupbox can be moved into a panel of its own.

Comment 2

[Philip Chee]

> Not much room there. Perhaps the Find-as-you-type groupbox can be moved into a
> panel of its own.
Only a suggestion: call it Advanced->Accessibility
And add |accessibility.browsewithcaret| to that panel.

Comment 3

[Edmund Wong (:ewong)]

Attachment: Add checkbox to KeyNavigation for accessibility.browsewithcaret (F7-by-default mode)

Comment 4

[Philip Chee]

> +<!ENTITY accessibilityBrowseWithCaret.label "Browse With Caret (F7-by-default mode)">

> +<!ENTITY browseWithCaretDesc.label "Always use the cursor keys to navigate within pages.">
Suggestions:

1. Enable keyboard navigation and selection within web pages using a visible caret.

2. Allows users to select arbitrary content with the keyboard and move through content as if inside a read-only editor. This allows copying arbitrary pieces of text to the clipboard. The F7 key toggles this feature on/off.

> +<!ENTITY browseWithCaret.label "Browse with Caret">
Repeating "Browse with Caret" doesn't convey any additional information.
Firefox:
<!ENTITY useCursorNavigation.label       "Always use the cursor keys to navigate within pages">

> +<!ENTITY browseWithCaret.accesskey "r">
Reference:
<https://developer.mozilla.org/en/XUL_Accesskey_FAQ_and_Policies#How_do_I_pick_an_accesskey_letter.3f>
Firefox:
<!ENTITY useCursorNavigation.accesskey   "c">

Comment 5

[Edmund Wong (:ewong)]

Attachment: Added checkbox for accessibility.browsewithcaret (F7-by-default mode)

Incl. fixes from comment #4

Comment 6

[Ian Neal]

(In reply to comment #5)
> Created attachment 513642 [details] [diff] [review]
> Added checkbox for accessibility.browsewithcaret (F7-by-default mode)
> 
> Incl. fixes from comment #4

Before I review it, just interested whether F7 is the key on all platforms so cc'ing stefanh for confirmation.

Comment 7

[Ian Neal]

(In reply to comment #4)
> > +<!ENTITY browseWithCaret.accesskey "r">
> Reference:
> <https://developer.mozilla.org/en/XUL_Accesskey_FAQ_and_Policies#How_do_I_pick_an_accesskey_letter.3f>
> Firefox:
> <!ENTITY useCursorNavigation.accesskey   "c">
We have plenty of accesskeys to pick from on this pane, so I would say "A" was better.

Comment 8

[Stefan [:stefanh]]

(In reply to comment #6)
> Before I review it, just interested whether F7 is the key on all platforms so
> cc'ing stefanh for confirmation.

F7 is the key on Mac, yes.

Comment 9

[Ian Neal]

Comment on attachment 513642 [details] [diff] [review]
Added checkbox for accessibility.browsewithcaret (F7-by-default mode)

>+++ b/suite/locales/en-US/chrome/common/pref/pref-keynav.dtd

>+<!ENTITY accessibilityBrowseWithCaret.label "Browse With Caret (F7-by-default mode)">
I'm not sure that we should have the (F7-by-default mode) bit here.

>+<!ENTITY browseWithCaretDesc.label "Allows users to select arbitrary content with the keyboard and move through content as if inside a read-only editor.  This allows copying arbitrary pieces of text to the clipboard.  The F7 key toggles this feature on/off.">
This sounds too wordy for a pref pane, more suited to help. I would prefer something like:
"Enable keyboard navigation and selection within web pages using a visible caret. The F7 key toggles this feature on/off."

>+<!ENTITY browseWithCaret.label "Always use the cursor keys to navigate within pages">
>+<!ENTITY browseWithCaret.accesskey "c">
"A" would be a better accesskey

Comment 10

[Edmund Wong (:ewong)]

Attachment: Add checkbox to KeyNavigation for accessibility.browsewithcaret (F7-by-default mode) (v2)

Comment 11

[Stefan [:stefanh]]

You might want to file a help bug for this so we remember to add it in help.

Comment 12

[Edmund Wong (:ewong)]

(In reply to comment #11)
> You might want to file a help bug for this so we remember to add it in help.

Filed bug 635806.

Comment 13

[[e-mail]]

So, there are actually three preferences in play here:
> accessibility.browsewithcaret
Off by default, this actually reflects the state of the feature.
> accessibility.browsewithcaret_shortcut.enabled
On by default; if this is off, pressing F7 does nothing.
> accessibility.warn_on_browsewithcaret
On by default; if this is on, pressing F7 warns before turning the main preference on (it never warns before turning it off). If you tick the checkbox then this preference gets reset.

So I think we should go for two or maybe all three of these preferences:
Caret browsing enables you to navigate and select within pages using the cursor keys to move a visible caret.
[X] Use caret browsing
[X] Use the F7 shortcut to toggle caret browsing
    [X] Warn me before turning on caret browsing
(strings subject to bikeshedding)

Comment 14

[[e-mail]]

Comment on attachment 513846 [details] [diff] [review]
Add checkbox to KeyNavigation for accessibility.browsewithcaret (F7-by-default mode) (v2)

>+                class="indent"
Don't see why this needs to be indented.

>+<!ENTITY browseWithCaretDesc.label "Enable keyboard navigation and selection within web pages using a visible caret.  The F7 key toggles this feature on/off.">
This shouldn't use "on/off", it should say "on or off".

>+<!ENTITY browseWithCaret.label "Always use the cursor keys to navigate within pages">
>+<!ENTITY browseWithCaret.accesskey "A">
"Always" seems to be a bit excessive to me, given that it's only until you press F7.

Comment 15

[Edmund Wong (:ewong)]

Attachment: Added accessibility.browsewithcaret group to keyboard Navigation preference panel.

Comment 16

[[e-mail]]

Comment on attachment 514436 [details] [diff] [review]
Added accessibility.browsewithcaret group to keyboard Navigation preference panel.

>+function UpdateBrowseWithCaretItems()
>+{
>+  document.getElementById("browseWithCaretWarn").disabled =
>+    !document.getElementById("accessibility.browsewithcaret").value ||
>+    document.getElementById("accessibility.browsewithcaret").locked;
>+
>+  document.getElementById("browseWithCaretShortCut").disabled =
>+    !document.getElementById("accessibility.browsewithcaret").value ||
>+    document.getElementById("accessibility.browsewithcaret").locked;
>+}
This isn't quite right; the Warn pref should be disabled if the ShortCut pref is false or if the Warn pref is locked; the ShortCut pref shouldn't need any special disabling.

>+      <preference id="accessibility.browsewithcaretshortcut"
>+                  name="accessibility.browsewithcaret_shortcut.enabled"
>+                  type="bool"
>+                  onchange="UpdateBrowseWithCaretItems();"/>
This is the only change that we're interested in.

>+    <groupbox>
I compared this to the existing groupbox which I noticed had align="start", which makes the checkboxes look marginally nicer...

>+      <vbox>
...but you also need to remove this unnecessary vbox.

Comment 17

[[e-mail]]

Comment on attachment 514436 [details] [diff] [review]
Added accessibility.browsewithcaret group to keyboard Navigation preference panel.

>+      <preference id="accessibility.browsewithcaretshortcut"
>+                  name="accessibility.browsewithcaret_shortcut.enabled"
Oops.

Comment 18

[Edmund Wong (:ewong)]

Attachment: Added accessibility.browsewithcaret group to keyboard Navigation preference panel. (v2)

Comment 19

[Edmund Wong (:ewong)]

Attachment: Added accessibility.browsewithcaret group to Keyboard Navigation preference panel (v3)

Comment 20

[[e-mail]]

Comment on attachment 514466 [details] [diff] [review]
Added accessibility.browsewithcaret group to Keyboard Navigation preference panel (v3)

>+      <preference id="accessibility.browsewithcaret_shortcut"
>+                  name="accessibility.browsewithcaret_shortcut"
In case it wasn't clear, the correct name is accessibility.browsewithcaret_shortcut.enabled

>+      <checkbox id="browseWithCaretShortCut"
>+                class="indent"
This one doesn't need an indent. (I forgot to mention that last time, sorry.)

ui-r=me with those fixed.

Comment 21

[Edmund Wong (:ewong)]

Attachment: Added accessibility.browsewithcaret group to Keyboard Navigation preference panel (v4)

Comment 22

[Ian Neal]

Comment on attachment 514689 [details] [diff] [review]
Added accessibility.browsewithcaret group to Keyboard Navigation preference panel (v4)

>+++ b/suite/locales/en-US/chrome/common/pref/pref-keynav.dtd
>+<!ENTITY accessibilityBrowseWithCaret.label "Browse With Caret (F7-by-default mode)">
(F7-by-default mode) seems to have reappeared in this label somehow...

>+<!ENTITY browseWithCaretDesc.label "Caret browsing enables you to navigate and select within pages using the cursor keys to move a visible caret.">
>+<!ENTITY browseWithCaretUse.label "Use caret browsing">
>+<!ENTITY browseWithCaretUse.accesskey "c">
"U" would be a better key to use.
r=me with those addressed.

Comment 23

[Edmund Wong (:ewong)]

Attachment: Added accessibility.browsewithcaret group to Keyboard Navigation preference panel (v4) [Checkin: comment 26]

Comment 24

[Stefan [:stefanh]]

ewong, is this ready to land? The mac keynav pane is empty after the removal of fayt prefs and it would be nice to see something in the pane :-)

Comment 25

[Edmund Wong (:ewong)]

(In reply to comment #24)
> ewong, is this ready to land? The mac keynav pane is empty after the removal of
> fayt prefs and it would be nice to see something in the pane :-)

I believe so.   I've set the checkin-needed.

Comment 26

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 515063 [details] [diff] [review]
Added accessibility.browsewithcaret group to Keyboard Navigation preference panel (v4) [Checkin: comment 26]

http://hg.mozilla.org/comm-central/rev/7df41cd35756

>+function UpdateBrowseWithCaretItems()
>+{
>+  document.getElementById("browseWithCaretWarn").disabled =
>+    !document.getElementById("accessibility.browsewithcaret_shortcut.enabled").value ||
>+    document.getElementById("accessibility.browsewithcaret").locked;
>+
>+}

I took the liberty to remove that blank line upon checkin.