Opened 9 years ago

Closed 4 years ago

#12701 closed enhancement (patchwelcome)

[patch] [cla] [needs update and work] RTL/LTR keys mapping abstraction

Reported by: Jonathan Bond-Caron Owned by: bill
Priority: low Milestone: 1.13
Component: Dijit Version: 1.6.0
Keywords: Cc:
Blocked By: Blocking:

Description

There's quite a bit of that checks for "RTL" or "LTR" keys:

e.g.

map[this.isLeftToRight() ? dk.LEFT_ARROW : dk.RIGHT_ARROW]="_onLeftArrow";

This patch adds an abstraction which can simplify code.

dk = widget.getKeyMapping(); map[dk.UDR_LEFT_ARROW]="_onLeftArrow";

"UDR" stands for user direction.

Attachments (3)

UDR_keys.patch (891 bytes) - added by Jonathan Bond-Caron 9 years ago.
UDR event codes abstraction
12701.patch (747 bytes) - added by Katie Vance 8 years ago.
Use dojo.delegate to augment key mapping
12701.2.patch (1.5 KB) - added by Katie Vance 8 years ago.
Move to a11y

Download all attachments as: .zip

Change History (14)

Changed 9 years ago by Jonathan Bond-Caron

Attachment: UDR_keys.patch added

UDR event codes abstraction

comment:1 Changed 9 years ago by Jonathan Bond-Caron

See ticket #12700 for use case.

There's probably another/better? way of doing this but IMHO it simplifies keyboard handling (not having to think about RTL).

This could be used in dijit widgets.

comment:2 Changed 9 years ago by bill

Milestone: tbdfuture
Summary: CLA: RTL/LTR keys mapping abstraction[patch] [cla] RTL/LTR keys mapping abstraction

Thanks for the patch. I hate to bloat _WidgetBase but perhaps it makes for an overall savings. Would want to convert the existing code to use the new feature to see the savings.

The main issue I see with your patch though is that different widgets LTR and RTL widgets can simultaneously exist on the same page (it's a feature that would theoretically be used for portals where only some of the portlets are translated into Hebrew/Arabic? and the others are in English). Yet your patch is overwriting the data in the global dojo.keys hash.

Probably should use dojo.delegate() to setup and LTR keys and an RTL keys, and then getKeys() (or keys() or keys) would return one or the other depending on the widgets direction.

Also, typically we'd call them PREV_ARROW and NEXT_ARROW.

comment:3 Changed 9 years ago by bill

Component: Dijit - FormDijit
Owner: changed from Douglas Hays to bill

This isn't really specific to form widgets (although Doug can grab the ticket if he wants to).

Changed 8 years ago by Katie Vance

Attachment: 12701.patch added

Use dojo.delegate to augment key mapping

comment:4 Changed 8 years ago by Katie Vance

Reconfigured the patch to use dojo.delegate.

comment:5 Changed 8 years ago by bill

That's better, although a couple issues:

  • patch is accessing dojo.keys and dojo.delegate globals; there shouldn't be any references to the dojo namespace since the AMD conversion
  • patch assumes dojo/keys is loaded, but it might not be (note that _WidgetBase does not list dojo/keys as a dependency), and I don't think I want it to
  • patch can be more compact by putting the ternary inside of the dojo.delegate call

Not sure where the code should go, it could be a separate mixin like _WidgetKeyboardMixin although that seems a bit heavyweight, or maybe as part of a11y.js.

Changed 8 years ago by Katie Vance

Attachment: 12701.2.patch added

Move to a11y

comment:6 Changed 8 years ago by Katie Vance

a11y.js seemed like a good home, so I moved the code there. However, in doing so, I had no choice to make it a function because I no longer had access to the dijits isLeftToRight method. I can add to this patch and start using the new method once it's acceptable.

comment:7 Changed 8 years ago by bill

Could make a11y.js extend _WidgetBase same way as _BidiSupport.js or the dojox.mvc code does.

comment:8 Changed 8 years ago by Katie Vance

In that case, does it make sense to just move the method to _BidiSupport? It is bidi related.

comment:9 Changed 8 years ago by bill

Hmm, maybe. The two issues I see are:

  • Although we use "bidi" to refer to everything about supporting RTL text, apparently officially it only means supporting pages that simultaneously have both LTR and RTL text, and that's what _BidiSupport.js is for.
  • _BidiSupport is an optional module, it isn't loaded on all pages. Yet we need this.keys to be available all the time.

Still thinking about what makes sense. I suppose having a _WidgetKeysMixin wouldn't be too bad, it would replace dojo/keys in the dependency list for each module. Will give it some more thought.

comment:10 Changed 7 years ago by bill

Priority: highlow

comment:11 Changed 4 years ago by dylan

Milestone: future1.12
Resolution: patchwelcome
Status: newclosed
Summary: [patch] [cla] RTL/LTR keys mapping abstraction[patch] [cla] [needs update and work] RTL/LTR keys mapping abstraction

This patch was unfortunately never finished, but would have been a good addition to make. Closing as patchwelcome, but please reopen if someone wants to revisit it and work on a pull request per our contribution guidelines at https://github.com/dojo/dojo/blob/master/CONTRIBUTING.md

Note: See TracTickets for help on using tickets.