Missing focus when selected the related checkbox or radiobox in Preference window.

REOPENED
Assigned to

Status

()

Core
Keyboard: Navigation
REOPENED
15 years ago
6 years ago

People

(Reporter: Jessie Li, Assigned: Jessie Li)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed1.3)

Attachments

(2 attachments, 1 obsolete attachment)

2.77 KB, patch
Jessie Li
: review+
jag (Peter Annema)
: superreview+
Details | Diff | Splinter Review
10.09 KB, patch
neil@parkwaycc.co.uk
: review+
jag (Peter Annema)
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

15 years ago
If one checkbox option include an editable text field, when the checkbox is
check, the focus should be in the related text field.

In preference, the following textfield need the focus:
Privacy&Security->Cookies
Offline& Disk Space
Advanced
Advanced->Mouse Wheel
(Assignee)

Comment 1

15 years ago
taking
Assignee: aaronl → jessie.li
(Assignee)

Comment 2

15 years ago
Created attachment 114778 [details] [diff] [review]
patch
(Assignee)

Comment 3

15 years ago
Comment on attachment 114778 [details] [diff] [review]
patch

Kyle, it is a simple patch, plese review it for me, thanks!
Attachment #114778 - Flags: review?(kyle.yuan)
make sure jag or ben sr this....

Comment 5

15 years ago
Comment on attachment 114778 [details] [diff] [review]
patch

r=kyle
two nits:
1. please do not change the signature of hwaara, it looks good for me.
2. please keep the indent consistent with the present code like this:
if (!foo)
  foo();
Attachment #114778 - Flags: review?(kyle.yuan) → review+
(Assignee)

Updated

15 years ago
Attachment #114778 - Flags: superreview?(jaggernaut)
(Assignee)

Updated

15 years ago
Blocks: 193068
No longer blocks: 193265

Comment 6

15 years ago
I don't see why you need to add those oninput lines to the textboxes. You should
only have to set the focus when clicking on the radiobutton/checkbox.

If this is a generic behaviour we want, should we somehow codify that in the
markup, similar to how labels specify which control they're a label for:

<radiobutton control="id-of-textbox" label="Radio with textbox"/>
<textbox id="id-of-textbox"/>
(Assignee)

Comment 7

15 years ago
Hi jag,

You see, in pref-masterpass.xul file, it implemented like this:

<hbox align="center">
          <radio value="2" label="&managepassword.asktimeout;" id="askTimeout"
                 style="margin: 0px;"                       
oncommand="changePasswordSettings(true);"/>
          <textbox id="passwordTimeout" size="4"
                   preftype="int"
                   prefstring="security.password_lifetime"
                   oninput="changePasswordSettings(false);"/>
          <label value="&managepassword.timeout.unit;" style="margin: 4px;"/>
        </hbox>

Comment 8

15 years ago
jag, the oninput line is used for selecting the correspoding
checkbox/radiobutton while typing in textbox, but it seems useless because every
textbox is disabled befoce its checkbox/radiobutton got checked.

I still think using js to do this work is more flexible than using markup. 
(Assignee)

Comment 9

15 years ago
Hi jag,

I try using what you suggested:
<radiobutton control="id-of-textbox" label="Radio with textbox"/>
<textbox id="id-of-textbox"/>

But it can't add focus to the text field. 
So I think modifying the js is better, like this:
if (! el.disabled)
          el.focus();

And I will remove the <oninput>.

Please sr it for me again, thanks!
(Assignee)

Comment 10

15 years ago
Created attachment 114997 [details] [diff] [review]
new patch

It is modified as the last comments.
Attachment #114778 - Attachment is obsolete: true
(Assignee)

Comment 11

15 years ago
Comment on attachment 114997 [details] [diff] [review]
new patch

Please help me to sr it again. Thanks!
Attachment #114997 - Flags: superreview?(jaggernaut)
Attachment #114997 - Flags: review+

Comment 12

15 years ago
> I try using what you suggested:
> <radiobutton control="id-of-textbox" label="Radio with textbox"/>
> <textbox id="id-of-textbox"/>

the XUL markup I showed was a proposal that would have to be implemented for
this to work. I'll run this by some people to see what they think.

For now, the JS solution is simple and works, but to support this kind of UI
behaviour it might be better to do it through markup instead of making everyone
implement it in JS like you're doing now.

Comment 13

15 years ago
Comment on attachment 114997 [details] [diff] [review]
new patch

