1302014 - XUL text-link label doesn't respond to accesskey

Status: RESOLVED WONTFIX

(Core :: DOM: Core & HTML, defect)

Description

[Fischer [:Fischer]]

Attachment: 0001-Add-test-xul-text-link-label.patch

STR:
1. Apply the 0001-Add-test-xul-text-linklabel.patch
2. Open about:preferences > General panel
3. Trigger accesskey "l" to open the "learn more" link on the General panel header.

Expected result:
The "learn more" page opens in an new tab.

Actual result:
The "learn more" page doesn't open.

Comment 1

[Fischer [:Fischer]]

Hi Olli,

In the about:preferences, there are many <label class="text-link"> links [1].
However, <label class="text-link"> doesn't respond to accesskey like html <a> link.
Currently only <label control="..."> is allowed to respond to accesskey.

That would be good to UX on these xul label links in the about:preferences.
Right now, I got a local test patch to enable accesskey on <label class="text-link">
Is it ok to enable accesskey on <label class="text-link"> or do we have any other concern?

Thank you

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Style/text-link

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

I think Neil would be the right person to answer to the question.


Anyhow, I don't understand what
 href="..." would do in <label> element. label isn't a link.
I guess href attribute is then read elsewhere and link followed there, right?
And what does enabling the accesskey do? How does <label> react when accesskey is pressed?
What if you just put an html:a element inside the <label>?

Comment 3

[:Gijs (he/him)]

