128025 - show a confirm dialog when turning ON caret browsing (F7)

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P2)

Description

[Aaron Leventhal]

We don't want users getting into weird modes by pressing the wrong key.
When Ctrl+Shift+K is used to start browse with caret mode, a confirm dialog
should appear asking if they want to enter this mode. The default answer should
be No. There should be a checkbox, "Ask in the future"/

Comment 1

[Aaron Leventhal]

nsbeta1+ per Chris Saari

Comment 2

[Aaron Leventhal]

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

Comment 3

[Aaron Leventhal]

Attachment: Uses pref listener and binding to <browser> tag to support browse with caret, adds dialog, changes key to F7

I had to change the toggle key to F7 - something that would work on all locales
and platforms. Otherwise I wouldn't be able to inform the user in the confirm
dialog, what the toggle key to turn it back off again is. Since we're not
discoverable in any menu, this information is important.

Here is the message in the dialog:

Do you want to turn Caret Browsing on? This feature allows you to move around
the content with a cursor, and select text with the keyboard. Pressing F7
toggles it on and off.
[Use caret]	  [Cancel]

Comment 4

[Aaron Leventhal]

Correction, here is the exact wording of the dialog:

Do you want to turn Caret Browsing on? This feature allows you to move around
the content with a cursor, and select text with the keyboard. Pressing F7 turns
Caret Browsing on and off.

[ Use caret ]        [ Cancel ]

Comment 5

[Aaron Leventhal]

I have confirmed that this works in help, view source, inspect, mailnews, and
browser.

Comment 6

[Aaron Leventhal]

Attachment: Forgot file - contains htmlBindings.xml which removes Accel+Shift+K binding

Comment 7

[Akkana Peck]

r=akkana on everything except the browser.xml changes -- I don't understand
those well enough to review them.

I wonder how cheap/expensive it is to get the pref service every time we bring
up a new window.  In a lot of other places we make a static variable to cache
the prefs service, and it might be a good idea to do that here since it happens
on every window.

Comment 8

[Aaron Leventhal]

Akkana and I grepped through the source. It seems no one is caching the pref
service with a static in this way -- it doesn't seem to be an issue.

Comment 9

[shliang]

Comment on attachment 76762 [details] [diff] [review]
Forgot file - contains htmlBindings.xml which removes Accel+Shift+K binding

sr=hewitt

Comment 10

[Brian Ryner (not reading)]

Comment on attachment 76762 [details] [diff] [review]
Forgot file - contains htmlBindings.xml which removes Accel+Shift+K binding

r=bryner

Comment 11

[Boris Zbarsky [:bzbarsky]]

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

Comment 12

[marlon bishop]

cc'ing Jatin, to take a look at this dialog.

Comment 13

[Asa Dotzler [:asa]]

Comment on attachment 76762 [details] [diff] [review]
Forgot file - contains htmlBindings.xml which removes Accel+Shift+K binding

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 14

[Jatin Billimoria]

My suggested wording, if there is still time:

Pressing F7 turns Caret Browsing on or off. This feature places a moveable
cursor in web pages, allowing you to select text with the keyboard. Do you want
to turn Caret Browsing on?

[ Yes ]        [ Cancel ]

Comment 15

[Aaron Leventhal]

I have changed this to Jatin's wording. Question is whether I need r=/sr= or a=
again for the fixed wording.

Comment 16

[Aaron Leventhal]

Attachment: Wording only change. Need new r= and a= only. Brendan gives blanked sr='s for wording only changes.

Comment 17

[Aaron Leventhal]

Comment on attachment 77075 [details] [diff] [review]
Wording only change. Need new r= and a= only. Brendan gives blanked sr='s for wording only changes.

Adding brendan's universal sr= for wording changes

Comment 18

[Brian Ryner (not reading)]

Comment on attachment 77075 [details] [diff] [review]
Wording only change. Need new r= and a= only. Brendan gives blanked sr='s for wording only changes.

r=bryner

Comment 19

[Aaron Leventhal]

Comment on attachment 77075 [details] [diff] [review]
Wording only change. Need new r= and a= only. Brendan gives blanked sr='s for wording only changes.

Adding sr= via brendan's universal sr= for wording-only changes

Comment 20

[marlon bishop]

a=marlon

Comment 21

[Jaime Rodriguez, Jr.]

adt1.0.0+, pending L10N and UE. Adding mcarlson, and lorikaplan to cc: list.

Comment 22

[Michele Carlson]

l10n approved. please check it in quick! thanks

Comment 23

[Aaron Leventhal]

fix checked in

Comment 24

[Lori Kaplan]

a=UI 

Comment 25

[sairuh (rarely reading bugmail)]

vrfy'd fixed using 2002.04.09 comm bits on linux rh7.2, win2k and mac 10.1.3.

clarifying the summary, since the dlg *only* appears when i turn *on* caret
browsing. (spoke to aaronl, who said that's by design.)

Comment 26

[Aaron Leventhal]

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

Comment 27

[Matthew T (active 1999-2002)]

This should be backed out, as it makes the feature completely inaccessible on
Mac OS.

(I could wonder how on earth this got UI approval when (a) it has buttons
labelled `Yes' and `Cancel' and (b) it includes a checkbox label ending in a
period, but such bugs are trivial in comparison.)

Comment 28

[Jeremy M. Dolan]

Well, I collided with mpt... the only other comment I had was:

why is "BUTTON_TITLE_IS_STRING" used, then the string "Yes" is looked up?
_CANCEL is used for the other button... when not _YES?

Comment 29

[Matthew T (active 1999-2002)]

Attachment: What the dialog looks like when you press F7 on Mac OS

Comment 30

[Matthew T (active 1999-2002)]

Attachment: Mini-spec for the alert

This design doesn't mention what the keyboard shortcut is, because the string
would need to be made platform-specific (Shift+Command+K on the Mac,
Shift+Control+K elsewhere). Doing that isn't impossible (the Tabbed Browsing
panel in the prefs does it, for example), but not worth implementing, given
that this is a short-term fix until a proper fix is implemented where the alert
doesn't exist at all.