sr=jag iff you change those lines from

  if (! el.disabled)

to

  if (!el.disabled)
Attachment #114997 - Flags: superreview?(jaggernaut) → superreview+
(Assignee)

Updated

15 years ago
Attachment #114997 - Flags: approval1.3?

Comment 14

15 years ago
Comment on attachment 114997 [details] [diff] [review]
new patch

a=asa (on behalf of drivers) for checkin to 1.3
Attachment #114997 - Flags: approval1.3? → approval1.3+

Comment 15

15 years ago
checked in trunk with jag's comments. Will check in to 1.3 branch later
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
vrfy'd fixed with 2003.02.24 comm trunk bits on linux rh8.0 and win2k.
Status: RESOLVED → VERIFIED

Comment 17

15 years ago
just checked in MOZILLA_1_3_BRANCH

Comment 18

15 years ago
Sorry that I have to reopen this, but this code can potentially be called when
the pref panel loads, thus confusing the keyboard user. To reproduce the
problem, go to the advanced pref panel and tick "Send this email address...",
then go to another pref panel and back to the advanced pref panel. This error
was removed from another pref panel not so long ago, but I can't find where :-(
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 19

15 years ago
Created attachment 115609 [details] [diff] [review]
patch for comment18
(Assignee)

Updated

15 years ago
Attachment #115609 - Flags: review?(neil)

Comment 20

15 years ago
Comment on attachment 115609 [details] [diff] [review]
patch for comment18

Instead of having an extra parameter, I would like to see an extra function,
either just to set focus, or to call the existing function and set focus.
Attachment #115609 - Flags: review?(neil) → review-

Comment 21

15 years ago
Comment on attachment 115609 [details] [diff] [review]
patch for comment18

OK, so this follows the precedent in pref-masterpass.js; nice fix in
pref-offline.xul :-)
Attachment #115609 - Flags: review- → review+
(Assignee)

Updated

15 years ago
Attachment #115609 - Flags: superreview?(jaggernaut)

Comment 22

15 years ago
Comment on attachment 115609 [details] [diff] [review]
patch for comment18

sr=jag.

Thanks for catching this, Neil.

Jessie: nice catch on that ]]>
Attachment #115609 - Flags: superreview?(jaggernaut) → superreview+
(Assignee)

Updated

15 years ago
Attachment #115609 - Flags: approval1.3?

Comment 23

15 years ago
Comment on attachment 115609 [details] [diff] [review]
patch for comment18

check it in trunk.
(Assignee)

Comment 24

15 years ago
.
Status: REOPENED → RESOLVED
Last Resolved: 15 years ago15 years ago
Resolution: --- → FIXED

Comment 25

15 years ago
Comment on attachment 115609 [details] [diff] [review]
patch for comment18

a=asa (on ebahlf of drivers) for checkin to 1.3 branch.
Attachment #115609 - Flags: approval1.3? → approval1.3+

Comment 26

15 years ago
Comment on attachment 115609 [details] [diff] [review]
patch for comment18

check it in to the MOZILLA_1_3_BRANCH
oops, this is broken on Mac (OS X, 10.2.4). if i select (turn ON) the checkbox
via the keyboard (using spacebar), the assoicated textfield remains disabled.

strangely, it's not a problem if i select the checkbox with the mouse.

tested with 2003.03.03.15 comm trunk bits. then again, this was a problem with
the 2/24 (forgot to check there earlier, sorry). reopening, but let me know if
that's another issue.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 28

15 years ago
This should be another issue. My patch is cross-platform, it is just xul and js.
I can reproduce this problem in Mac with trunk 2003.2.10. So I think this bug is
not caused by my patch. BTW, I find Mac platform has a lot of keyboard
navigation bugs when I use it.

There is another way to define this bug. Could you find a mozilla build in Mac
without this problem, then put my patch in the build to see if it would cause
this problem. If not, please mark this bug verified. Thanks for your assistance.
(Assignee)

Comment 29

15 years ago
Hi Sairuh,

Have you tried my patch in Mac? If it is not due to my patch, please change the
status of this bug. If it is, I will check it again. Thanks!

Updated

15 years ago
Whiteboard: fixed1.3

Updated

14 years ago
Attachment #114778 - Flags: superreview?(jag)
QA Contact: bugzilla → keyboard.navigation
You need to log in before you can comment on or make changes to this bug.