(In reply to Olli Pettay [:smaug] from comment #2)
> I think Neil would be the right person to answer to the question.
> 
> 
> Anyhow, I don't understand what
>  href="..." would do in <label> element. label isn't a link.
> I guess href attribute is then read elsewhere and link followed there, right?

XUL label.text-link gets this binding:
http://searchfox.org/mozilla-central/rev/950e13cca1fda6507dc53c243295044e8eda4493/toolkit/content/widgets/text.xml#292

which implements the current behaviour. It's possible it could be adapted to deal with access keys.

OTOH, it's not necessary to use <label class="text-link"> for an action rather than for navigating to the href specified. Yes, you can abuse them with data: or javascript: URLs to use them for this purpose, but that is not why they exist.

> And what does enabling the accesskey do? How does <label> react when
> accesskey is pressed?

They activate (focus or press) the labeled control, if the label labels a control. This isn't the case here.

<label class=text-link> is a XUL/XBL hack to get the 'correct' behaviour (for middle clicks and modifier-clicks) for href links that you'd normally use html:a for, in XUL land.

Fischer, the patch in bug 1028029 doesn't change any <label class=text-link> items. It's also a bit unusual, IME, for plain HTML links to have access keys. Can you clarify where you need the access key behaviour to work to resolve bug 1028029?

Comment 4

[Fischer [:Fischer]]

(In reply to :Gijs Kruitbosch from comment #3)
> (In reply to Olli Pettay [:smaug] from comment #2)
> > I think Neil would be the right person to answer to the question.
> > 
> > 
> > Anyhow, I don't understand what
> >  href="..." would do in <label> element. label isn't a link.
> > I guess href attribute is then read elsewhere and link followed there, right?
> 
> XUL label.text-link gets this binding:
> http://searchfox.org/mozilla-central/rev/
> 950e13cca1fda6507dc53c243295044e8eda4493/toolkit/content/widgets/text.xml#292
> 
> which implements the current behaviour. It's possible it could be adapted to
> deal with access keys.
> 
> OTOH, it's not necessary to use <label class="text-link"> for an action
> rather than for navigating to the href specified. Yes, you can abuse them
> with data: or javascript: URLs to use them for this purpose, but that is not
> why they exist.
> 
> > And what does enabling the accesskey do? How does <label> react when
> > accesskey is pressed?
> 
> They activate (focus or press) the labeled control, if the label labels a
> control. This isn't the case here.
> 
> <label class=text-link> is a XUL/XBL hack to get the 'correct' behaviour
> (for middle clicks and modifier-clicks) for href links that you'd normally
> use html:a for, in XUL land.
> 
> Fischer, the patch in bug 1028029 doesn't change any <label class=text-link>
> items. It's also a bit unusual, IME, for plain HTML links to have access
> keys. Can you clarify where you need the access key behaviour to work to
> resolve bug 1028029?
The current bug 1028029 patch is out-dated and not aligned with the central code base.

In the about:preferences Sync panel, when logged-in, a link to https://accounts.firefox.com/settings would appear and right now that link is a xul label link (see [1]).
The bug 1028029 is to add accesskey on the buttons and links related to firefox account to enhance the usability, which is a fair and nice improvement.

Currently, xul label link doesn't respond to accesskey.
So to make the link to https://accounts.firefox.com/settings respond to accesskey, 2 possible approaches:
(1) Change xul label link to html <a> link:
    - This requires update markup and CSS.
    - Pros: Not touch DOM implementation
    - Cons: In the future, need to replace xul label link to html link for similar request.
(2) Enable accesskey on xul label link. 
    - This requires update DOM implementation.
    - Pros: In the future, just add accesskey on xul label link for similar request and no extra work is needed.
    - Cons: Touch the DOM implementation
   
I am not sure if the approach #2 is ok since touching DOM implementation affects whole platform.
So I would like to check if the approach #2 is good to go in this bug first, then proceed on the bug 1028029.

[1] https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/sync.xul#235

Comment 5

[:Gijs (he/him)]

(In reply to Fischer [:Fischer] from comment #4)
> In the about:preferences Sync panel, when logged-in, a link to
> https://accounts.firefox.com/settings would appear and right now that link
> is a xul label link (see [1]).
> The bug 1028029 is to add accesskey on the buttons and links related to
> firefox account to enhance the usability, which is a fair and nice
> improvement.
> 
> Currently, xul label link doesn't respond to accesskey.
> So to make the link to https://accounts.firefox.com/settings respond to
> accesskey, 2 possible approaches:
> (1) Change xul label link to html <a> link:
>     - This requires update markup and CSS.
>     - Pros: Not touch DOM implementation
>     - Cons: In the future, need to replace xul label link to html link for
> similar request.

I don't understand why this is a "con". I think if all that this is about is this 1 link to settings, and we are very convinced it needs an access key (I am actually not convinced, but there we are) then we should go ahead and just replace the label with a 'real' link.

> (2) Enable accesskey on xul label link. 
>     - This requires update DOM implementation.
>     - Pros: In the future, just add accesskey on xul label link for similar
> request and no extra work is needed.
>     - Cons: Touch the DOM implementation

<label class="text-link">'s behaviour isn't implemented in DOM, but in a toolkit XBL binding, as I pointed out in comment #3. I don't think it makes sense to unconditionally change the DOM implementation, or to hardcode a reference to the de facto "standard" XUL "text-link" class into the DOM code. If we want to implement access keys for this binding then that should probably be done in the XBL binding, too, though I defer to Neil if there are reasons not to do that that I am not aware of.

Comment 6

[Fischer [:Fischer]]

(In reply to :Gijs Kruitbosch from comment #5)
> (In reply to Fischer [:Fischer] from comment #4)
> > In the about:preferences Sync panel, when logged-in, a link to
> > https://accounts.firefox.com/settings would appear and right now that link
> > is a xul label link (see [1]).
> > The bug 1028029 is to add accesskey on the buttons and links related to
> > firefox account to enhance the usability, which is a fair and nice
> > improvement.
> > 
> > Currently, xul label link doesn't respond to accesskey.
> > So to make the link to https://accounts.firefox.com/settings respond to
> > accesskey, 2 possible approaches:
> > (1) Change xul label link to html <a> link:
> >     - This requires update markup and CSS.
> >     - Pros: Not touch DOM implementation
> >     - Cons: In the future, need to replace xul label link to html link for
> > similar request.
> 
> I don't understand why this is a "con". I think if all that this is about is
> this 1 link to settings, and we are very convinced it needs an access key (I
> am actually not convinced, but there we are) then we should go ahead and
> just replace the label with a 'real' link.
Yes, I agree. This is not necessary a con. To use 'real' html link is good for me too. 

> 
> > (2) Enable accesskey on xul label link. 
> >     - This requires update DOM implementation.
> >     - Pros: In the future, just add accesskey on xul label link for similar
> > request and no extra work is needed.
> >     - Cons: Touch the DOM implementation
> 
> <label class="text-link">'s behaviour isn't implemented in DOM, but in a
> toolkit XBL binding, as I pointed out in comment #3. I don't think it makes
> sense to unconditionally change the DOM implementation, or to hardcode a
> reference to the de facto "standard" XUL "text-link" class into the DOM
> code. If we want to implement access keys for this binding then that should
> probably be done in the XBL binding, too, though I defer to Neil if there
> are reasons not to do that that I am not aware of.
The accesskey of xul label registration is at [1]. It checks if it is a <label control="...">, then registering accesskey.
Performing accesskey is at [2]. It also checks if it is a <label control="...">, then triggering click.

Bu maybe there are some potential issue I don't see so I would like to get some advices.

[1] https://dxr.mozilla.org/mozilla-central/source/layout/xul/nsXULLabelFrame.cpp#41
[2] https://dxr.mozilla.org/mozilla-central/source/dom/xul/nsXULElement.cpp#612

Comment 7

[Fischer [:Fischer]]

(In reply to Fischer [:Fischer] from comment #6)
> (In reply to :Gijs Kruitbosch from comment #5)
> > (In reply to Fischer [:Fischer] from comment #4)
> > > In the about:preferences Sync panel, when logged-in, a link to
> > > https://accounts.firefox.com/settings would appear and right now that link
> > > is a xul label link (see [1]).
> > > The bug 1028029 is to add accesskey on the buttons and links related to
> > > firefox account to enhance the usability, which is a fair and nice
> > > improvement.
> > > 
> > > Currently, xul label link doesn't respond to accesskey.
> > > So to make the link to https://accounts.firefox.com/settings respond to
> > > accesskey, 2 possible approaches:
> > > (1) Change xul label link to html <a> link:
> > >     - This requires update markup and CSS.
> > >     - Pros: Not touch DOM implementation
> > >     - Cons: In the future, need to replace xul label link to html link for
> > > similar request.
> > 
> > I don't understand why this is a "con". I think if all that this is about is
> > this 1 link to settings, and we are very convinced it needs an access key (I
> > am actually not convinced, but there we are) then we should go ahead and
> > just replace the label with a 'real' link.
> Yes, I agree. This is not necessary a con. To use 'real' html link is good
> for me too. 
> 
> > 
> > > (2) Enable accesskey on xul label link. 
> > >     - This requires update DOM implementation.
> > >     - Pros: In the future, just add accesskey on xul label link for similar
> > > request and no extra work is needed.
> > >     - Cons: Touch the DOM implementation
> > 
> > <label class="text-link">'s behaviour isn't implemented in DOM, but in a
> > toolkit XBL binding, as I pointed out in comment #3. I don't think it makes
> > sense to unconditionally change the DOM implementation, or to hardcode a
> > reference to the de facto "standard" XUL "text-link" class into the DOM
> > code. If we want to implement access keys for this binding then that should
> > probably be done in the XBL binding, too, though I defer to Neil if there
> > are reasons not to do that that I am not aware of.
> The accesskey of xul label registration is at [1]. It checks if it is a
> <label control="...">, then registering accesskey.
> Performing accesskey is at [2]. It also checks if it is a <label
> control="...">, then triggering click.
In XBL binding, text-link label listens to click event then open link, See [3]. 
[3] https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/text.xml#380

> Bu maybe there are some potential issue I don't see so I would like to get
> some advices.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/layout/xul/nsXULLabelFrame.
> cpp#41
> [2]
> https://dxr.mozilla.org/mozilla-central/source/dom/xul/nsXULElement.cpp#612

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

This impact of this bug is very low and I would prefer if we focus our time on something that will benefit users more. Fischer, would you agree to closet this bug as wontfix?

Comment 9

[Fischer [:Fischer]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8)
> This impact of this bug is very low and I would prefer if we focus our time
> on something that will benefit users more. Fischer, would you agree to
> closet this bug as wontfix?
Yes, it is fine to mark this bug as WONTFIX since using html <a> link is a good option for accesskey request as well.