Closed Bug 1028029 Opened 10 years ago Closed 8 years ago

Improve accesskeys in Sync pane of in-content preferences

Categories

(Firefox :: Settings UI, defect)

Product:
Firefox
Component:
Settings UI
Version:
31 Branch
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: whimboo, Assigned: Fischer)

References

Details

Attachments

(2 files)

Bug 1028029 - Improve accesskeys in Sync pane of in-content preferences
58 bytes, text/x-review-board-request
jaws
: review+
Details
not-aligned-on-OSX.png
152.38 KB, image/png
Details 
Mozilla/5.0 (X11; Linux x86_64; rv:33.0) Gecko/20100101 Firefox/33.0 ID:20140619030203 CSet: f78e532e8a10

The in-content preferences are missing some accesskeys for the Sync pref pane.

When not logged in:
* Create Account
* Sign In
* Using an older version of Sync

When logged in:
* The Disconnect button
* Manage link

Also the accesskey 'c' for Device Name does not work. Looks like it focuses the label but not the text field. So the text field has to be manually focused.
Assignee: nobody → fliu
Comment on attachment 8781951 [details]
Bug 1028029 - Improve accesskeys in Sync pane of in-content preferences

https://reviewboard.mozilla.org/r/72262/#review69844

::: browser/components/preferences/in-content/sync.xul:238
(Diff revision 1)
>                  <label id="fxaDisplayName" hidden="true"/>
>                  <label id="fxaEmailAddress1"/>
>                  <hbox class="fxaAccountBoxButtons" align="center">
> -                  <button id="fxaUnlinkButton" label="&disconnect.label;"/>
> -                  <html:a id="verifiedManage" target="_blank"
> +                  <button id="fxaUnlinkButton" label="&disconnect.label;" accesskey="&disconnect.accesskey;"/>
> +                  <html:a id="verifiedManage" target="_blank" accesskey="&verifiedManage.accesskey;"
>                      onkeypress="gSyncPane.openManageFirefoxAccount(event);"

<html:a id="verifiedManage"> is based on the unlanded patch of Bug id="verifiedManage".
The current code in the central is <label id="verifiedManage" class="text-link">. If using <label>, the accesskey does not function.
Comment on attachment 8781951 [details]
Bug 1028029 - Improve accesskeys in Sync pane of in-content preferences

https://reviewboard.mozilla.org/r/72262/#review69844

> <html:a id="verifiedManage"> is based on the unlanded patch of Bug id="verifiedManage".
> The current code in the central is <label id="verifiedManage" class="text-link">. If using <label>, the accesskey does not function.

Correct:
<html:a id="verifiedManage"> is based on the unlanded patch of Bug 1120967.
The current code in the central is <label id="verifiedManage" class="text-link">. If using <label>, the accesskey does not function.
Comment on attachment 8781951 [details]
Bug 1028029 - Improve accesskeys in Sync pane of in-content preferences

Hi Jared,

This simple patch adds the accesskey attribute on the buttons and the links under the Firefox Account section.

- The accesskey values are added into the sync.dtd. Is it enough or other process required for l10n handling ?

