Last Comment Bug 568283 - Add checkbox for accessibility.browsewithcaret (F7-by-default mode)
: Add checkbox for accessibility.browsewithcaret (F7-by-default mode)
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: seamonkey2.1b3
Assigned To: Edmund Wong (:ewong)
:
Mentors:
: 634645 (view as bug list)
Depends on: 623889
Blocks: 623903
  Show dependency treegraph
 
Reported: 2010-05-26 10:59 PDT by Jens Hatlak (:InvisibleSmiley)
Modified: 2011-03-01 11:04 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Add checkbox to KeyNavigation for accessibility.browsewithcaret (F7-by-default mode) (1.97 KB, patch)
2011-02-18 06:57 PST, Edmund Wong (:ewong)
no flags Details | Diff | Review
Added checkbox for accessibility.browsewithcaret (F7-by-default mode) (2.17 KB, patch)
2011-02-18 17:16 PST, Edmund Wong (:ewong)
iann_bugzilla: review-
Details | Diff | Review
Add checkbox to KeyNavigation for accessibility.browsewithcaret (F7-by-default mode) (v2) (2.05 KB, patch)
2011-02-20 01:26 PST, Edmund Wong (:ewong)
iann_bugzilla: review+
Details | Diff | Review
Added accessibility.browsewithcaret group to keyboard Navigation preference panel. (4.26 KB, patch)
2011-02-23 00:47 PST, Edmund Wong (:ewong)
neil: ui‑review-
Details | Diff | Review
Added accessibility.browsewithcaret group to keyboard Navigation preference panel. (v2) (4.25 KB, patch)
2011-02-23 01:58 PST, Edmund Wong (:ewong)
no flags Details | Diff | Review
Added accessibility.browsewithcaret group to Keyboard Navigation preference panel (v3) (3.94 KB, patch)
2011-02-23 05:17 PST, Edmund Wong (:ewong)
neil: ui‑review+
Details | Diff | Review
Added accessibility.browsewithcaret group to Keyboard Navigation preference panel (v4) (3.93 KB, patch)
2011-02-23 17:59 PST, Edmund Wong (:ewong)
iann_bugzilla: review+
ewong: ui‑review+
Details | Diff | Review
Added accessibility.browsewithcaret group to Keyboard Navigation preference panel (v4) [Checkin: comment 26] (3.91 KB, patch)
2011-02-25 06:58 PST, Edmund Wong (:ewong)
ewong: review+
ewong: ui‑review+
Details | Diff | Review

Description Jens Hatlak (:InvisibleSmiley) 2010-05-26 10:59:10 PDT
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 2010-11-20 01:48:01 PST
Not much room there. Perhaps the Find-as-you-type groupbox can be moved into a panel of its own.
Comment 2 Philip Chee 2011-01-07 05:04:56 PST
> 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) 2011-02-18 06:57:46 PST
Created attachment 513446 [details] [diff] [review]
Add checkbox to KeyNavigation for accessibility.browsewithcaret (F7-by-default mode)
Comment 4 Philip Chee 2011-02-18 07:53:35 PST
> +<!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) 2011-02-18 17:16:15 PST
Created attachment 513642 [details] [diff] [review]
Added checkbox for accessibility.browsewithcaret (F7-by-default mode)

Incl. fixes from comment #4
Comment 6 Ian Neal 2011-02-19 05:48:17 PST
(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 2011-02-19 05:50:41 PST
(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] (away until May 28) 2011-02-19 08:27:27 PST
(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 2011-02-19 09:15:05 PST
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) 2011-02-20 01:26:25 PST
Created attachment 513846 [details] [diff] [review]
Add checkbox to KeyNavigation for accessibility.browsewithcaret (F7-by-default mode) (v2)
Comment 11 Stefan [:stefanh] (away until May 28) 2011-02-21 09:48:25 PST
You might want to file a help bug for this so we remember to add it in help.
Comment 12 Edmund Wong (:ewong) 2011-02-21 18:49:34 PST
(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 neil@parkwaycc.co.uk 2011-02-22 07:41:04 PST
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 neil@parkwaycc.co.uk 2011-02-22 07:46:06 PST
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) 2011-02-23 00:47:25 PST
Created attachment 514436 [details] [diff] [review]
Added accessibility.browsewithcaret group to keyboard Navigation preference panel.
Comment 16 neil@parkwaycc.co.uk 2011-02-23 01:37:58 PST
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 neil@parkwaycc.co.uk 2011-02-23 01:40:49 PST
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) 2011-02-23 01:58:39 PST
Created attachment 514447 [details] [diff] [review]
Added accessibility.browsewithcaret group to keyboard Navigation preference panel. (v2)
Comment 19 Edmund Wong (:ewong) 2011-02-23 05:17:17 PST
Created attachment 514466 [details] [diff] [review]
Added accessibility.browsewithcaret group to Keyboard Navigation preference panel (v3)
Comment 20 neil@parkwaycc.co.uk 2011-02-23 05:33:28 PST
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) 2011-02-23 17:59:59 PST
Created attachment 514689 [details] [diff] [review]
Added accessibility.browsewithcaret group to Keyboard Navigation preference panel (v4)
Comment 22 Ian Neal 2011-02-24 17:09:05 PST
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) 2011-02-25 06:58:16 PST
Created attachment 515063 [details] [diff] [review]
Added accessibility.browsewithcaret group to Keyboard Navigation preference panel (v4) [Checkin: comment 26]
Comment 24 Stefan [:stefanh] (away until May 28) 2011-02-26 13:06:25 PST
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) 2011-02-26 18:01:36 PST
(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) 2011-02-27 01:24:14 PST
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.
Comment 27 Philip Chee 2011-03-01 11:04:25 PST
*** Bug 634645 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.