Closed Bug 568283 Opened 10 years ago Closed 9 years ago

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

Categories

(SeaMonkey :: Preferences, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.1b3

People

(Reporter: InvisibleSmiley, Assigned: ewong)

References

Details

Attachments

(1 file, 7 obsolete files)

3.91 KB, patch
ewong
: review+
ewong
: ui-review+
Details | Diff | Splinter Review
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.
Not much room there. Perhaps the Find-as-you-type groupbox can be moved into a panel of its own.
> 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.
Depends on: 623889
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Blocks: 623903
> +<!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">
Incl. fixes from comment #4
Attachment #513446 - Attachment is obsolete: true
Attachment #513642 - Flags: review?(iann_bugzilla)
Attachment #513446 - Flags: review?(iann_bugzilla)
(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.
(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.
(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 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
Attachment #513642 - Flags: review?(iann_bugzilla) → review-
Attachment #513642 - Attachment is obsolete: true
Attachment #513846 - Flags: review?(iann_bugzilla)
Attachment #513846 - Flags: review?(iann_bugzilla) → review+
You might want to file a help bug for this so we remember to add it in help.
(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.
Attachment #513846 - Flags: ui-review?(neil)
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 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.
Attachment #513846 - Attachment is obsolete: true
Attachment #514436 - Flags: ui-review?(neil)
Attachment #513846 - Flags: ui-review?(neil)
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.
Attachment #514436 - Flags: ui-review?(neil) → ui-review-
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.
Attachment #514436 - Attachment is obsolete: true
Attachment #514447 - Flags: ui-review?(neil)
Attachment #514447 - Flags: ui-review?(neil)
Attachment #514447 - Attachment is obsolete: true
Attachment #514466 - Flags: ui-review?(neil)
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.
Attachment #514466 - Flags: ui-review?(neil) → ui-review+
Attachment #514466 - Attachment is obsolete: true
Attachment #514689 - Flags: ui-review+
Attachment #514689 - Flags: review?(iann_bugzilla)
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.
Attachment #514689 - Flags: review?(iann_bugzilla) → review+
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 :-)
(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.
Keywords: checkin-needed
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.
Attachment #515063 - Attachment description: Added accessibility.browsewithcaret group to Keyboard Navigation preference panel (v4) → Added accessibility.browsewithcaret group to Keyboard Navigation preference panel (v4) [Checkin: comment 26]
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1b3
Duplicate of this bug: 634645
You need to log in before you can comment on or make changes to this bug.