- This patch is based on the unlanded patch of Bug 1120967.
Because the accesskey does not function on <label id="verifiedManage" class="text-link"> but functions on <html:a id="verifiedManage">, I write t
his patch based on Bug 1120967 patch.
(In Bug 1120967, label#verifiedManage is replaced by html:a#verifiedManage)

Thank you
Attachment #8781951 - Flags: feedback?(jaws)
I will wait to give feedback on this patch until we can sort out the issues in bug 1120967.
Comment on attachment 8781951 [details]
Bug 1028029 - Improve accesskeys in Sync pane of in-content preferences

https://reviewboard.mozilla.org/r/72262/#review75298

Now that bug 1120967 has been r+'d this will likely need to be rebased.

::: browser/locales/en-US/chrome/browser/preferences/sync.dtd:74
(Diff revision 1)
>  <!ENTITY signedInLoginFailure.beforename.label "Please sign in to reconnect">
>  <!ENTITY signedInLoginFailure.aftername.label "">
>  
>  <!ENTITY notSignedIn.label           "You are not signed in.">
>  <!ENTITY signIn.label                "Sign in">
> +<!ENTITY signIn.accesskey            "G">

this should be lowercase "g"
Will update the patch after the bug 1302014 is resolved.
Depends on: 1302014
Comment on attachment 8781951 [details]
Bug 1028029 - Improve accesskeys in Sync pane of in-content preferences

@Jaws,
> Also the accesskey 'c' for Device Name does not work. Looks like it focuses the label but not the text field. So the text field has to be manually focused.
The Device Name input area is disabled by default. User has to trigger, by mouse click or accesskey, the #fxaChangeDeviceName button to enable the Device Name input area, then when the input area loses focus, user can focus back to it by the accesskey 'c'.

Thank you
Attachment #8781951 - Flags: review?(jaws)
Comment on attachment 8781951 [details]
Bug 1028029 - Improve accesskeys in Sync pane of in-content preferences

https://reviewboard.mozilla.org/r/72262/#review77380

::: browser/components/preferences/in-content/sync.xul:321
(Diff revision 4)
>          <label accesskey="&syncDeviceName.accesskey;"
>                 control="fxaSyncComputerName">
>            &fxaSyncDeviceName.label;
>          </label>

This label should probably be hooked up to the fxaChangeDeviceName so that using the access key will enable the field and place focus there. Will that work?

::: browser/themes/shared/incontentprefs/preferences.inc.css:500
(Diff revision 4)
> +  display: flex;
> +  align-items: center;

I removed these two lines and didn't see any difference to the Signed Out view. What are they needed for? I assume you added this because you removed the align="center" attribute but I'm not seeing any changes with or without that attribute.
Comment on attachment 8781951 [details]
Bug 1028029 - Improve accesskeys in Sync pane of in-content preferences

https://reviewboard.mozilla.org/r/72262/#review77606

Basically, we shouldn't have an accesskey that doesn't work in the default case. Let's see if we can fix that one access key, and if not let's remove it.
Attachment #8781951 - Flags: review?(jaws) → review-
Attached image not-aligned-on-OSX.png — 

    
    

    
  
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #12)
> Comment on attachment 8781951 [details]
> Bug 1028029 - Improve accesskeys in Sync pane of in-content preferences
> 
> https://reviewboard.mozilla.org/r/72262/#review77380
> 
> ::: browser/components/preferences/in-content/sync.xul:321
> (Diff revision 4)
> >          <label accesskey="&syncDeviceName.accesskey;"
> >                 control="fxaSyncComputerName">
> >            &fxaSyncDeviceName.label;
> >          </label>
> 
> This label should probably be hooked up to the fxaChangeDeviceName so that
> using the access key will enable the field and place focus there. Will that
> work?
> 

When pressing accesskey, actually underneath, it is triggered and a call to focus the textbox#fxaSyncComputerName is invoked [1]. However, the textbox is disabled so nothing happens. 

For HTML label and input, the case is similar that nothing happens when input is disabled. The difference is that accesskey would dispatch mouse click event on label [2] so still could enable input inside label’s click event. No way to do this for XUL label because it doesn’t dispatch click event but only try to focus on associated textbox.

One possible workaround is to replace that “Device Name” label with button and set its style like a label (But not a clean fix I think).
Maybe, just like your comment #13, just remove this accesskey and since there is already a accesskey on the button#fxaChangeDeviceName to focus the textbox#fxaSyncComputerName.

How do you think?

[1] https://dxr.mozilla.org/mozilla-central/source/dom/xul/nsXULElement.cpp#668
[2] https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLLabelElement.cpp#253

> ::: browser/themes/shared/incontentprefs/preferences.inc.css:500
> (Diff revision 4)
> > +  display: flex;
> > +  align-items: center;
> 
> I removed these two lines and didn't see any difference to the Signed Out
> view. What are they needed for? I assume you added this because you removed
> the align="center" attribute but I'm not seeing any changes with or without
> that attribute.
This is to make sure the text aligned-center vertically on OS X. See not-aligned-on-OSX.png.
Flags: needinfo?(jaws)

    
    

    
  
Thanks for the screenshot and thanks for the detailed explanation about the accesskey behavior. We should just remove the access key on the groupbox caption.
Flags: needinfo?(jaws)

    
    

    
      
  

    
  

        Attachment #8781951 -
        Flags: review?(jaws)

    
    

    
  
Comment on attachment 8781951 [details]
Bug 1028029 - Improve accesskeys in Sync pane of in-content preferences

https://reviewboard.mozilla.org/r/72262/#review79150

        Attachment #8781951 -
        Flags: review?(jaws) → review+

    
    

    
  
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/09bb9527ecc0
Improve accesskeys in Sync pane of in-content preferences. r=jaws
Keywords: checkin-needed

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/09bb9527ecc0
Status: NEW → RESOLVED
Closed: 8 years ago

          status-firefox52:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52

    
    

    
  
There's only one problem with this patch: naming is now completely inconsistent.

STRINGX.label should have a STRINGX.accesskey, not a STRINGX.label.accesskey, localization tools will have no idea how to match those two strings at this point. Those *.label.accesskey should be renamed to *.accesskey
Flags: needinfo?(fliu)

    
    

    
  
I realize the NI is not really helpful, I'm going to clone this bug in a new one to update string IDs.
Flags: needinfo?(fliu)
Depends on: 1305407

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: