Opened 14 years ago

Closed 14 years ago

#656 closed enhancement (fixed)

reduce repetitive usage of HTMLArea.is_ie test

Reported by: mokhet Owned by: mokhet
Priority: normal Milestone:
Component: Xinha Core Version: trunk
Severity: normal Keywords: performance is_ie
Cc:

Description

After i've submited the ticket #654, i've find a few places where the test should be done once while creating the object prototype and not everytime in the instance javascript at every call.

here is the list of the functions i've noticed that could be enhanced this way, i will submit a patch for them in a moment. I think this can improve a bit the performances since most of thoses functions are intensively used.

  • function stripTag(el) in HTMLArea.prototype._wordClean
  • HTMLArea.prototype.insertNodeAtSelection
  • HTMLArea.prototype.getParentElement
  • HTMLArea.prototype._activeElement
  • HTMLArea.prototype._selectionEmpty
  • HTMLArea.prototype.selectNodeContents
  • HTMLArea.prototype.insertHTML
  • HTMLArea.prototype.getSelectedHTML
  • HTMLArea.prototype.dom_checkBackspace and HTMLArea.prototype.ie_checkBackspace
    for those 2 functions, i have renamed them checkBackspace and instead of always creating both functions, i create only the one needed. In the dom_checkBackspace, i think it's weird to use a setTimeout to eventually cancel an event, it seems to work but still i think it's not a "solid" way to handle the case. Anyway, this is another topic :)
  • HTMLArea.prototype._getSelection
  • HTMLArea.prototype._createRange
  • HTMLArea.getOuterHTML

there is also HTMLArea._postback, HTMLArea._getback and HTMLArea._geturlcontent but i'll open another ticket later since there is more to do with thoses 3 functions.

I dont understand well how is working the function autoWrap in HTMLArea.prototype._editorEvent but i think there is a bug or at least useless code in changeset 477.

   if(HTMLArea.is_gecko)
    {
      var s = editor._getSelection()
      var autoWrap = function (textNode, tag)
      {

        if(HTMLArea.is_ie) <-- HERE
        {

How can (HTMLArea.is_ie) be ever TRUE when we are in a function define only when (HTMLArea.is_gecko) ? But as i said, i dont understand this part of the code.

Anyway, I really think this function (HTMLArea.prototype._editorEvent) needs also to be splitted in 2 functions between IE and other world (as in all the patches submited). Because we do 9 tests on (HTMLArea.is_ie) and 4 tests on (HTMLArea.is_gecko) in this function, this is too much specificity for a generic handler event imo, especially since it is used very often. This part of the code is really confuse for me, i'll try to make a patch but i cant promiss anything.

Attachments (12)

striptags.patch (794 bytes) - added by mokhet 14 years ago.
insertNodeAtSelection.patch (966 bytes) - added by mokhet 14 years ago.
getParentElement.patch (3.9 KB) - added by mokhet 14 years ago.
_activeElement.patch (1.2 KB) - added by mokhet 14 years ago.
_selectionEmpty.patch (927 bytes) - added by mokhet 14 years ago.
selectNodeContents.patch (1.5 KB) - added by mokhet 14 years ago.
insertHTML.patch (1.2 KB) - added by mokhet 14 years ago.
getSelectedHTML.patch (1.0 KB) - added by mokhet 14 years ago.
checkBackspace.patch (4.1 KB) - added by mokhet 14 years ago.
_getSelection.patch (676 bytes) - added by mokhet 14 years ago.
_createRange.patch (697 bytes) - added by mokhet 14 years ago.
getOuterHTML.patch (598 bytes) - added by mokhet 14 years ago.

Download all attachments as: .zip

Change History (27)

Changed 14 years ago by mokhet

Changed 14 years ago by mokhet

Changed 14 years ago by mokhet

Changed 14 years ago by mokhet

Changed 14 years ago by mokhet

Changed 14 years ago by mokhet

Changed 14 years ago by mokhet

Changed 14 years ago by mokhet

Changed 14 years ago by mokhet

Changed 14 years ago by mokhet

Changed 14 years ago by mokhet

Changed 14 years ago by mokhet

comment:1 Changed 14 years ago by gogo

In checkbackspace, there should be a comment before each of the functions to explain that one is DOM and one is IE. Otherwise I say commit this stuff. This is a good start to the original plan for V2 of breaking htmlarea.js into a core file, and a special layer for each browser.

The redundant code is indeed redundant, I probably left it there incase it came in handy if we ever need autoWrap for IE. The gecko only code there is for handling the automatic linking of url-looking and email-looking text.. eg type in blah@… and it will make it into a mailto: link automatically. It also handles undoing that automatic creation by hitting undo, or escape immediately after it auto-creates. Finally it handles updating those automatic links if you later go back and edit the text, eg if I go change blah@… to foobar@… the mailto link will be updated accordingly - that is the reason for the timeout, because we don't want to update the link on every keypress, just when the user has finished editing that auto-linked text.

comment:2 Changed 14 years ago by mokhet

  • Owner changed from gogo to mokhet
  • Status changed from new to assigned

actually applying the patches and reformating a bit to statisfy BSD style in the updated parts.

But, in case someone is trying, i wanted to let everyone know to not apply the striptags.patch, because it missing one brace in the submit file.

comment:3 Changed 14 years ago by mokhet

Humm, i have started by applying all the patches and i wanted to submit all at once in one changeset, but now it seems a bad idea. If i'm unlucky and breaking something i think it will be easier to find out what is broke if every function update have their own changeset. I'm testing all my changes but committing on the core file is scarying me like hell and i dont want to break it.

comment:4 Changed 14 years ago by mokhet

stripTag.patch applied in changeset:450

comment:5 Changed 14 years ago by mokhet

insertNodeAtSelection.patche applied in changeset:451

comment:6 Changed 14 years ago by mokhet

getParentElement.patch applied in changeset:452

comment:7 Changed 14 years ago by mokhet

activeElement.patch applied in changeset:453

comment:8 Changed 14 years ago by mokhet

_selectionEmpty.patch applied in changeset:454

comment:9 Changed 14 years ago by mokhet

selectNodeContents.patch applied in changeset:455

comment:10 Changed 14 years ago by mokhet

insertHTML.patch applied in changeset:456

comment:11 Changed 14 years ago by mokhet

getSelectedHTML.patch applied in changeset:457

comment:12 Changed 14 years ago by mokhet

_getSelection.patch applied in changeset:458

comment:13 Changed 14 years ago by mokhet

_createRange.patch applied in changeset:459

comment:14 Changed 14 years ago by mokhet

getOuterHTML.patch applied in changeset:460

comment:15 Changed 14 years ago by mokhet

  • Resolution set to fixed
  • Status changed from assigned to closed

checkBackspace.patch applied in changeset:461

and this is closing this ticket, at least for now :) I have tested every changeset, steps after steps and everything is still working the same way. I think nothing is broke, you cant imagine how scare i am. lol

thanks gogo for the checkbackspace explanation, now i understand why there is a setTimeout
You asked me to add a comment on the checkbackspace declaration, but i have no clue about what i should write, so i've write :

  // this function is for IE
  // this function is for DOM
Note: See TracTickets for help on using tickets.