A proper fix would put this function in the prefs dialog (with no alert),
rather than having a keyboard shortcut, and would use Alt+Up/Down or similar
for normal scrolling when the function is turned on.

Comment 31

[Jeremy M. Dolan]

Aaronl, one more thing... why were the strings for the dialog placed in
tabbrowser.properties?

Do any other browsers include this accessability feature?

Comment 32

[Jaime Rodriguez, Jr.]

removing adt1.0.0+, as this bug has been reopened.

Comment 33

[Michele Carlson]

removing l10n approval since it's too late for UI changes in the branch.

Comment 34

[Aaron Leventhal]

tabbrowser.properties made the most sense for the string. I believe that
properties file is still included even in embedded projects, but we should check
to be sure. I didn't want to create a new properties file for this 1 string.

I agree this feature could use some tweaking. 

The reason a keystroke toggle is important, is that this feature can be used for
keyboard only users to go into a mode where they can select text, and then leave
that mode.

I don't mind mentionion "Start" as opposed to "Yes". However, I think it's
important to mention the keystroke so that people don't get lost in the feature.
Possibly the best idea is to make sure the feature always starts off when the
browser starts up. That way no one would ever get stuck in this.

This is an accessibility feature. I don't mind not having a great keystroke for
Mac users on an accessibility feature such as this, because Mac is still not an
accessible platform, far from it in fact. When Mac becomes accessible, I'll be
more inclined to worry about having quick toggles for accessibility features.
Therefore, it's a good time to use a function key.

Comment 35

[Jonas Jørgensen]

mpt in comment 30:
> A proper fix would put this function in the prefs dialog (with no alert),
> rather than having a keyboard shortcut, and would use Alt+Up/Down or similar
> for normal scrolling when the function is turned on.

I'd really, really, really like this in the Edit or View menu as well.

Comment 36

[Steve]

Hello

Crashing:
When using F7, it is crashing the browser.
Pressing F7
toggles it on and off. It is crashing while turning it off 
In the mail program it is not doing this.

Mozilla 1.3a

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3a) Gecko/20021121

I see that this bug is set as blocking 1.3a so sorry if this is a dup report. 

Comment 37

[Roman Neuhauser]

<aol>Yeah, me too</aol>
Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.3a) Gecko/20030211 Phoenix/0.5

same with my colleague, he uses 1.3a on Windows XP.

Comment 38

[Steve]

Bug in Bugzilla.
While I do have my email address as CC in a few bugs,
I have never added myself to this bug's CC list.

I always post some comment while adding myself to a bug.
You will find no prier posting here from me.

I am removing myself from this bug's CC list and ,as always, posting
about it.

Have a nice day.

Comment 39

[sairuh (rarely reading bugmail)]

see also bug 165362.

Comment 40

[Alex Vincent [:WeirdAl]]

aaronl:
Warning: assignment to undeclared variable promptService
Source File: 
Line: 17

http://lxr.mozilla.org/seamonkey/source/xpfe/global/resources/content/bindings/browser.xml#348
Needs a var keyword.

Nothing significant on this bug has happened in a while; I can and will create
the patch for the warning momentarily.  But should this bug stay open?  Does it
crash current builds?  If so, can we get a Talkback ID?

Comment 41

[Alex Vincent [:WeirdAl]]

Attachment: patch for strict warning

Comment 42

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 120347 [details] [diff] [review]
patch for strict warning

Next time, please do 'diff -u7' or more.  Context is good.  In this case it may
have let me not have to go look up the code in the file...

Comment 43

[Paul Wyskoczka]

ADT: Nominating topembed

Comment 44

[Michael Buckland]

Discussed in top embed bug triage.  Plussing.  John AAron, can you do the review?

Comment 45

[John Gaunt (redfive)]

Comment on attachment 120347 [details] [diff] [review]
patch for strict warning

r=jgaunt

Comment 46

[Mike Connor [:mconnor]]

bug 207166 was submitted against Firebird to change Yes/Cancel to Yes/No.  The
following attachment is a patch for Firebird to address that request.  It also
replaces the BUTTON_TITLE_IS_STRING for the Yes to a simpler BUTTON_TITLE_YES
and removing the extra string from tabbrowser.properties.   Comment #28 here
noted the same issue.  Porting the patch is easy enough if you decide the
wording change is appropriate.

http://bugzilla.mozilla.org/attachment.cgi?id=126687&action=view

Comment 47

[Andreas Kunz]

Mike, this will probably be fixed in bug 232512.

This bug here is open because of mpt's comment #27, not because of any crashes
(there are other bugs about that).

Alex, bz: that strict warning patch in attachment 120347 [details] [diff] [review] has reviews, but not
yet gone in. Can anybody check this in?

Comment 48

[Aaron Leventhal]

There is a confirm dialog.

If tweaks need to be made to the UI, please open a new clean bug.