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

Status: REOPENED

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

Description

[Jessie Li]

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

Comment 1

[Jessie Li]

taking

Comment 2

[Jessie Li]

Attachment: patch

Comment 3

[Jessie Li]

Comment on attachment 114778 [details] [diff] [review]
patch

Kyle, it is a simple patch, plese review it for me, thanks!

Comment 4

[Boris Zbarsky [:bzbarsky]]

make sure jag or ben sr this....

Comment 5

[Kyle Yuan]

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();

Comment 6

[jag (Peter Annema)]

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"/>

Comment 7

[Jessie Li]

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

[Kyle Yuan]

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. 

Comment 9

[Jessie Li]

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!

Comment 10

[Jessie Li]

Attachment: new patch

It is modified as the last comments.

Comment 11

[Jessie Li]

Comment on attachment 114997 [details] [diff] [review]
new patch

Please help me to sr it again. Thanks!

Comment 12

[jag (Peter Annema)]

> 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

[jag (Peter Annema)]

Comment on attachment 114997 [details] [diff] [review]
new patch

sr=jag iff you change those lines from

  if (! el.disabled)

to

  if (!el.disabled)

Comment 14

[Asa Dotzler [:asa]]

Comment on attachment 114997 [details] [diff] [review]
new patch

a=asa (on behalf of drivers) for checkin to 1.3

Comment 15

[Pete Zha]

checked in trunk with jag's comments. Will check in to 1.3 branch later

Comment 16

[sairuh (rarely reading bugmail)]

vrfy'd fixed with 2003.02.24 comm trunk bits on linux rh8.0 and win2k.

Comment 17

[Pete Zha]

just checked in MOZILLA_1_3_BRANCH

Comment 18

[[e-mail]]

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 :-(

Comment 19

[Jessie Li]

Attachment: patch for comment18

Comment 20

[[e-mail]]

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.

Comment 21

[[e-mail]]

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 :-)

Comment 22

[jag (Peter Annema)]

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

sr=jag.

Thanks for catching this, Neil.

Jessie: nice catch on that ]]>

Comment 23

[Gilbert Fang]

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

check it in trunk.

Comment 24

[Jessie Li]

.

Comment 25

[Asa Dotzler [:asa]]

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

a=asa (on ebahlf of drivers) for checkin to 1.3 branch.

Comment 26

[Gilbert Fang]

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

check it in to the MOZILLA_1_3_BRANCH

Comment 27

[sairuh (rarely reading bugmail)]

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.

Comment 28

[Jessie Li]

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.

Comment 29

[Jessie Li]

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!

Comment 30

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.