Closed Bug 1291915 Opened 5 years ago Closed 2 years ago

Ensure commandkeys/a11y works and react to retranslation

Categories

(L20n :: JS Library, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: stas, Unassigned)

References

Details

(Whiteboard: [gecko-l20n])

I've seen this in bug 1288406.  Given the following translation:

    downloads-key =
        [xul/key]        { OS() ->
            [lin]    y
           *[other]  j
        }

and the XUL source:

    <key id="key_openDownloads" data-l10n-id="downloads-key" command="Tools:Downloads" modifiers="accel,shift"/>

the Ctrl+Shift+Y key shortcut is broken and doesn't trigger anything.  That said, I could see `key="y"` in the DOM inspector which suggests both OS() and the -> (the selection expression) worked fine.

    <key id="key_openDownloads" data-l10n-id="downloads-key" command="Tools:Downloads" modifiers="accel,shift" key="⁨y⁩"/>

This might be a nasty race condition.
Blocks: 1292127
No longer blocks: 1291693
I looked into this and it seems that the OS() works on linux and mac as expected.

The only reason I could find for the difference between linux and not-linux is the modifiers attribute.

On linux it's modifiers="accel", on macos/win it is modifiers="accel,shift".

If we can add it to whitelisted attributes, I believe it will work.
Flags: needinfo?(stas)
I'm glad that it works.  I filed this two months ago and don't remember the details.  Perhaps I made a mistake when testing or one of the m-c merges fixed this.  In any case thanks for looking into this in depth!

IIUC we have a choice of adding `modifiers` to the list of localizable XUL attributes vs continuing to use #ifdefs in the XUL code.  In both cases we could get rid of *-mac, *-win and *-unix entities.

Pike, what's you preference?
Flags: needinfo?(stas) → needinfo?(community)
Flags: needinfo?(community) → needinfo?(l10n)
I'm confused, I don't see how the modifiers do anything for us here, 'cause they didn't change at all.

Also, looking at a limited experiment with devtools, it seems the the key element doesn't actually listed to DOM changes :-/
Flags: needinfo?(l10n)
> I'm confused, I don't see how the modifiers do anything for us here, 'cause they didn't change at all.

So,

We have this ifdef: http://hg.mozilla.org/projects/larch/file/tip/browser/base/content/browser-sets.inc#l223
and we have those two entities: http://hg.mozilla.org/projects/larch/file/tip/browser/locales/en-US/browser/menubar.ftl#l300

Now, thanks to OS() macro we can reduce it to:

  <key id="key_openDownloads" data-l10n-id="downloads-key" command="Tools:Downloads" modifiers="accel"/>


    downloads-key =
        [xul/key]        { OS() ->
            [lin]    y
           *[other]  j
        }

but the problem we face is that in the current code modifiers are different for linux and non-linux.

:stas is giving two solutions:

1) We can preserve the ifdef preprocessing instruction
2) We can add `modifiers` to allowed attributes and do:

    downloads-key =
        [xul/key]        { OS() ->
            [lin]    y
           *[other]  j
        }
        [xul/modifiers]  { OS() ->
            [lin]    accel,shift
           *[other]  accel
        }

What is your preference?
Flags: needinfo?(l10n)
I'm torn on changing how we do modifiers, just because localizers usually get things like that wrong.

This bug is about the control keys working the way that l20n.js sets them. The strongest way to test that that's true is to have a language switch that introduces a different set of keybindings. Can we test that that works?
Flags: needinfo?(l10n)
That involves packaging, I wonder if we can avoid that. How about we create a new translation with different attribute values and setAttribute its id on the element?
Yeah, that'd be another code path that we can use to check this.
I was able to verify that the changes to the <key> element's attributes take effect only if the parent <keyset> is explicitly re-added to the document.

    https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/key#Related

I tested with:

    const key = document.getElementById('key_viewSource');
    const keyset = key.parentNode;
    keyset.parentNode.appendChild(keyset);

My further testing suggests that our current implementation might be racy.  If I delay l20n's startup logic by a few seconds, after it runs some command keys work as expected (Accel+T) while others break (Accel+U).  Actually most of them break.  In fact, I'm not completely sure why this works correctly on larch right now.  Is waiting for readyState === 'complete' early enough?
Blocks: 1280669
Summary: Using OS() with [xul/key] breaks the key shortcut → Ensure commandkeys/a11y works and react to retranslation
No longer blocks: 1292127
David, can you help us figuring out how to make a11y pick up the changes we make to the DOM at runtime?

The bug took a few turns in its history, but comment 8 reflects the actual question at this point.
Flags: needinfo?(dbolter)
Eitan can you check this out?
Flags: needinfo?(dbolter) → needinfo?(eitan)
(In reply to Axel Hecht [:Pike] from comment #9)
> David, can you help us figuring out how to make a11y pick up the changes we
> make to the DOM at runtime?
> 

What do you mean by "a11y"? Are you asking if a retranslation changes the value of nsIAccessible.keyboardShortcut, and if not why?

From the comments it looks like you're asking more generally how to get XUL to behave and actually work with a shortcut change.

If its the latter, I think someone more familiar with XUL would have an answer. I have the larch branch built, so I can do some digging if needed.

How do developers typically trigger a retranslation? Are there docs for that?
Flags: needinfo?(eitan) → needinfo?(l10n)
Retranslation is something we're actually introducing with l20n, so there's no typical way to do that yet. In particular because we don't have corresponding localizations to our code on larch yet.

With our code, the document is actually filled with English (or localized) strings asynchronously, so anything that's assuming that the document doesn't change after the initial parsing can regress. bug 1292527 is a good example on how this impacts platform integration.

We seem to have at least impact on command keys, but there's way more accessibility tech which integrates into our various platforms, so we're hoping for some help from you as you'd know the expected behaviour.

At least command keys, accesskeys and screenreaders come to my mind, not sure if there's more?
Flags: needinfo?(l10n)
Flags: needinfo?(eitan)
Sorry, I don't have the XUL knowledge to help here. Unflagging the ni so I don't block here.
Flags: needinfo?(eitan)

This is now fixed.

Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.