Skip to content

Modularise editable element text access#424

Closed
divec wants to merge 1 commit into
wikimedia:masterfrom
divec:text-entry
Closed

Modularise editable element text access#424
divec wants to merge 1 commit into
wikimedia:masterfrom
divec:text-entry

Conversation

@divec
Copy link
Copy Markdown
Contributor

@divec divec commented Dec 24, 2015

Move the code specific to each type of editable element text access into a
dedicated class, and use a factory pattern so a project using jQuery.IME can
register its own editable element text access implementation.

TextEntry,
TextEntry#getTextBeforeSelection( maxLength ),
TextEntry#replaceTextAtSelection( precedingCharCount, newText ):

  • Abstract base class with methods to read/replace editable element text

FormWidgetEntry,
ContentEditableEntry:

  • Concrete implementations for particular editable element types

TextEntryFactory,
TextEntryFactory#wrap( $element ),

  • Factory class with method to wrap editable element

$.ime.TextEntry,
$.ime.textEntryFactory:

  • Publicly accesible abstract base class and factory singleton

Comment thread src/jquery.ime.js
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Developers outside Wikimedia will not be familiar with OOJS. You can remove reference to OOJS and just state what really the method does- in this case having static members initizlied with empty object.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's worth mentioning that this is based on OOJS so in future we don't add code which may conflict with it, and therefore leave the door open to actually using it.

@santhoshtr
Copy link
Copy Markdown
Member

Very good improvements. Thanks.

I get 5397 assertions of 6061 passed, 664 failed. while running tests in FF(Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0 Iceweasel/45.0a2). There are some 8 tests that are failing in Chrome but they are real failures because of input method mappings. So rest of the failures seems to be a regression for Firefox.

A sample is given below. All CE tests return blank output.

Odia InScript ka -> କୋ - contenteditable
Expected:   

"କୋ"

Result:     

""

Diff:   

"କୋ" "" 

Source:     

imeTest/</<@http://localhost/jquery.ime/test/jquery.ime.test.js:308:1
x.Callbacks/c@http://ajax.googleapis.com/ajax/libs/jquery/1.10.1/jquery.min.js:4:26046
x.Callbacks/p.add@http://ajax.googleapis.com/ajax/libs/jquery/1.10.1/jquery.min.js:4:26361
imeTest/<@http://localhost/jquery.ime/test/jquery.ime.test.js:293:4

At the same time, when I tested manually in FF by typing something in the CE, I dont see any functionality broken.So I guess issue is in the testing instrumentation.

@divec
Copy link
Copy Markdown
Contributor Author

divec commented Jan 2, 2016

Hmmm, I think there's something strange going on with the tests on Firefox. I'm seeing 53 failures on this commit (d6778c8), versus 693 failures(!) on the prior commit (ea97e4b).

Server: cd src/jquery.ime && python -m SimpleHTTPServer
Browser: firefox http://localhost:8000/test/index.html
User agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:42.0) Gecko/20100101 Firefox/42.0

@divec
Copy link
Copy Markdown
Contributor Author

divec commented Jan 2, 2016

On a subsequent identical run on this commit ( d6778c8 ), I see only 8 failures on Firefox (the same 8 failures as on Chromium), so I think there's a race condition or something similar.

@divec divec force-pushed the text-entry branch 3 times, most recently from 9be365a to ca73b01 Compare January 5, 2016 22:43
Move the code specific to each type of editable element text access into a
dedicated class, and use a factory pattern so a project using jQuery.IME can
register its own editable element text access implementation.

TextEntry,
TextEntry#getTextBeforeSelection( maxLength ),
TextEntry#replaceTextAtSelection( precedingCharCount, newText ):
* Abstract base class with methods to read/replace editable element text

FormWidgetEntry:
* Concrete implementation for form widgets containing plain text.
* IE8 fallback compatibility

ContentEditableEntry:
* Concrete implementation for ContentEditable widgets
* Fires jQuery fake composition/input events, for clean interaction with other JS code

TextEntryFactory,
TextEntryFactory#wrap( $element ),
* Factory class with method to wrap editable element

$.ime.TextEntry,
$.ime.textEntryFactory,
$.ime.inheritClass:
* Publicly accesible abstract base class, factory singleton, and inheritance tool
@santhoshtr
Copy link
Copy Markdown
Member

Merged in commit 65cdfaf
Thanks!

@santhoshtr santhoshtr closed this Jan 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants