Closed Bug 454520 Opened 11 years ago Closed 5 years ago

Caret Browsing confirmation dialog ([F7]) should have "NO" as the default value

Categories

(Firefox :: Keyboard Navigation, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 32

People

(Reporter: test2, Assigned: Gijs)

Details

(Whiteboard: p=3 s=it-32c-31a-30b.3 [qa-])

Attachments

(1 file, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; ja; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; ja; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1

Most people do not need to use caret browsing mode.  Some of them can accidentaly push [F7] and [Enter].  After that they do not know how to "Undo" it.  Even restarting firefox do not get it back.

In Japanese Windows (and other OS), IME (input method editor) assigns [F7] as Katakana convertion toggle and [Enter] fixes it.  They hit [F7] - [Enter] combination quickly in that case.  People occasionaly hit that combination carelessly when IME is disabled. (Note that they enable and disable IME many times on browsing)

I am also one of users experienced that.  I wrote how to fix it on my blog.
Nearly 10 people comments of thanks.
(Japanese) http://nonn-et-twk.net/twk/node/109
(English Translation) http://translate.google.co.jp/translate?u=http%3A%2F%2Fnonn-et-twk.net%2Ftwk%2Fnode%2F109&hl=ja&ie=UTF-8&sl=ja&tl=en

See also
https://bugzilla.mozilla.org/show_bug.cgi?id=374292
https://bugzilla.mozilla.org/show_bug.cgi?id=358970


Reproducible: Always

Steps to Reproduce:
1.Press F7 (Caret browsing short cut)
2.You will see the confirmation dialog

Actual Results:  
The default value is "Yes" (enable the caret browsing mode)

Expected Results:  
The default value should be "No" (cancel the operation)

To fix the problem

http://mxr.mozilla.org/mozilla1.8.0/source/toolkit/content/widgets/browser.xml#864

864             var buttonPressed = promptService.confirmEx(window, 
865               this.mStrBundle.GetStringFromName('browsewithcaret.checkWindowTitle'), 
866               this.mStrBundle.GetStringFromName('browsewithcaret.checkLabel'),
867               (promptService.BUTTON_TITLE_YES * promptService.BUTTON_POS_0) +
868               (promptService.BUTTON_TITLE_NO * promptService.BUTTON_POS_1),
869               null, null, null, this.mStrBundle.GetStringFromName('browsewithcaret.checkMsg'),
870               checkValue);

should be

867               (promptService.BUTTON_TITLE_YES * promptService.BUTTON_POS_0) +
868               (promptService.BUTTON_TITLE_NO * promptService.BUTTON_POS_1) + BUTTON_POS_1_DEFAULT,
Which option gets the focus default depends on the system.
The focus on windows is always on yes, for apple AFAIk on no.

The hotkey should be changed if F7 is used as default action for something else in a japanese windows and/or the caret browsing should not survive a restart.
This bug was reported using Firefox 3.0 or older, which is no longer supported. The bug has also not been changed in over 500 days and is still in UNCO.
Reporter, please retest this bug in Firefox 3.6.10 or later using a fresh profile, http://support.mozilla.com/en-US/kb/managing+profiles. If you still see this problem, please update the bug. If you no longer see the bug, please set the resolution to RESOLVED, WORKSFORME.

This is a mass search of unconfirmed bugs that have no activity on them, so if you feel a bug was marked in error, just remove the CLOSEME comment in the whiteboard within the next month.
Whiteboard: [CLOSEME 2010-11-15]
Reproduceable in Firefox 3.6.10 (windows).
Over seventy people have said thanks to my blog entry at http://nonn-et-twk.net/twk/node/109.  It seems much more people have suffered this occation.
The default Windows Japanese IME assigns F7 key to Katakana convertion.  This problem is live.  Even if the default focus is not a good solution, you should have some way to recover (For example, by adding a default-unchecked checkbox to the dialog to specify the decision remains in the next firefox execution).
Version: unspecified → 3.6 Branch
Whiteboard: [CLOSEME 2010-11-15]
Confirming in the latest trunk build of Firefox.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: x86 → All
Version: 3.6 Branch → Trunk
Also, the remembering of the choice does not work (at least not if I check "Remember my choice" and press No.

On next F7 press, it's asking again. Does not remember my "NO".

I know this might be a separate bug, but I think it worsens this one as well, increasing the irritation (I'm on 13.0.1 for Linux Mint)
This switches the button default to 'No' and also makes the checkbox ('Don't show me this dialog again') not be a lie if you pick 'No'. David, I think this is an OK tradeoff considering most people don't use this feature (willingly), but I wonder if you can doublecheck if I'm not unduly biased. Flagging Jared for the actual code review.
Attachment #8428702 - Flags: review?(jaws)
Attachment #8428702 - Flags: review?(dbolter)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Comment on attachment 8428702 [details] [diff] [review]
fix caret browsing dialog to default to no and remember choices,

Review of attachment 8428702 [details] [diff] [review]:
-----------------------------------------------------------------

Defaulting to 'No' seems reasonable to me.
Attachment #8428702 - Flags: review?(dbolter) → feedback+
Comment on attachment 8428702 [details] [diff] [review]
fix caret browsing dialog to default to no and remember choices,

Review of attachment 8428702 [details] [diff] [review]:
-----------------------------------------------------------------

Seems like we should have had a test for this. Care to add one? The changes look fine otherwise.
Attachment #8428702 - Flags: review?(jaws)
Now with test.
Attachment #8430846 - Flags: review?(jaws)
Attachment #8428702 - Attachment is obsolete: true
Also, that was harder than I thought, which makes me worry about focus and the real world of try server. Try push: https://tbpl.mozilla.org/?tree=Try&rev=0e8403a2ac01
Comment on attachment 8430846 [details] [diff] [review]
fix caret browsing dialog to default to no and remember choices,

Review of attachment 8430846 [details] [diff] [review]:
-----------------------------------------------------------------

Quite a thorough test, sorry for putting you through writing all of that.

::: toolkit/content/tests/browser/browser_f7_caret_browsing.js
@@ +122,5 @@
> +  let promiseGotKey = promiseCaretPromptOpened();
> +  hitF7();
> +  let prompt = yield promiseGotKey;
> +  let doc = prompt.document;
> +  is(doc.documentElement.defaultButton, "cancel", "No button should be the default");

single quotes around 'No' so that it doesn't read: "there should not be a button that is default" :)
Attachment #8430846 - Flags: review?(jaws) → review+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #11)
> Comment on attachment 8430846 [details] [diff] [review]
> fix caret browsing dialog to default to no and remember choices,
> 
> Review of attachment 8430846 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Quite a thorough test, sorry for putting you through writing all of that.

No worries, at least now we have some coverage...

> ::: toolkit/content/tests/browser/browser_f7_caret_browsing.js
> @@ +122,5 @@
> > +  let promiseGotKey = promiseCaretPromptOpened();
> > +  hitF7();
> > +  let prompt = yield promiseGotKey;
> > +  let doc = prompt.document;
> > +  is(doc.documentElement.defaultButton, "cancel", "No button should be the default");
> 
> single quotes around 'No' so that it doesn't read: "there should not be a
> button that is default" :)

Oops, good point. Changed throughout.

http://hg.mozilla.org/integration/fx-team/rev/60ed415ed0ab

Marco, can you add this to this sprint? (I thought I'd done that already with this bug, but I must be confused with another one...)
Flags: needinfo?(mmucci)
Whiteboard: p=3 [fixed-in-fx-team]
Added to Iteration 32.3.  Gijs, can you recommend if this bug should be marked as [qa-] or [qa+] for QA verification.
Flags: needinfo?(mmucci)
Whiteboard: p=3 [fixed-in-fx-team] → [fixed-in-fx-team] p=3 s=it-32c-31a-30b.3 [qa?]
This has automated test coverage so we should be OK here.
Whiteboard: [fixed-in-fx-team] p=3 s=it-32c-31a-30b.3 [qa?] → [fixed-in-fx-team] p=3 s=it-32c-31a-30b.3 [qa-]
https://hg.mozilla.org/mozilla-central/rev/60ed415ed0ab
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team] p=3 s=it-32c-31a-30b.3 [qa-] → p=3 s=it-32c-31a-30b.3 [qa-]
Target Milestone: --- → Firefox 32
Status: RESOLVED → VERIFIED
Unfortunately there have been two intermittent failures on Linux x64 due to timeouts, in not too many pushes:
https://tbpl.mozilla.org/php/getParsedLog.php?id=40857044&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=40875525&tree=Mozilla-Inbound

So I've backed this out for now:
remote:   https://hg.mozilla.org/integration/fx-team/rev/8a1406fb9ff4
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
(In reply to Ed Morley [:edmorley UTC+0] from comment #16)
> Unfortunately there have been two intermittent failures on Linux x64 due to
> timeouts, in not too many pushes:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=40857044&tree=Fx-Team
> https://tbpl.mozilla.org/php/getParsedLog.php?id=40875525&tree=Mozilla-
> Inbound
> 
> So I've backed this out for now:
> remote:   https://hg.mozilla.org/integration/fx-team/rev/8a1406fb9ff4

:-(

I'm not sure what would cause this offhand, and will try to look at it in more detail tomorrow...
Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/8a1406fb9ff4
Target Milestone: Firefox 32 → ---
Attachment #8430846 - Attachment is obsolete: true
I can reproduce the linux failure locally. The new patch wfm (changed how I'm dealing with focus). I'm pushing to try, and then planning on repushing to fx-team, assuming try is green.

remote:   https://tbpl.mozilla.org/?tree=Try&rev=ceb6909d728b
https://hg.mozilla.org/mozilla-central/rev/81d79924f919
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.